[vlc-devel] [PATCH 1/3] skins2: window_manager: use C++ for: loops

Tristan Matthews le.businessman at gmail.com
Sun Apr 4 21:04:47 UTC 2021


Hi,

On Sun, Apr 4, 2021 at 10:38 AM Alexandre Janniaux <ajanni at videolabs.io> wrote:
>
> ---
>  modules/gui/skins2/src/window_manager.cpp | 149 ++++++++++------------
>  1 file changed, 64 insertions(+), 85 deletions(-)
>
> diff --git a/modules/gui/skins2/src/window_manager.cpp b/modules/gui/skins2/src/window_manager.cpp
> index 9663b79721..90f901bcf0 100644
> --- a/modules/gui/skins2/src/window_manager.cpp
> +++ b/modules/gui/skins2/src/window_manager.cpp
> @@ -156,11 +156,8 @@ void WindowManager::move( TopWindow &rWindow, int left, int top ) const
>      checkAnchors( &rWindow, xOffset, yOffset );
>
>      // Move all the windows
> -    WinSet_t::const_iterator it;
> -    for( it = m_movingWindows.begin(); it != m_movingWindows.end(); ++it )
> -    {
> -        (*it)->move( (*it)->getLeft() + xOffset, (*it)->getTop() + yOffset );
> -    }
> +    for( auto window : m_movingWindows )
> +        window->move( window->getLeft() + xOffset, window->getTop() + yOffset );
>  }
>
>
> @@ -183,29 +180,27 @@ void WindowManager::startResize( GenericLayout &rLayout, Direction_t direction )
>      const AncList_t &ancList1 = rLayout.getAnchorList();
>
>      // Iterate through all the hanged windows
> -    for( itWin = m_dependencies[rLayout.getWindow()].begin();
> -         itWin != m_dependencies[rLayout.getWindow()].end(); ++itWin )
> +    for( auto window : m_dependencies[rLayout.getWindow()] )
>      {
>          // Now, check for anchoring between the 2 windows
>          const AncList_t &ancList2 =
> -            (*itWin)->getActiveLayout().getAnchorList();
> -        for( itAnc1 = ancList1.begin(); itAnc1 != ancList1.end(); ++itAnc1 )
> +            window->getActiveLayout().getAnchorList();
> +        for( auto anc1 : ancList1 )
>          {
> -            for( itAnc2 = ancList2.begin();
> -                 itAnc2 != ancList2.end(); ++itAnc2 )
> +            for( auto anc2 : ancList2 )
>              {
> -                if( (*itAnc1)->isHanging( **itAnc2 ) )
> +                if( anc1->isHanging( *anc2 ) )
>                  {
>                      // Add the dependencies of the hanged window to one of the
>                      // lists of moving windows
>                      Position::Ref_t aRefPos =
> -                        (*itAnc1)->getPosition().getRefLeftTop();
> +                        anc1->getPosition().getRefLeftTop();
>                      if( aRefPos == Position::kRightTop )
> -                        buildDependSet( m_resizeMovingE, *itWin );
> +                        buildDependSet( m_resizeMovingE, window );
>                      else if( aRefPos == Position::kLeftBottom )
> -                        buildDependSet( m_resizeMovingS, *itWin );
> +                        buildDependSet( m_resizeMovingS, window );
>                      else if( aRefPos == Position::kRightBottom )
> -                        buildDependSet( m_resizeMovingSE, *itWin );
> +                        buildDependSet( m_resizeMovingSE, window );
>                      break;
>                  }
>              }
> @@ -285,33 +280,32 @@ void WindowManager::resize( GenericLayout &rLayout,
>      rLayout.refreshAll();
>
>      // Move all the anchored windows
> -    WinSet_t::const_iterator it;
>      if( m_direction == kResizeE ||
>          m_direction == kResizeSE )
>      {
> -        for( it = m_resizeMovingE.begin(); it != m_resizeMovingE.end(); ++it )
> +        for( auto window : m_resizeMovingE )
>          {
> -            (*it)->move( (*it)->getLeft() + xNewOffset,
> -                         (*it)->getTop() );
> +            window->move( window->getLeft() + xNewOffset,
> +                          window->getTop() );
>          }
>      }
>      if( m_direction == kResizeS ||
>          m_direction == kResizeSE )
>      {
> -        for( it = m_resizeMovingS.begin(); it != m_resizeMovingS.end(); ++it )
> +        for( auto window : m_resizeMovingS )
>          {
> -            (*it)->move( (*it)->getLeft(),
> -                         (*it)->getTop( )+ yNewOffset );
> +            window->move( window->getLeft(),
> +                          window->getTop( )+ yNewOffset );
>          }
>      }
>      if( m_direction == kResizeE ||
>          m_direction == kResizeS ||
>          m_direction == kResizeSE )
>      {
> -        for( it = m_resizeMovingSE.begin(); it != m_resizeMovingSE.end(); ++it )
> +        for( auto window : m_resizeMovingSE )
>          {
> -            (*it)->move( (*it)->getLeft() + xNewOffset,
> -                         (*it)->getTop() + yNewOffset );
> +            window->move( window->getLeft() + xNewOffset,
> +                          window->getTop() + yNewOffset );
>          }
>      }
>  }
> @@ -365,13 +359,12 @@ void WindowManager::unmaximize( TopWindow &rWindow )
>
>  void WindowManager::synchVisibility() const
>  {
> -    WinSet_t::const_iterator it;
> -    for( it = m_allWindows.begin(); it != m_allWindows.end(); ++it )
> +    for( auto window : m_allWindows )
>      {
>          // Show the window if it has to be visible
> -        if( (*it)->getVisibleVar().get() )
> +        if( window->getVisibleVar().get() )
>          {
> -            (*it)->innerShow();
> +            window->innerShow();
>          }
>      }
>  }
> @@ -379,14 +372,13 @@ void WindowManager::synchVisibility() const
>
>  void WindowManager::saveVisibility()
>  {
> -    WinSet_t::const_iterator it;
>      m_savedWindows.clear();
> -    for( it = m_allWindows.begin(); it != m_allWindows.end(); ++it )
> +    for( auto window : m_allWindows )
>      {
>          // Remember the window if it is visible
> -        if( (*it)->getVisibleVar().get() )
> +        if( window->getVisibleVar().get() )
>          {
> -            m_savedWindows.insert( *it );
> +            m_savedWindows.insert( window );
>          }
>      }
>  }
> @@ -400,10 +392,9 @@ void WindowManager::restoreVisibility() const
>          msg_Warn( getIntf(), "restoring visibility for no window" );
>      }
>
> -    WinSet_t::const_iterator it;
> -    for( it = m_savedWindows.begin(); it != m_savedWindows.end(); ++it )
> +    for( auto window : m_savedWindows )
>      {
> -        (*it)->show();
> +        window->show();
>      }
>  }
>
> @@ -411,10 +402,9 @@ void WindowManager::restoreVisibility() const
>  void WindowManager::raiseAll() const
>  {
>      // Raise all the windows
> -    WinSet_t::const_iterator it;
> -    for( it = m_allWindows.begin(); it != m_allWindows.end(); ++it )
> +    for( auto window : m_allWindows )
>      {
> -        (*it)->raise();
> +        window->raise();
>      }
>  }
>
> @@ -422,14 +412,13 @@ void WindowManager::raiseAll() const
>  void WindowManager::showAll( bool firstTime ) const
>  {
>      // Show all the windows
> -    WinSet_t::const_iterator it;
> -    for( it = m_allWindows.begin(); it != m_allWindows.end(); ++it )
> +    for( auto window : m_allWindows )
>      {
>          // When the theme is opened for the first time,
>          // only show the window if set as visible in the XML
> -        if( (*it)->getInitialVisibility() || !firstTime )
> +        if( window->getInitialVisibility() || !firstTime )
>          {
> -            (*it)->show();
> +            window->show();
>          }
>      }
>  }
> @@ -446,10 +435,9 @@ void WindowManager::show( TopWindow &rWindow ) const
>
>  void WindowManager::hideAll() const
>  {
> -    WinSet_t::const_iterator it;
> -    for( it = m_allWindows.begin(); it != m_allWindows.end(); ++it )
> +    for( auto window : m_allWindows )
>      {
> -        (*it)->hide();
> +        window->hide();
>      }
>  }
>
> @@ -461,10 +449,9 @@ void WindowManager::setOnTop( bool b_ontop )
>      pVarOnTop->set( b_ontop );
>
>      // set/unset the "on top" status
> -    WinSet_t::const_iterator it;
> -    for( it = m_allWindows.begin(); it != m_allWindows.end(); ++it )
> +    for( auto window : m_allWindows )
>      {
> -        (*it)->toggleOnTop( b_ontop );
> +        window->toggleOnTop( b_ontop );
>      }
>  }
>
> @@ -485,13 +472,12 @@ void WindowManager::buildDependSet( WinSet_t &rWinSet,
>
>      // Iterate through the anchored windows
>      const WinSet_t &anchored = m_dependencies[pWindow];
> -    WinSet_t::const_iterator iter;
> -    for( iter = anchored.begin(); iter != anchored.end(); ++iter )
> +    for( auto window : anchored )
>      {
>          // Check that the window isn't already in the set before adding it
> -        if( rWinSet.find( *iter ) == rWinSet.end() )
> +        if( rWinSet.find( window ) == rWinSet.end() )
>          {
> -            buildDependSet( rWinSet, *iter );
> +            buildDependSet( rWinSet, window );
>          }
>      }
>  }
> @@ -501,85 +487,78 @@ void WindowManager::checkAnchors( TopWindow *pWindow,
>                                    int &xOffset, int &yOffset ) const
>  {
>      (void)pWindow;
> -    WinSet_t::const_iterator itMov, itSta;
> -    AncList_t::const_iterator itAncMov, itAncSta;
>
>      // Check magnetism with screen edges first (actually it is the work area)
>      SkinsRect workArea = OSFactory::instance( getIntf() )->getWorkArea();
>      // Iterate through the moving windows
> -    for( itMov = m_movingWindows.begin();
> -         itMov != m_movingWindows.end(); ++itMov )
> +    for( auto moving : m_movingWindows )
>      {
>          // Skip the invisible windows
> -        if( ! (*itMov)->getVisibleVar().get() )
> +        if( !moving->getVisibleVar().get() )
>          {
>              continue;
>          }
>
> -        int newLeft = (*itMov)->getLeft() + xOffset;
> -        int newTop = (*itMov)->getTop() + yOffset;
> +        int newLeft = moving->getLeft() + xOffset;
> +        int newTop = moving->getTop() + yOffset;
>          if( newLeft > workArea.getLeft() - m_magnet &&
>              newLeft < workArea.getLeft() + m_magnet )
>          {
> -            xOffset = workArea.getLeft() - (*itMov)->getLeft();
> +            xOffset = workArea.getLeft() - moving->getLeft();
>          }
>          if( newTop > workArea.getTop() - m_magnet &&
>              newTop < workArea.getTop() + m_magnet )
>          {
> -            yOffset = workArea.getTop() - (*itMov)->getTop();
> +            yOffset = workArea.getTop() - moving->getTop();
>          }
>          int right = workArea.getLeft() + workArea.getWidth();
> -        if( newLeft + (*itMov)->getWidth() > right - m_magnet &&
> -            newLeft + (*itMov)->getWidth() < right + m_magnet )
> +        if( newLeft + moving->getWidth() > right - m_magnet &&
> +            newLeft + moving->getWidth() < right + m_magnet )
>          {
> -            xOffset = right - (*itMov)->getLeft() - (*itMov)->getWidth();
> +            xOffset = right - moving->getLeft() - moving->getWidth();
>          }
>          int bottom = workArea.getTop() + workArea.getHeight();
> -        if( newTop + (*itMov)->getHeight() > bottom - m_magnet &&
> -            newTop + (*itMov)->getHeight() <  bottom + m_magnet )
> +        if( newTop + moving->getHeight() > bottom - m_magnet &&
> +            newTop + moving->getHeight() <  bottom + m_magnet )
>          {
> -            yOffset =  bottom - (*itMov)->getTop() - (*itMov)->getHeight();
> +            yOffset =  bottom - moving->getTop() - moving->getHeight();
>          }
>      }
>
>      // Iterate through the moving windows
> -    for( itMov = m_movingWindows.begin();
> -         itMov != m_movingWindows.end(); ++itMov )
> +    for( auto moving : m_movingWindows )
>      {
>          // Skip the invisible windows
> -        if( ! (*itMov)->getVisibleVar().get() )
> +        if( !moving->getVisibleVar().get() )
>          {
>              continue;
>          }
>
>          // Get the anchors in the main layout of this moving window
>          const AncList_t &movAnchors =
> -            (*itMov)->getActiveLayout().getAnchorList();
> +            moving->getActiveLayout().getAnchorList();
>
>          // Iterate through the static windows
> -        for( itSta = m_allWindows.begin();
> -             itSta != m_allWindows.end(); ++itSta )
> +        for( auto window : m_allWindows )
>          {
>              // Skip the moving windows and the invisible ones
> -            if( m_movingWindows.find( (*itSta) ) != m_movingWindows.end() ||
> -                ! (*itSta)->getVisibleVar().get() )
> +            if( m_movingWindows.find( window ) != m_movingWindows.end() ||
> +                ! window->getVisibleVar().get() )
>              {
>                  continue;
>              }
>
>              // Get the anchors in the main layout of this static window
>              const AncList_t &staAnchors =
> -                (*itSta)->getActiveLayout().getAnchorList();
> +                window->getActiveLayout().getAnchorList();
>
>              // Check if there is an anchoring between one of the movAnchors
>              // and one of the staAnchors
> -            for( itAncMov = movAnchors.begin();
> -                 itAncMov != movAnchors.end(); ++itAncMov )
> +            for( auto movAnchor : movAnchors )
>              {
> -                for( itAncSta = staAnchors.begin();
> -                     itAncSta != staAnchors.end(); ++itAncSta )
> +                for( auto staAnchor : staAnchors )
>                  {
> -                    if( (*itAncSta)->canHang( **itAncMov, xOffset, yOffset ) )
> +                    if( staAnchor->canHang( *movAnchor, xOffset, yOffset ) )
>                      {
>                          // We have found an anchoring!
>                          // There is nothing to do here, since xOffset and
> @@ -593,8 +572,8 @@ void WindowManager::checkAnchors( TopWindow *pWindow,
>                          // Temporary variables
>                          int xOffsetTemp = -xOffset;
>                          int yOffsetTemp = -yOffset;
> -                        if( (*itAncMov)->canHang( **itAncSta, xOffsetTemp,
> -                                                  yOffsetTemp ) )
> +                        if( movAnchor->canHang( *staAnchor, xOffsetTemp,
> +                                                yOffsetTemp ) )
>                          {
>                              // We have found an anchoring!
>                              // xOffsetTemp and yOffsetTemp have been updated,
> --
> 2.31.1
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

The whole set LGTM (and this one in particular is a win for readability).

I haven't touched C++ in many moons so I was a bit curious about the
const_iterator to range (i.e. for auto loop) changes,I think there's a
slight change in that you could do mutable operations in the bodies of
these loops now? I think if you're only doing const ops in those any
way so it's not a big deal, it was just something that jumped out.

Best,
-t


More information about the vlc-devel mailing list