[vlc-devel] [PATCH 3/4] Qt: refactor vout resize
Rémi Denis-Courmont
remi at remlab.net
Mon Aug 8 22:50:37 CEST 2016
Le sunnuntaina 7. elokuuta 2016, 0.11.56 EEST Anatoliy Anischovich a écrit :
> ---
> modules/gui/qt/components/interface_widgets.cpp | 52 ++++++++-----------
> modules/gui/qt/components/interface_widgets.hpp | 11 +++-
> modules/gui/qt/main_interface.cpp | 68
> +++++++++++-------------- modules/gui/qt/main_interface.hpp |
> 4 +-
> 4 files changed, 62 insertions(+), 73 deletions(-)
>
> diff --git a/modules/gui/qt/components/interface_widgets.cpp
> b/modules/gui/qt/components/interface_widgets.cpp index db590c5..740d29f
> 100644
> --- a/modules/gui/qt/components/interface_widgets.cpp
> +++ b/modules/gui/qt/components/interface_widgets.cpp
> @@ -74,10 +74,10 @@
> **********************************************************************/
>
> VideoWidget::VideoWidget( intf_thread_t *_p_i )
> - : QFrame( NULL ) , p_intf( _p_i )
> + : QFrame( NULL ) , p_intf( _p_i ) , i_width( 0 ), i_height( 0 )
> {
> /* Set the policy to expand in both directions */
> - // setSizePolicy( QSizePolicy::Expanding, QSizePolicy::Expanding );
> + setSizePolicy( QSizePolicy::Expanding, QSizePolicy::Expanding );
>
> layout = new QHBoxLayout( this );
> layout->setContentsMargins( 0, 0, 0, 0 );
> @@ -106,8 +106,7 @@ void VideoWidget::sync( void )
> /**
> * Request the video to avoid the conflicts
> **/
> -WId VideoWidget::request( struct vout_window_t *p_wnd, unsigned int
> *pi_width,
> - unsigned int *pi_height, bool
> b_keep_size )
> +WId VideoWidget::request( struct vout_window_t *p_wnd )
I can see why you´d pass the requested width and height by value rather than
by reference. The return values are not actually used by the vout_window_t
front-end. But removing them completely does not seem right.
> {
> if( stable )
> {
> @@ -116,12 +115,6 @@ WId VideoWidget::request( struct vout_window_t *p_wnd,
> unsigned int *pi_width, }
> assert( !p_window );
>
> - if( b_keep_size )
> - {
> - *pi_width = size().width();
> - *pi_height = size().height();
> - }
> -
> /* The owner of the video window needs a stable handle (WinId).
> Reparenting * in Qt4-X11 changes the WinId of the widget, so we need to
> create another * dummy widget that stays within the reparentable widget. */
> @@ -178,38 +171,35 @@ WId VideoWidget::request( struct vout_window_t
> *p_wnd, unsigned int *pi_width, return stable->winId();
> }
>
> +void VideoWidget::reportSize( int w, int h )
> +{
> + if( !p_window )
> + return;
> +
> + vout_window_ReportSize( p_window, w, h );
> +}
> +
> /* Set the Widget to the correct Size */
> /* Function has to be called by the parent
> Parent has to care about resizing itself */
> -void VideoWidget::setSize( unsigned int w, unsigned int h )
> +void VideoWidget::setSize( int w, int h )
Why? Dimensions cannot be negative. Unless you are connecting directly to a Qt
signal, I don´t see a reason to change the types.
> {
> - /* If the size changed, resizeEvent will be called, otherwise not,
> - * in which case we need to tell the vout what the size actually is
> - */
> - if( (unsigned)size().width() == w && (unsigned)size().height() == h )
> - {
> - if( p_window != NULL )
> - vout_window_ReportSize( p_window, w, h );
> - return;
> - }
> + i_width = w;
> + i_height = h;
Calling vout_window_ReportSize() here indeed looks very wrong.
vout_window_ReportSize() MUST NOT be called until the window size is committed
all the way to/by the window manager.
But storing the dimensions to object members would seem to suffer essentially
the same flaw: blindly assuming that the window size will applied as
requested.
> - resize( w, h );
> emit sizeChanged( w, h );
> - /* Work-around a bug?misconception? that would happen when vout core
> resize - twice to the same size and would make the vout not centered.
> - This cause a small flicker.
> - See #3621
> - */
> - if( (unsigned)size().width() == w && (unsigned)size().height() == h )
> - updateGeometry();
> +
> sync();
> }
>
> +QSize VideoWidget::sizeHint() const
> +{
> + return QSize(i_width, i_height);
> +}
> +
> void VideoWidget::resizeEvent( QResizeEvent *event )
> {
> - if( p_window != NULL )
> - vout_window_ReportSize( p_window, event->size().width(),
> - event->size().height() );
> + reportSize( event->size().width(), event->size().height() );
>
> QWidget::resizeEvent( event );
> }
> diff --git a/modules/gui/qt/components/interface_widgets.hpp
> b/modules/gui/qt/components/interface_widgets.hpp index a07119e..dc64413
> 100644
> --- a/modules/gui/qt/components/interface_widgets.hpp
> +++ b/modules/gui/qt/components/interface_widgets.hpp
> @@ -59,7 +59,7 @@ public:
> VideoWidget( intf_thread_t * );
> virtual ~VideoWidget();
>
> - WId request( struct vout_window_t *, unsigned int *, unsigned int *,
> bool );
> + WId request( struct vout_window_t * );
> void release( void );
> void sync( void );
>
> @@ -70,6 +70,7 @@ protected:
> }
>
> virtual void resizeEvent(QResizeEvent *) Q_DECL_OVERRIDE;
> + virtual QSize sizeHint() const Q_DECL_OVERRIDE;
>
> private:
> intf_thread_t *p_intf;
> @@ -77,11 +78,17 @@ private:
>
> QWidget *stable;
> QLayout *layout;
> +
> + int i_width;
> + int i_height;
As noted above, this looks very suspicious to me.
> +
> + void reportSize( int, int );
> +
> signals:
> void sizeChanged( int, int );
>
> public slots:
> - void setSize( unsigned int, unsigned int );
> + void setSize( int, int );
> };
>
> /******************** Background Widget ****************/
> diff --git a/modules/gui/qt/main_interface.cpp
> b/modules/gui/qt/main_interface.cpp index 81eea83..5555fda 100644
> --- a/modules/gui/qt/main_interface.cpp
> +++ b/modules/gui/qt/main_interface.cpp
> @@ -218,10 +218,10 @@ MainInterface::MainInterface( intf_thread_t *_p_intf )
> : QVLCMW( _p_intf ) {
> CONNECT( videoWidget, sizeChanged( int, int ),
> this, videoSizeChanged( int, int ) );
> - }
> - CONNECT( this, askVideoToResize( unsigned int, unsigned int ),
> - this, setVideoSize( unsigned int, unsigned int ) );
> + CONNECT( this, askVideoToResize( int, int ),
> + this, setVideoSize( int, int ) );
>
> + }
> CONNECT( this, askVideoSetFullScreen( bool ),
> this, setVideoFullScreen( bool ) );
> }
> @@ -731,7 +731,7 @@ void MainInterface::getVideoSlot( WId *p_id, struct
> vout_window_t *p_wnd, toggleUpdateSystrayMenu();
>
> /* Request the videoWidget */
> - WId ret = videoWidget->request( p_wnd, pi_width, pi_height,
> !b_autoresize );
> + WId ret = videoWidget->request( p_wnd );
> *p_id = ret;
> if( ret ) /* The videoWidget is available */
> {
> @@ -741,8 +741,7 @@ void MainInterface::getVideoSlot( WId *p_id, struct
> vout_window_t *p_wnd, showVideo();
>
> /* Ask videoWidget to resize correctly, if we are in normal mode */
> - if( b_autoresize )
> - videoWidget->setSize( *pi_width, *pi_height );
> + emit askVideoToResize( *pi_width, *pi_height );
> }
> }
>
> @@ -777,43 +776,36 @@ void MainInterface::releaseVideoSlot( void )
> stackCentralOldWidget = bgWidget;
> }
>
> -void MainInterface::setVideoSize( unsigned int w, unsigned int h )
> +
> +/* Resize video widget to video size */
> +/* If the video size is too large for the screen, resize it to the screen
> + size. */
> +void MainInterface::setVideoSize( int w, int h )
> {
> - if (!isFullScreen() && !isMaximized() )
> + if( isFullScreen() || isMaximized() )
> + return;
Nothing wrong with deindenting as such, but if you bundle it with another
change, it makes review hard.
> +
> + QRect screen = QApplication::desktop()->availableGeometry();
> + if( h > screen.height() )
> {
> - /* Resize video widget to video size, or keep it at the same
> - * size. Call setSize() either way so that vout_window_ReportSize
> - * will always get called.
> - * If the video size is too large for the screen, resize it
> - * to the screen size.
> - */
> - if (b_autoresize)
> + w = screen.width();
> + h = screen.height();
> + if( !b_minimalView )
> {
> - QRect screen = QApplication::desktop()->availableGeometry();
> - if( h > screen.height() )
> - {
> - w = screen.width();
> - h = screen.height();
> - if( !b_minimalView )
> - {
> - if( menuBar()->isVisible() )
> - h -= menuBar()->height();
> - if( controls->isVisible() )
> - h -= controls->height();
> - if( statusBar()->isVisible() )
> - h -= statusBar()->height();
> - if( inputC->isVisible() )
> - h -= inputC->height();
> - }
> - h -= style()->pixelMetric(QStyle::PM_TitleBarHeight);
> - h -= style()->pixelMetric(QStyle::PM_LayoutBottomMargin);
> - h -= 2 *
> style()->pixelMetric(QStyle::PM_DefaultFrameWidth); - }
> - videoWidget->setSize( w, h );
> + if( menuBar()->isVisible() )
> + h -= menuBar()->height();
> + if( controls->isVisible() )
> + h -= controls->height();
> + if( statusBar()->isVisible() )
> + h -= statusBar()->height();
> + if( inputC->isVisible() )
> + h -= inputC->height();
> }
> - else
> - videoWidget->setSize( videoWidget->width(),
> videoWidget->height() );
> + h -=
> style()->pixelMetric(QStyle::PM_TitleBarHeight);
> + h -= style()->pixelMetric(QStyle::PM_LayoutBottomMargin);
> + h -= 2 * style()->pixelMetric(QStyle::PM_DefaultFrameWidth);
There should be a comment explaining the constant here. I for one don´t
understand why it´s there.
> }
> + videoWidget->setSize( w, h );
> }
>
> void MainInterface::videoSizeChanged( int w, int h )
> diff --git a/modules/gui/qt/main_interface.hpp
> b/modules/gui/qt/main_interface.hpp index 18ab8ce..e4cf8e9 100644
> --- a/modules/gui/qt/main_interface.hpp
> +++ b/modules/gui/qt/main_interface.hpp
> @@ -252,7 +252,7 @@ private slots:
> debug();
> }
>
> - void setVideoSize( unsigned int, unsigned int );
> + void setVideoSize( int, int );
> void videoSizeChanged( int, int );
> void setVideoFullScreen( bool );
> void setVideoOnTop( bool );
> @@ -267,7 +267,7 @@ signals:
> void askGetVideo( WId *, struct vout_window_t *, unsigned *, unsigned
> *, bool );
> void askReleaseVideo( );
> - void askVideoToResize( unsigned int, unsigned int );
> + void askVideoToResize( int, int );
> void askVideoSetFullScreen( bool );
> void askVideoOnTop( bool );
> void minimalViewToggled( bool );
--
Rémi Denis-Courmont
http://www.remlab.net/
More information about the vlc-devel
mailing list