CompoundEditor-related fixes

Changes.md

@@ -1,7 +1,16 @@
 1.6.x.x (relative to 1.6.8.0)
 =======
 
+Fixes
+-----
 
+- Layouts (#4197) :
+  - Stopped detached panels appearing separately in window manager task bars and tab switchers.
+  - Detached panels now minimise and raise as a group with the main window.
+  - Fixed window title for detached panels. It is now always synchronised with the title of the main window.
+- OpenColorIO :
+  - Fixed display transform used by editors in detached panels. This problem was particularly noticeable in the LightEditor.
+  - Fixed display transform used by newly added editors. This was also particularly noticeable for the LightEditor.
 
 1.6.8.0 (relative to 1.6.7.0)
 =======

python/GafferUI/CompoundEditor.py

@@ -175,16 +175,12 @@ def _detachedPanels( self ) :
 	def _createDetachedPanel( self, *args, **kwargs ) :
 
 		panel = _DetachedPanel( self,  *args, **kwargs )
-		panel.__removeOnCloseConnection = panel.closedSignal().connect( lambda w : w.parent()._removeDetachedPanel( w ), scoped = True )
+		panel.__removeOnCloseConnection = panel.closedSignal().connect( lambda w : w.compoundEditor()._removeDetachedPanel( w ), scoped = True )
 		panel.keyPressSignal().connect( CompoundEditor.__keyPress )
 
 		scriptWindow = self.ancestor( GafferUI.ScriptWindow )
 		if scriptWindow :
-			panel.setTitle( scriptWindow.getTitle() )
-			weakSetTitle = Gaffer.WeakMethod( panel.setTitle )
-			panel.__titleChangedConnection = scriptWindow.titleChangedSignal().connect( lambda w, t : weakSetTitle( t ), scoped = True )
-			# It's not directly in the qt hierarchy so shortcut events don't make it to the MenuBar
-			scriptWindow.menuBar().addShortcutTarget( panel )
+			scriptWindow.addChildWindow( panel, removeOnClose = True )
 
 		self.__detachedPanels.append( panel )
 		return panel
@@ -193,10 +189,8 @@ def _removeDetachedPanel( self, panel ) :
 
 		self.__detachedPanels.remove( panel )
 		panel.__removeOnCloseConnection = None
-		panel.__titleChangedConnection = None
-		panel._applyVisibility()
+		panel.close()
 
-		assert( not panel.visible() )
 		GafferUI.WidgetAlgo.keepUntilIdle( panel )
 
 	def __visibilityChanged(self, widget) :
@@ -207,11 +201,12 @@ def __visibilityChanged(self, widget) :
 
 	def __parentChanged( self, widget ) :
 
-		# Make sure we have the correct keyboard shortcut listeners
 		scriptWindow = self.ancestor( GafferUI.ScriptWindow )
 		if scriptWindow is not None :
+			# If we created any detached panels before we were
+			# in a ScriptWindow, add them to the ScriptWindow now.
 			for panel in self._detachedPanels() :
-				scriptWindow.menuBar().addShortcutTarget( panel )
+				scriptWindow.addChildWindow( panel, removeOnClose = True )
 
 	def __repr__( self ) :
 
@@ -434,6 +429,22 @@ def __handlePosition( splitContainer ) :
 # > base classes but instead using a has-a relationship. We could also move
 # > the keypress handling out of CompoundEditor.
 
+# Utility to find the owning CompoundEditor for any widget. This is complicated
+# slightly by the fact that _DetachedPanels are parented to the ScriptWindow
+# rather than the CompoundEditor.
+## \todo Perhaps it makes sense for all of CompoundEditor to be subsumed into
+# ScriptWindow, so they are one and the same?
+def _owningCompoundEditor( widget ) :
+
+	while widget is not None :
+		if isinstance( widget, CompoundEditor ) :
+			return widget
+		elif isinstance( widget, _DetachedPanel ) :
+			return widget.compoundEditor()
+		widget = widget.parent()
+
+	return None
+
 # The internal class used to allow hierarchical splitting of the layout.
 class _SplitContainer( GafferUI.SplitContainer ) :
 
@@ -509,17 +520,14 @@ def __findContainer( w, editorType ) :
 
 		container[0].addEditor( editor )
 
-	def serialiseChildren( self, scriptNode = None ) :
-
-		if not scriptNode :
-			scriptNode = self.ancestor( GafferUI.CompoundEditor ).scriptNode()
+	def serialiseChildren( self ) :
 
 		if self.isSplit() :
 			sizes = self.getSizes()
 			splitPosition = ( float( sizes[0] ) / sum( sizes ) ) if sum( sizes ) else 0
 			return "( GafferUI.SplitContainer.{}, {}, ( {}, {} ) )".format(
 				str( self.getOrientation() ), splitPosition,
-				self[0].serialiseChildren( scriptNode ), self[1].serialiseChildren( scriptNode )
+				self[0].serialiseChildren(), self[1].serialiseChildren()
 			)
 		else :
 			# not split - a tabbed container full of editors
@@ -598,7 +606,7 @@ def __init__( self, cornerWidget=None, **kw ) :
 	def addEditor( self, nameOrEditor ) :
 
 		if isinstance( nameOrEditor, str ) :
-			editor = GafferUI.Editor.create( nameOrEditor, self.ancestor( CompoundEditor ).scriptNode() )
+			editor = GafferUI.Editor.create( nameOrEditor, _owningCompoundEditor( self ).scriptNode() )
 		else :
 			editor = nameOrEditor
 
@@ -677,7 +685,7 @@ def __layoutMenuDefinition( self ) :
 
 		m = IECore.MenuDefinition()
 
-		layouts = GafferUI.Layouts.acquire( self.ancestor( CompoundEditor ).scriptNode().applicationRoot() )
+		layouts = GafferUI.Layouts.acquire( _owningCompoundEditor( self ).scriptNode().applicationRoot() )
 		for c in sorted( layouts.registeredEditors() ) :
 			m.append( "/" + IECore.CamelCase.toSpaced( c ), { "command" : functools.partial( Gaffer.WeakMethod( self.addEditor ), c ) } )
 
@@ -819,7 +827,7 @@ def __detachTab( self, index = None ) :
 
 		editor = self.getCurrent() if index is None else self[ index ]
 
-		window = self.ancestor( GafferUI.CompoundEditor )._createDetachedPanel()
+		window = _owningCompoundEditor( self )._createDetachedPanel()
 		self.__matchWindowToWidget( window, editor, 10 )
 
 		self.removeEditor( editor )
@@ -833,7 +841,7 @@ def __detachPanel( self ) :
 		# collapsed our old parent split container and won't be able to find them
 		splitContainer = self.ancestor( _SplitContainer )
 
-		window = self.ancestor( GafferUI.CompoundEditor )._createDetachedPanel()
+		window = _owningCompoundEditor( self )._createDetachedPanel()
 		self.__matchWindowToWidget( window, splitContainer, 10 )
 
 		# We must join the parent or we end up with nested split containers in the hierarchy
@@ -879,30 +887,26 @@ def __removePanel( self ) :
 # configuration is exposed.
 class _DetachedPanel( GafferUI.Window ) :
 
-	def __init__( self, parentEditor, children = None, windowState = None ) :
+	def __init__( self, compoundEditor, children = None, windowState = None ) :
 
 		GafferUI.Window.__init__( self )
 
 		self.__splitContainer = _SplitContainer( borderWidth = 2 )
 		self.__splitContainer.append( _TabbedContainer() )
 		self.setChild( self.__splitContainer )
 
-		# @see parent() As we can't be moved between CompoundEditors
-		# (scriptNode references in editors will be wrong), we don't need
-		# accessors for this.
-		self.__parentEditor = weakref.ref( parentEditor )
+		self.__compoundEditor = weakref.ref( compoundEditor )
 
 		self.__splitContainer.restoreChildren( children )
 
 		self.__windowState = windowState or {}
 
-	## As were technically not in the Qt hierarchy, but do want to be logically
-	# owned by a CompoundEditor, we re-implement this to re-direct ancestor
-	# calls to which ever editor we were added to (CompoundEditor fills this
-	# in for us when we're constructed).
-	def parent( self ) :
+		self.parentChangedSignal().connect( Gaffer.WeakMethod( self.__parentChanged ) )
+
+	# The CompoundEditor this panel belongs to.
+	def compoundEditor( self ) :
 
-		return self.__parentEditor() if self.__parentEditor else None
+		return self.__compoundEditor()
 
 	def editors( self, type = GafferUI.Editor ) :
 
@@ -945,6 +949,21 @@ def reprArgs( self ) :
 	def _splitContainer( self ) :
 		return self.__splitContainer
 
+	def __parentChanged( self, widget ) :
+
+		if self.parent() is None :
+			return
+
+		assert( isinstance( self.parent(), GafferUI.ScriptWindow ) )
+		self.setTitle( self.parent().getTitle() )
+		self.parent().titleChangedSignal().connect( Gaffer.WeakMethod( self.__scriptWindowTitleChanged ) )
+		self.parent().menuBar().addShortcutTarget( self )
+
+	def __scriptWindowTitleChanged( self, window, title ) :
+
+		# Mirror the ScriptWindow's title onto our own.
+		self.setTitle( title )
+
 ## An internal eventFilter class managing all tab drag-drop events and logic.
 # Tab dragging is an exception and is implemented entirely using mouse-move
 # events. This was the only practical option after trying all combinations of
@@ -1133,7 +1152,7 @@ def __mouseRelease( self, _, event ) :
 
 				oldSize = self.__draggedTab._qtWidget().rect()
 
-				window = sourceContainer.ancestor( GafferUI.CompoundEditor )._createDetachedPanel()
+				window = _owningCompoundEditor( sourceContainer )._createDetachedPanel()
 
 				# We have to add the editor early so we can work out the center of
 				# the tab header widget otherwise it has no parent so no tab bar...
@@ -1355,8 +1374,7 @@ def __constrainGlobalPosTo( self, globalPos, qTabBar ) :
 	def __removeDetachedPanelIfEmpty( detachedPanel ) :
 
 		if detachedPanel and detachedPanel.isEmpty() :
-			parentEditor = detachedPanel.ancestor( CompoundEditor )
-			parentEditor._removeDetachedPanel( detachedPanel )
+			_owningCompoundEditor( detachedPanel )._removeDetachedPanel( detachedPanel )
 
 	@staticmethod
 	def __tryToRaiseWindow( qWindow ) :

python/GafferUI/Widget.py

@@ -1300,26 +1300,27 @@ def __contextMenu( self, qObject, qEvent ) :
 
 	def __parentChange( self, qObject, qEvent ) :
 
+		widget = Widget._owner( qObject )
+		if isinstance( widget, ( GafferUI.Window, GafferUI.Editor ) ) :
+			# Strictly speaking, the reparenting of _any_ widget might require
+			# us to propagate display transform changes. But in practice we
+			# don't currently need that, and don't want to incur the expense
+			# either. So we just propagate changes when windows and editors
+			# are reparented.
+			parent = widget.parent()
+			if widget.getDisplayTransform() is None and parent is not None and parent.displayTransform() is not None :
+				widget._Widget__propagateDisplayTransformChange()
+
 		## \todo It might be nice to investigate having the
 		# the signature for this signal match that of
 		# GraphComponent::parentChangedSignal(), which takes
 		# an additional argument for the previous parent. We
 		# may be able to get the value for that from a
 		# ParentAboutToChange event.
-		widget = Widget._owner( qObject )
 		if widget._parentChangedSignal is not None :
 			widget._parentChangedSignal( widget )
 			return True
 
-		if isinstance( widget, GafferUI.Window ) :
-			# Strictly speaking, the reparenting of _any_ widget might require
-			# use to propagate display transform changes. But in practice we
-			# don't currently need that, and don't want to incure the expense
-			# either. So we just propagate changes when windows are reparented.
-			parent = widget.parent()
-			if widget.getDisplayTransform() is None and parent is not None and parent.displayTransform() is not None :
-				widget._Widget__propagateDisplayTransformChange()
-
 		return False
 
 	# Although Qt has a drag and drop system, we ignore it and implement our

python/GafferUITest/CompoundEditorTest.py

@@ -103,6 +103,43 @@ def testDetachedPanelsLifetime( self ) :
 
 		self.assertEqual( wp(), None )
 
+	def testDetachedPanelInheritsDisplayTransform( self ) :
+
+		class CapturingEditor( GafferUI.Editor ) :
+
+			def __init__( self, scriptNode, **kw ) :
+
+				GafferUI.Editor.__init__( self, GafferUI.Label(), scriptNode, **kw )
+				self.displayTransformChanges = []
+
+			def _displayTransformChanged( self ) :
+
+				GafferUI.Editor._displayTransformChanged( self )
+				self.displayTransformChanges.append( self.displayTransform() )
+
+		displayTransform1 = lambda x : x * 1
+		displayTransform2 = lambda x : x * 2
+
+		script = Gaffer.ScriptNode()
+		editor = GafferUI.CompoundEditor( script )
+		scriptWindow = GafferUI.ScriptWindow.acquire( script )
+		scriptWindow.setDisplayTransform( displayTransform1 )
+		scriptWindow.setLayout( editor )
+		self.assertIs( editor.displayTransform(), displayTransform1 )
+
+		panel = editor._createDetachedPanel()
+		capturingEditor = CapturingEditor( script )
+		panel.addEditor( capturingEditor )
+		self.assertIs( panel.displayTransform(), displayTransform1 )
+		self.assertIs( capturingEditor.displayTransform(), displayTransform1 )
+		self.assertEqual( capturingEditor.displayTransformChanges, [ displayTransform1 ] )
+
+		scriptWindow.setDisplayTransform( displayTransform2 )
+		self.assertIs( editor.displayTransform(), displayTransform2 )
+		self.assertIs( panel.displayTransform(), displayTransform2 )
+		self.assertIs( capturingEditor.displayTransform(), displayTransform2 )
+		self.assertEqual( capturingEditor.displayTransformChanges, [ displayTransform1, displayTransform2 ] )
+
 	def testReprLifetime( self ) :
 
 		s = Gaffer.ScriptNode()