PathListingWidget : Remove expandNonLeaf

While revisiting the logic in `PathModel::updateExpansion()` we decided to remove `expandNonLeaf` as it is rarely used and hard to understand.

Changes.md

@@ -91,6 +91,7 @@ Breaking Changes
 - ImageReader : Renamed `None` preset to `Automatic`.
 - OpenColorIOTransform : Removed `availableColorSpaces()` and `availableRoles()` methods.
 - OpenColorIO : Changed default config.
+- PathListingWidget : Removed `expandNonLeaf` argument from `setSelection()` and `setSelectedPaths()` methods.
 - Subprocess32 : Removed Python module.
 - Six : Removed Python module.
 - Gaffer : Class constructors are now declared explicit disabling implicit conversions.

python/GafferCortexUI/FileIndexedIOPathPreview.py

@@ -104,7 +104,7 @@ def __indexedIOPathChanged( self, path ) :
  with Gaffer.Signals.BlockedConnection( self.__pathListingSelectionChangedConnection ) :
  ## \todo This functionality might be nice in the PathChooserWidget. We could
  # maybe even use a PathChooserWidget here anyway.
- self.__pathListing.setSelection( IECore.PathMatcher( [ str( pathCopy ) ] ), expandNonLeaf=False )
+ self.__pathListing.setSelection( IECore.PathMatcher( [ str( pathCopy ) ] ) )
  # expand as people type forwards
  if len( pathCopy ) > len( self.__prevPath ) :
  self.__pathListing.setPathExpanded( pathCopy, True )

python/GafferSceneUI/HierarchyView.py

@@ -238,7 +238,7 @@ def __transferSelectionFromContext( self ) :
 
  selection = ContextAlgo.getSelectedPaths( self.getContext() )
  with Gaffer.Signals.BlockedConnection( self.__selectionChangedConnection ) :
- self.__pathListing.setSelection( selection, scrollToFirst=True, expandNonLeaf=False )
+ self.__pathListing.setSelection( selection, scrollToFirst=True )
 
 GafferUI.Editor.registerType( "HierarchyView", HierarchyView )
 

python/GafferSceneUI/LightEditor.py

@@ -300,7 +300,7 @@ def __transferSelectionFromContext( self ) :
  selectedPaths = ContextAlgo.getSelectedPaths( self.getContext() )
  with Gaffer.Signals.BlockedConnection( self.__selectionChangedConnection ) :
  selection = [selectedPaths] + ( [IECore.PathMatcher()] * ( len( self.__pathListing.getColumns() ) - 1 ) )
- self.__pathListing.setSelection( selection, scrollToFirst=True, expandNonLeaf=False )
+ self.__pathListing.setSelection( selection, scrollToFirst=True )
 
  def __buttonDoubleClick( self, pathListing, event ) :
 
@@ -573,7 +573,7 @@ def __buttonPress( self, pathListing, event ) :
  for p in selection :
  p.clear()
  selection[columnIndex].addPath( str( cellPath ) )
- pathListing.setSelection( selection, expandNonLeaf = False, scrollToFirst = False )
+ pathListing.setSelection( selection, scrollToFirst = False )
 
  menuDefinition = IECore.MenuDefinition()
 

python/GafferUI/PathListingWidget.py

@@ -333,7 +333,7 @@ def getSortable( self ) :
  ## Sets the currently selected paths. Accepts a single `IECore.PathMatcher`
  # for `Row` and `Rows` modes, and a list of `IECore.PathMatcher`, one for
  # each column, for `Cell` and `Cells` modes.
- def setSelection( self, paths, scrollToFirst=True, expandNonLeaf=True ) :
+ def setSelection( self, paths, scrollToFirst=True ) :
 
  if self.__rowSelectionMode() :
  assert( isinstance( paths, IECore.PathMatcher ) )
@@ -354,7 +354,7 @@ def setSelection( self, paths, scrollToFirst=True, expandNonLeaf=True ) :
  ) :
  raise ValueError( "More than one cell selected" )
 
- self.__setSelectionInternal( paths, scrollToFirst, expandNonLeaf )
+ self.__setSelectionInternal( paths, scrollToFirst )
 
  ## Returns an `IECore.PathMatcher` object containing the currently selected
  # paths for `Row` or `Rows` modes, and a list of `IECore.PathMatcher`
@@ -379,15 +379,15 @@ def getSelectedPaths( self ) :
  )
 
  ## \deprecated
- def setSelectedPaths( self, pathOrPaths, scrollToFirst=True, expandNonLeaf=True ) :
+ def setSelectedPaths( self, pathOrPaths, scrollToFirst=True ) :
 
  paths = pathOrPaths
  if isinstance( pathOrPaths, Gaffer.Path ) :
  paths = [ pathOrPaths ]
 
  self.setSelection(
  IECore.PathMatcher( [ str( path ) for path in paths ] ),
- scrollToFirst, expandNonLeaf
+ scrollToFirst
  )
 
  ## \deprecated Use getSelectedPaths() instead.
@@ -417,13 +417,13 @@ def getDragPointer( self ) :
 
  return self.__dragPointer
 
- def __setSelectionInternal( self, paths, scrollToFirst=True, expandNonLeaf=True ) :
+ def __setSelectionInternal( self, paths, scrollToFirst=True ) :
 
  paths = paths if isinstance( paths, list ) else [paths] * len( self.getColumns() )
 
  _GafferUI._pathListingWidgetSetSelection(
  GafferUI._qtAddress( self._qtWidget() ),
- paths, scrollToFirst, expandNonLeaf
+ paths, scrollToFirst
  )
 
  def __getSelectionInternal( self ) :
@@ -593,7 +593,7 @@ def __keyPress( self, widget, event ) :
  else :
  selection = [IECore.PathMatcher()] * len( self.getColumns() )
 
- self.__setSelectionInternal( selection, scrollToFirst=False, expandNonLeaf=False )
+ self.__setSelectionInternal( selection, scrollToFirst=False )
  return True
 
  return False
@@ -782,7 +782,7 @@ def __rangeSelect( self, index ) :
  ) :
  selection[i].addPaths( newPaths )
 
- self.__setSelectionInternal( selection, scrollToFirst=False, expandNonLeaf=False )
+ self.__setSelectionInternal( selection, scrollToFirst=False )
  self.__lastSelectedIndex = QtCore.QPersistentModelIndex( index )
 
  def __toggleSelect( self, index ) :
@@ -809,7 +809,7 @@ def __toggleSelect( self, index ) :
  # for doing keyboard-based expansion, and we can make use
  # of if in our Shift-click range selection.
  self._qtWidget().setCurrentIndex( index )
- self.__setSelectionInternal( selection, scrollToFirst=False, expandNonLeaf=False )
+ self.__setSelectionInternal( selection, scrollToFirst=False )
 
  self.__lastSelectedIndex = QtCore.QPersistentModelIndex( index )
 
@@ -822,7 +822,7 @@ def __singleSelect( self, index ) :
  paths = IECore.PathMatcher( [ path ] )
  elif self.__cellSelectionMode :
  paths = [ IECore.PathMatcher( [ path ] ) if i == index.column() else IECore.PathMatcher() for i in range( 0, len( self.getColumns() ) ) ]
- self.setSelection( paths, scrollToFirst=False, expandNonLeaf=False )
+ self.setSelection( paths, scrollToFirst=False )
 
  self.__lastSelectedIndex = QtCore.QPersistentModelIndex( index )
 

python/GafferUI/UIEditor.py

@@ -889,7 +889,7 @@ def __dragMove( self, listing, event ) :
 
  selection = self.__pathListing.getPath().copy()
  selection[:] = self.__dragItem.fullName().split( "." )
- self.__pathListing.setSelectedPaths( [ selection ], scrollToFirst = False, expandNonLeaf = False )
+ self.__pathListing.setSelectedPaths( [ selection ], scrollToFirst = False )
 
  return True
 

python/GafferUITest/PathListingWidgetTest.py

@@ -351,13 +351,13 @@ def testRowSelectionScrolling( self ) :
  self.assertTrue( w.getExpansion().isEmpty() )
 
  s = IECore.PathMatcher( [ "/1", "/2", "/9", "/2/5" ] )
- w.setSelection( s, expandNonLeaf = False, scrollToFirst = False )
+ w.setSelection( s, scrollToFirst = False )
  self.assertEqual( w.getSelection(), s )
  _GafferUI._pathModelWaitForPendingUpdates( GafferUI._qtAddress( w._qtWidget().model() ) )
  self.assertTrue( w.getExpansion().isEmpty() )
 
  s.addPath( "/3/5" )
- w.setSelection( s, expandNonLeaf = False, scrollToFirst = True )
+ w.setSelection( s, scrollToFirst = True )
  self.assertEqual( w.getSelection(), s )
  _GafferUI._pathModelWaitForPendingUpdates( GafferUI._qtAddress( w._qtWidget().model() ) )
  self.assertEqual( w.getExpansion(), IECore.PathMatcher( [ "/3" ] ) )
@@ -391,84 +391,23 @@ def testCellSelectionScrolling( self ) :
 
  s1 = IECore.PathMatcher( [ "/1", "/2", "/9", "/2/5" ] )
  s2 = IECore.PathMatcher( [ "/1", "/2", "/9", "/2/5" ] )
- w.setSelection( [ s1, s2 ], expandNonLeaf = False, scrollToFirst = False )
+ w.setSelection( [ s1, s2 ], scrollToFirst = False )
  self.assertEqual( w.getSelection(), [ s1, s2 ] )
  _GafferUI._pathModelWaitForPendingUpdates( GafferUI._qtAddress( w._qtWidget().model() ) )
  self.assertTrue( w.getExpansion().isEmpty() )
 
  s1.addPath( "/3/5" )
- w.setSelection( [ s1, s2 ], expandNonLeaf = False, scrollToFirst = True )
+ w.setSelection( [ s1, s2 ], scrollToFirst = True )
  self.assertEqual( w.getSelection(), [ s1, s2 ] )
  _GafferUI._pathModelWaitForPendingUpdates( GafferUI._qtAddress( w._qtWidget().model() ) )
  self.assertEqual( w.getExpansion(), IECore.PathMatcher( [ "/3" ] ) )
 
  s2.addPath( "/4/6" )
- w.setSelection( [ s1, s2 ], expandNonLeaf = False, scrollToFirst = True )
+ w.setSelection( [ s1, s2 ], scrollToFirst = True )
  self.assertEqual( w.getSelection(), [ s1, s2 ] )
  _GafferUI._pathModelWaitForPendingUpdates( GafferUI._qtAddress( w._qtWidget().model() ) )
  self.assertEqual( w.getExpansion(), IECore.PathMatcher( [ "/3", "/4" ] ) )
 
- def testRowSelectionExpansion( self ) :
-
- d = {}
- for i in range( 0, 10 ) :
- dd = {}
- for j in range( 0, 10 ) :
- dd[str(j)] = j
- d[str(i)] = dd
-
- p = Gaffer.DictPath( d, "/" )
-
- w = GafferUI.PathListingWidget(
- p,
- selectionMode = GafferUI.PathListingWidget.SelectionMode.Rows,
- displayMode = GafferUI.PathListingWidget.DisplayMode.Tree
- )
- _GafferUI._pathListingWidgetAttachTester( GafferUI._qtAddress( w._qtWidget() ) )
-
- self.assertTrue( w.getSelection().isEmpty() )
- self.assertTrue( w.getExpansion().isEmpty() )
-
- s = IECore.PathMatcher( [ "/1", "/2", "/9", "/2/5" ] )
- w.setSelection( s, expandNonLeaf = True, scrollToFirst = False )
- self.assertEqual( w.getSelection(), s )
- _GafferUI._pathModelWaitForPendingUpdates( GafferUI._qtAddress( w._qtWidget().model() ) )
- self.assertEqual( w.getExpansion(), IECore.PathMatcher( [ "/1", "/2", "/9" ] ) )
-
- def testCellSelectionExpansion( self ) :
-
- d = {}
- for i in range( 0, 10 ) :
- dd = {}
- for j in range( 0, 10 ) :
- dd[str(j)] = j
- d[str(i)] = dd
-
- p = Gaffer.DictPath( d, "/" )
-
- w = GafferUI.PathListingWidget(
- p,
- selectionMode = GafferUI.PathListingWidget.SelectionMode.Cells,
- displayMode = GafferUI.PathListingWidget.DisplayMode.Tree
- )
- _GafferUI._pathListingWidgetAttachTester( GafferUI._qtAddress( w._qtWidget() ) )
-
- # The default widget has multiple columns preset for file browsing,
- # just use two to simply testing.
- c = [ w.defaultNameColumn, w.StandardColumn( "h", "a" ) ]
- w.setColumns( c )
- self.assertEqual( w.getColumns(), c )
-
- self.assertEqual( w.getSelection(), [IECore.PathMatcher()] * 2 )
- self.assertTrue( w.getExpansion().isEmpty() )
-
- s1 = IECore.PathMatcher( [ "/1", "/2", "/9", "/2/5" ] )
- s2 = IECore.PathMatcher( [ "/1", "/3", "/4", "/9", "/4/6" ] )
- w.setSelection( [ s1, s2 ], expandNonLeaf = True, scrollToFirst = False )
- self.assertEqual( w.getSelection(), [ s1, s2 ] )
- _GafferUI._pathModelWaitForPendingUpdates( GafferUI._qtAddress( w._qtWidget().model() ) )
- self.assertEqual( w.getExpansion(), IECore.PathMatcher( [ "/1", "/2", "/3", "/4", "/9" ] ) )
-
  def testSelectionSignalFrequency( self ) :
 
  d = {
@@ -585,7 +524,7 @@ def testExpandedPathsWhenPathChangesWithSelection( self ) :
  self.assertEqual( w.getPathExpanded( pa ), False )
  self.assertEqual( w.getPathExpanded( pae ), False )
 
- w.setSelectedPaths( [ pa ], expandNonLeaf = False )
+ w.setSelectedPaths( [ pa ] )
  self.assertEqual( w.getPathExpanded( pa ), False )
  self.assertEqual( w.getPathExpanded( pae ), False )
 

src/GafferUIModule/PathListingWidgetBinding.cpp

@@ -401,7 +401,6 @@ class PathModel : public QAbstractItemModel
  m_sortColumn( -1 ),
  m_sortOrder( Qt::AscendingOrder ),
  m_modifyingTreeViewExpansion( false ),
- m_expandNonLeafSelection( false ),
  m_updateScheduled( false )
  {
  connect( parent, &QTreeView::expanded, this, &PathModel::treeViewExpanded );
@@ -538,12 +537,10 @@ class PathModel : public QAbstractItemModel
 
  // See comments for `setExpansion()`. The PathMatcher vector is our source of
  // truth, and we don't even use the QItemSelectionModel.
- void setSelection( const Selection &selectedPaths, bool scrollToFirst = true, bool expandNonLeaf = true )
+ void setSelection( const Selection &selectedPaths, bool scrollToFirst = true )
  {
  cancelUpdate();
 
- m_expandNonLeafSelection = expandNonLeaf;
-
  IECore::PathMatcher mergedPaths;
  for( auto &p : selectedPaths )
  {
@@ -566,7 +563,7 @@ class PathModel : public QAbstractItemModel
  // Copy, so can't be modified without `setSelection()` call.
  m_selection = Selection( selectedPaths );
 
- if( m_expandNonLeafSelection || m_scrollToCandidates )
+ if( m_scrollToCandidates )
  {
  m_rootItem->dirtyExpansion();
  scheduleUpdate();
@@ -1352,34 +1349,6 @@ class PathModel : public QAbstractItemModel
  }
  }
 
- if( model->m_expandNonLeafSelection )
- {
- // OK to read `m_selectedPaths` from background thread, because write on UI
- // thread is preceded by `cancelUpdate()`.
- const unsigned selectionMatch = model->m_selectedPaths.match( path->names() );
- /// \todo I don't understand the purpose of this logic. It seems it might be
- /// more useful to expand all ancestors of the selection (even if they're not
- /// selected themselves).
- if( selectionMatch & IECore::PathMatcher::ExactMatch && !path->isLeaf() )
- {
- model->queueEdit(
- [this, model] {
- QTreeView *treeView = dynamic_cast<QTreeView *>( model->QObject::parent() );
- const QModelIndex index = model->createIndex( m_row, 0, this );
- // Not setting `m_modifyingTreeViewExpansion`, because we _do_ want the
- // change to be reflected back in to `m_expandedPaths`.
- treeView->setExpanded( index, true );
- }
- );
- }
-
- if( selectionMatch & IECore::PathMatcher::DescendantMatch )
- {
- // Force creation of children so we can expand them.
- dirtyIfUnrequested( m_childItemsState );
- }
- }
-
  m_expansionDirty = false;
  }
 
@@ -1787,7 +1756,6 @@ class PathModel : public QAbstractItemModel
  // Parameters used to control expansion update following call to
  // `setSelection()`.
  std::optional<IECore::PathMatcher> m_scrollToCandidates;
- bool m_expandNonLeafSelection;
 
  std::unique_ptr<Gaffer::BackgroundTask> m_updateTask;
  bool m_updateScheduled;
@@ -1888,7 +1856,7 @@ IECore::PathMatcher getExpansion( uint64_t treeViewAddress )
  return model ? model->getExpansion() : IECore::PathMatcher();
 }
 
-void setSelection( uint64_t treeViewAddress, object pythonPaths, bool scrollToFirst, bool expandNonLeaf )
+void setSelection( uint64_t treeViewAddress, object pythonPaths, bool scrollToFirst )
 {
  QTreeView *treeView = reinterpret_cast<QTreeView *>( treeViewAddress );
  PathModel *model = dynamic_cast<PathModel *>( treeView->model() );
@@ -1897,7 +1865,7 @@ void setSelection( uint64_t treeViewAddress, object pythonPaths, bool scrollToFi
  boost::python::container_utils::extend_container( paths, pythonPaths );
 
  IECorePython::ScopedGILRelease gilRelease;
- model->setSelection( paths, scrollToFirst, expandNonLeaf );
+ model->setSelection( paths, scrollToFirst );
 }
 
 list getSelection( uint64_t treeViewAddress )