Merge pull request #6797 from johnhaddon/autoParentImprovement

Widget : Refactor auto-parenting

Author: johnhaddon

Changes.md

@@ -21,6 +21,9 @@ API
 
 - Metadata : `ValueFunctions` now receive a `target` parameter. This is particularly useful when registering a function against a wildcard pattern.
 - PlugAlgo : Added `RampffData` and `RampfColor3fData` support to `createPlugFromData()`.
+- Widget :
+  - Improved automatic parenting via the `with parent` syntax. Children are now guaranteed to be fully constructed before they are parented.
+  - Turned `toolTip`, `parenting` and `displayTransform` keyword-only constructor arguments.
 
 Breaking Changes
 ----------------
@@ -39,6 +42,7 @@ Breaking Changes
 - OSL Shaders : Replaced `Pattern/FloatSpline` and `Pattern/ColorSpline` with `Pattern/FloatRamp` and `Pattern/ColorRamp`. Old Gaffer scripts will be updated automatically on load, and when resaved will reference the new shaders. Note that the `.osl` files for the old shaders are still available, so that old USD files will continue to render.
 - GafferUI : Renamed SplineWidget to RampWidget. Renamed SplinePlugValueWidget to RampPlugValueWidget. The old RampPlugValueWidget is no longer exposed, since it was only used internally.
 - Metadata : Added `target` argument to `ValueFunction` signature.
+- Widget : The `toolTip`, `parenting` and `displayTransform` constructor arguments are no longer positional.
 
 Build
 -----

python/GafferSceneUI/SceneInspector.py

@@ -163,11 +163,12 @@ def __init__( self, scriptNode, **kw ) :
 				rootSection = "TopRow",
 			)
 
-			GafferUI.PlugLayout(
+			compareRow = GafferUI.PlugLayout(
 				self.settings(),
 				orientation = GafferUI.ListContainer.Orientation.Horizontal,
 				rootSection = "CompareRow",
 			)
+			compareRow.plugValueWidget( self.settings()["compare"] ).connect( self )
 
 			with GafferUI.TabbedContainer() :
 
@@ -1060,7 +1061,14 @@ def createLabel( text ) :
 
 		GafferUI.WidgetAlgo.joinEdges( row )
 
-		self.parentChangedSignal().connect( Gaffer.WeakMethod( self.__parentChanged ) )
+	# We don't have access to the parent SceneInspector when we're constructed,
+	# so we have to rely on it calling this method to allow us to connect our
+	# input label to follow the node set.
+	## \todo Perhaps Editor should be a node with the settings plugs parented
+	#  directly to it, in which case we'd have direct access.
+	def connect( self, sceneInspector ) :
+
+		self.__inputLabelWidget.connectToNodeSetWidget( sceneInspector )
 
 	def hasLabel( self ) :
 
@@ -1079,13 +1087,6 @@ def _updateFromValues( self, values, exception ) :
 				for x in range( 0, 2 ) :
 					grid[x,y].setVisible( enabled )
 
-	def __parentChanged( self, unused ) :
-
-		# We can't get access to the parent SceneInspector until we get parented,
-		# so we have to connect our input label to follow the node set in this
-		# awkward delayed fashion.
-		self.__inputLabelWidget.connectToNodeSetWidget( self.ancestor( GafferUI.NodeSetEditor ) )
-
 SceneInspector._ComparePlugValueWidget = _ComparePlugValueWidget
 
 def __contextMenu( column, pathListing, menuDefinition ) :

python/GafferUI/Widget.py

@@ -55,11 +55,35 @@ class _WidgetMetaclass( Gaffer.Signals.Trackable.__class__ ) :
 
 	def __call__( cls, *args, **kw ) :
 
+		cls.pushParent( None ) # Disable auto-parenting of private widgets created in constructor.
 		instance = type.__call__( cls, *args, **kw )
 		instance._postConstructor()
+		cls.popParent()
+
+		if len( cls.__parentStack ) and cls.__parentStack[-1] is not None :
+			# Perform automatic parenting if necessary. We don't want to do this
+			# for menus, because they don't have the same parenting semantics.
+			# If other types end up with similar requirements then we should
+			# probably just have a mechanism for them to say they don't want to
+			# participate rather than hardcoding stuff here.
+			if not isinstance( instance, GafferUI.Menu )  :
+				parenting = kw.get( "parenting", {} )
+				cls.__parentStack[-1].addChild( instance, **parenting )
 
 		return instance
 
+	__parentStack = []
+
+	@classmethod
+	def pushParent( cls, parent ) :
+
+		cls.__parentStack.append( parent )
+
+	@classmethod
+	def popParent( cls ) :
+
+		return cls.__parentStack.pop()
+
 ## The Widget class provides a base class for all widgets in GafferUI.
 #
 # When building UIs, you will typically derive from an existing Widget subclass such as Window,
@@ -123,7 +147,7 @@ class Widget( Gaffer.Signals.Trackable, metaclass = _WidgetMetaclass ) :
 	# If a current parent has been defined using the `with` syntax described above,
 	# the parenting argument may be passed as a dictionay of optional keywords for the
 	# automatic `parent.addChild()` call.
-	def __init__( self, topLevelWidget, toolTip="", parenting = None, displayTransform = None ) :
+	def __init__( self, topLevelWidget, *, toolTip="", parenting = None, displayTransform = None ) :
 
 		Gaffer.Signals.Trackable.__init__( self )
 
@@ -184,17 +208,6 @@ def __init__( self, topLevelWidget, toolTip="", parenting = None, displayTransfo
 		self.__visible = not isinstance( self, GafferUI.Window )
 		self.__displayTransform = displayTransform
 
-		# perform automatic parenting if necessary. we don't want to do this
-		# for menus, because they don't have the same parenting semantics. if other
-		# types end up with similar requirements then we should probably just have
-		# a mechanism for them to say they don't want to participate rather than
-		# hardcoding stuff here.
-		if len( self.__parentStack ) and not isinstance( self, GafferUI.Menu ) :
-			if self.__initNesting() == self.__parentStack[-1][1] + 1 :
-				if self.__parentStack[-1][0] is not None :
-					parenting = parenting or {}
-					self.__parentStack[-1][0].addChild( self, **parenting )
-
 		self.__eventFilterInstalled = False
 		# if a class has overridden getToolTip, then the tooltips
 		# may be dynamic based on context, and we need to display
@@ -768,31 +781,12 @@ def _pushParent( cls, container ) :
 
 		assert( isinstance( container, ( GafferUI.ContainerWidget, type( None ) ) ) )
 
-		cls.__parentStack.append( ( container, cls.__initNesting() ) )
+		cls.__class__.pushParent( container )
 
 	@classmethod
 	def _popParent( cls ) :
 
-		return cls.__parentStack.pop()[0]
-
-	# Returns how many Widgets are currently in construction
-	# on the call stack. We use this to avoid automatically
-	# parenting Widgets that are being created inside a constructor.
-	@staticmethod
-	def __initNesting() :
-
-		widgetsInInit = set()
-		frame = inspect.currentframe()
-		while frame :
-			if frame.f_code.co_name=="__init__" :
-				frameSelf = frame.f_locals[frame.f_code.co_varnames[0]]
-				if isinstance( frameSelf, Widget ) :
-					widgetsInInit.add( frameSelf )
-			frame = frame.f_back
-
-		return len( widgetsInInit )
-
-	__parentStack = []
+		return cls.__class__.popParent()
 
 	## Converts a Qt key code into a string
 	@classmethod

python/GafferUITest/ContainerWidgetTest.py

@@ -137,5 +137,24 @@ def __init__( self ) :
 		self.assertIsInstance( w.getChild()[0], GafferUI.TextWidget )
 		self.assertIsInstance( w.getChild()[1], GafferUI.Button )
 
+	def testChildFullyConstructedBeforeParenting( self ) :
+
+		class TestWidget( GafferUI.Label ) :
+
+			def __init__( self, **kw ) :
+
+				GafferUI.Label.__init__( self, **kw )
+
+				assert( self.parent() is None )
+
+			def _postConstructor( self ) :
+
+				assert( self.parent() is None )
+
+		with GafferUI.ListContainer() as container:
+			widget = TestWidget()
+
+		self.assertIs( widget.parent(), container )
+
 if __name__ == "__main__":
 	unittest.main()

python/GafferUITest/TabbedContainerTest.py

@@ -35,11 +35,14 @@
 #
 ##########################################################################
 
+import gc
 import unittest
 
 import GafferUI
 import GafferUITest
 
+from Qt import QtWidgets
+
 class TabbedContainerTest( GafferUITest.TestCase ) :
 
 	def test( self ) :
@@ -319,6 +322,21 @@ def testTabVisibility( self ) :
 		for tab in t2, t3 :
 			self.assertTrue( container.getTabVisible( tab ) )
 
+	def testWithSplitContainerParent( self ) :
+
+		# This exposes a problem whereby a TabbedContainer parented to
+		# a SplitContainer would create a zombie `QTabBar` instance in
+		# Python.
+		with GafferUI.SplitContainer() as splitContainer :
+			with GafferUI.TabbedContainer() :
+				GafferUI.Label( "Test" )
+
+		# There should be only one QTabBar in existence, and it should
+		# be valid.
+		qtTabBars = [ o for o in gc.get_objects() if isinstance( o, QtWidgets.QTabBar ) ]
+		self.assertEqual( len( qtTabBars ), 1 )
+		self.assertTrue( GafferUI._qtObjectIsValid( qtTabBars[0] ) )
+
 	def tearDown( self ) :
 
 		self.__current = None