[vlc-devel] [PATCH 2/2] qt: refresh time labels on display mode change

[Romain Vimont]

Thank you for the review.

On Mon, Apr 09, 2018 at 05:47:05PM +0200, Hugo Beauzée-Luyssen wrote:
> Hi,
> 
> Various nitpicks inline:
> 
> On Mon, Apr 9, 2018, at 3:05 PM, Romain Vimont wrote:
> > Toggling remaining-time/total-time did not update the time labels, so
> > they were only refreshed on the next positionUpdated() (which is not
> > called while paused).
> > 
> > Instead, store the position/time on every update, and refresh the time
> > labels immediately on remaining-time/total-time toggle.
> > 
> > Fixes 19810
> 
> Ticket reference misses a # :D

Yes :)

> 
> > ---
> >  modules/gui/qt/components/interface_widgets.cpp | 12 +++++++++++-
> >  modules/gui/qt/components/interface_widgets.hpp |  5 ++++-
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/modules/gui/qt/components/interface_widgets.cpp b/modules/
> > gui/qt/components/interface_widgets.cpp
> > index 5ce20a28e0..2f0959fbaa 100644
> > --- a/modules/gui/qt/components/interface_widgets.cpp
> > +++ b/modules/gui/qt/components/interface_widgets.cpp
> > @@ -944,12 +944,21 @@ TimeLabel::TimeLabel( intf_thread_t *_p_intf, 
> > TimeLabel::Display _displayType  )
> >  
> >  void TimeLabel::setRemainingTime( bool remainingTime )
> >  {
> > -    if (displayType != TimeLabel::Elapsed)
> > +    if( displayType != TimeLabel::Elapsed )
> > +    {
> >          b_remainingTime = remainingTime;
> > +        refresh();
> > +    }
> > +}
> > +
> > +void TimeLabel::refresh()
> > +{
> > +    setDisplayPosition( cachedPos, cachedTime, cachedLength );
> >  }
> >  
> >  void TimeLabel::setDisplayPosition( float pos, int64_t t, int length )
> >  {
> > +    cachedPos = pos;
> >      if( pos == -1.f )
> >      {
> >          setMinimumSize( QSize( 0, 0 ) );
> > @@ -1011,6 +1020,7 @@ void TimeLabel::setDisplayPosition( float pos, 
> > int64_t t, int length )
> >              break;
> >      }
> >      cachedLength = length;
> > +    cachedTime = t;
> >  }
> >  
> >  void TimeLabel::setDisplayPosition( float pos )
> > diff --git a/modules/gui/qt/components/interface_widgets.hpp b/modules/
> > gui/qt/components/interface_widgets.hpp
> > index a4353a0d1d..b83bbede74 100644
> > --- a/modules/gui/qt/components/interface_widgets.hpp
> > +++ b/modules/gui/qt/components/interface_widgets.hpp
> > @@ -198,7 +198,9 @@ protected:
> >  private:
> >      intf_thread_t *p_intf;
> >      bool b_remainingTime;
> > -    int cachedLength;
> > +    float cachedPos = -1;
> > +    int64_t cachedTime = 0;
> 
> Can't you infer time from cachedLength and cachedPos?

I kept all the parameters of the setDisplayPosition() slot
(positionUpdated() signal).

Maybe the length might not be provided (I don't know), or
cachedPos*cachedLength will give a different result (due to rounding
errors).

> > +    int cachedLength = 0;
> 
> I'm not so high on using inline initialization, especially for a commit that needs backport in a branch which aims at not using C++11. 
> Also here, it splits members initialization between the constructor initialization list & the members declaration, which reduces readability IMHO

OK, I'll move them to the constructor initialization list.

Before this change, cachedLength was not initialized at all.

> 
> >      TimeLabel::Display displayType;
> >  
> >      char psz_length[MSTRTIME_MAX_SIZE];
> > @@ -208,6 +210,7 @@ private slots:
> >      void setRemainingTime( bool );
> >      void setDisplayPosition( float pos, int64_t time, int length );
> >      void setDisplayPosition( float pos );
> > +    void refresh();
> 
> Does this needs to be a slot?

Absolutely not ;)

> >  signals:
> >      void broadcastRemainingTime( bool );
> >  };
> 
> -- 
>   Hugo Beauzée-Luyssen
>   hugo at beauzee.fr
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel