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

[Hugo Beauzée-Luyssen]

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

> ---
>  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?

> +    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

>      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?

>  signals:
>      void broadcastRemainingTime( bool );
>  };

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr