From 06c0274ed8276962d922e8f73e6e64e1f7645ab7 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 25 Jul 2024 15:59:28 +0100 Subject: [PATCH 1/3] PathColumn : Add `contextMenuSignal()` This allows columns to contribute to their own context menu, combined with the menu items defined by `PathListingWidget.columnContextMenuSignal()`. --- Changes.md | 1 + include/GafferUI/PathColumn.h | 28 ++++++ python/GafferUI/PathListingWidget.py | 8 +- src/GafferUI/PathColumn.cpp | 5 + src/GafferUIModule/PathColumnBinding.cpp | 111 +++++++++++++++++++++++ 5 files changed, 149 insertions(+), 4 deletions(-) diff --git a/Changes.md b/Changes.md index 2c044764140..e72a143ee0d 100644 --- a/Changes.md +++ b/Changes.md @@ -48,6 +48,7 @@ API - A `DeprecationWarning` is now emitted by `_plugConnections()`. Use `_blockedUpdateFromValues()` instead. - NodeGadget, ConnectionGadget : Added `updateFromContextTracker()` virtual methods. - Path : Added `inspectionContext()` virtual method. +- PathColumn : Added `contextMenuSignal()`, allowing the creation of custom context menus. Breaking Changes ---------------- diff --git a/include/GafferUI/PathColumn.h b/include/GafferUI/PathColumn.h index c265ce298d4..4d806967f94 100644 --- a/include/GafferUI/PathColumn.h +++ b/include/GafferUI/PathColumn.h @@ -50,6 +50,7 @@ namespace GafferUI { +class MenuDefinition; class PathListingWidget; /// Abstract class for extracting properties from a Path in a form @@ -162,6 +163,9 @@ class GAFFERUI_API PathColumn : public IECore::RefCounted, public Gaffer::Signal ButtonSignal &buttonReleaseSignal(); ButtonSignal &buttonDoubleClickSignal(); + using ContextMenuSignal = Gaffer::Signals::Signal>; + ContextMenuSignal &contextMenuSignal(); + private : PathColumnSignal m_changedSignal; @@ -169,6 +173,7 @@ class GAFFERUI_API PathColumn : public IECore::RefCounted, public Gaffer::Signal ButtonSignal m_buttonPressSignal; ButtonSignal m_buttonReleaseSignal; ButtonSignal m_buttonDoubleClickSignal; + ContextMenuSignal m_contextMenuSignal; SizeMode m_sizeMode; @@ -274,4 +279,27 @@ class PathListingWidget }; +/// C++ interface for the `IECore.MenuDefinition` Python class. Provided for use +/// in `PathColumn::contextMenuSignal()`, so that event handling may be +/// implemented from C++ if desired. +class MenuDefinition +{ + + public : + + struct MenuItem + { + using Command = std::function; + Command command; + std::string description; + std::string icon; + std::string shortCut; + bool divider = false; + bool active = true; + }; + + virtual void append( const std::string &path, const MenuItem &item ) = 0; + +}; + } // namespace GafferUI diff --git a/python/GafferUI/PathListingWidget.py b/python/GafferUI/PathListingWidget.py index 03a25c23751..4779823a9b1 100644 --- a/python/GafferUI/PathListingWidget.py +++ b/python/GafferUI/PathListingWidget.py @@ -766,7 +766,9 @@ def __dragEnd( self, widget, event ) : def __contextMenu( self, widget ) : - if not self.columnContextMenuSignal().numSlots() : + mousePosition = GafferUI.Widget.mousePosition( relativeTo = self ) + column = self.columnAt( mousePosition ) + if not column.contextMenuSignal().numSlots() and not self.columnContextMenuSignal().numSlots() : # Allow legacy clients connected to `Widget.contextMenuSignal()` to # do their own thing instead. return False @@ -774,9 +776,6 @@ def __contextMenu( self, widget ) : # Select the path under the mouse, if it's not already selected. # The user will expect to be operating on the thing under the mouse. - mousePosition = GafferUI.Widget.mousePosition( relativeTo = self ) - column = self.columnAt( mousePosition ) - path = self.pathAt( mousePosition ) if path is not None : path = str( path ) @@ -797,6 +796,7 @@ def __contextMenu( self, widget ) : # Use signals to build menu and display it. menuDefinition = IECore.MenuDefinition() + column.contextMenuSignal()( column, self, menuDefinition ) self.columnContextMenuSignal()( column, self, menuDefinition ) if menuDefinition.size() : diff --git a/src/GafferUI/PathColumn.cpp b/src/GafferUI/PathColumn.cpp index 8af129a815b..fd2a45795df 100644 --- a/src/GafferUI/PathColumn.cpp +++ b/src/GafferUI/PathColumn.cpp @@ -110,6 +110,11 @@ PathColumn::ButtonSignal &PathColumn::buttonDoubleClickSignal() return m_buttonDoubleClickSignal; } +PathColumn::ContextMenuSignal &PathColumn::contextMenuSignal() +{ + return m_contextMenuSignal; +} + ////////////////////////////////////////////////////////////////////////// // StandardPathColumn ////////////////////////////////////////////////////////////////////////// diff --git a/src/GafferUIModule/PathColumnBinding.cpp b/src/GafferUIModule/PathColumnBinding.cpp index a7448b7d87f..5d2952641a4 100644 --- a/src/GafferUIModule/PathColumnBinding.cpp +++ b/src/GafferUIModule/PathColumnBinding.cpp @@ -47,6 +47,7 @@ #include "IECorePython/RefCountedBinding.h" #include "IECorePython/ScopedGILLock.h" +#include "boost/mpl/vector.hpp" #include "boost/python/suite/indexing/container_utils.hpp" using namespace boost::python; @@ -147,6 +148,83 @@ class PathListingWidgetAccessor : public GafferUI::PathListingWidget } // namespace +////////////////////////////////////////////////////////////////////////// +// MenuDefinitionAccessor class +////////////////////////////////////////////////////////////////////////// + +namespace +{ + +struct GILReleaseMenuCommand +{ + + GILReleaseMenuCommand( MenuDefinition::MenuItem::Command command ) + : m_command( command ) + { + } + + void operator()() + { + IECorePython::ScopedGILRelease gilRelease; + m_command(); + } + + private : + + MenuDefinition::MenuItem::Command m_command; + +}; + +// Provides a C++ interface to the functionality implemented in the Python +// MenuDefinition class. +class MenuDefinitionAccessor : public GafferUI::MenuDefinition +{ + + public : + + MenuDefinitionAccessor( object menuDefinition ) + : m_menuDefinition( menuDefinition ) + { + } + + object menuDefinition() + { + return m_menuDefinition; + } + + void append( const std::string &path, const MenuItem &item ) override + { + IECorePython::ScopedGILLock gilLock; + + dict pythonItem; + + if( item.command != nullptr ) + { + pythonItem["command"] = make_function( + GILReleaseMenuCommand( item.command ), + boost::python::default_call_policies(), + boost::mpl::vector() + ); + } + + pythonItem["description"] = item.description; + pythonItem["icon"] = item.icon; + pythonItem["shortCut"] = item.shortCut; + pythonItem["divider"] = item.divider; + pythonItem["active"] = item.active; + + m_menuDefinition.attr( "append" )( path, pythonItem ); + } + + private : + + // The Python MenuDefinition object. + object m_menuDefinition; + +}; + +} // namespace + ////////////////////////////////////////////////////////////////////////// // Bindings ////////////////////////////////////////////////////////////////////////// @@ -330,6 +408,37 @@ struct ButtonSignalSlotCaller } }; +struct ContextMenuSignalCaller +{ + static void call( PathColumn::ContextMenuSignal &s, PathColumn &column, object pathListingWidget, object menuDefinition ) + { + PathListingWidgetAccessor pathListingWidgetAccessor( pathListingWidget ); + MenuDefinitionAccessor menuDefinitionAccessor( menuDefinition ); + IECorePython::ScopedGILRelease gilRelease; + s( column, pathListingWidgetAccessor, menuDefinitionAccessor ); + } +}; + +struct ContextMenuSignalSlotCaller +{ + void operator()( boost::python::object slot, PathColumn &column, PathListingWidget &pathListingWidget, MenuDefinition &menuDefinition ) + { + try + { + slot( + PathColumnPtr( &column ), + static_cast( pathListingWidget ).widget(), + static_cast( menuDefinition ).menuDefinition() + + ); + } + catch( const error_already_set & ) + { + IECorePython::ExceptionAlgo::translatePythonException(); + } + } +}; + template const char *pathColumnProperty( const T &column ) { @@ -384,6 +493,7 @@ void GafferUIModule::bindPathColumn() SignalClass, ChangedSignalSlotCaller>( "PathColumnSignal" ); SignalClass( "ButtonSignal" ); + SignalClass( "ContextMenuSignal" ); } pathColumnClass.def( init( arg( "sizeMode" ) = PathColumn::SizeMode::Default ) ) @@ -393,6 +503,7 @@ void GafferUIModule::bindPathColumn() .def( "buttonPressSignal", &PathColumn::buttonPressSignal, return_internal_reference<1>() ) .def( "buttonReleaseSignal", &PathColumn::buttonReleaseSignal, return_internal_reference<1>() ) .def( "buttonDoubleClickSignal", &PathColumn::buttonDoubleClickSignal, return_internal_reference<1>() ) + .def( "contextMenuSignal", &PathColumn::contextMenuSignal, return_internal_reference<1>() ) .def( "getSizeMode", (PathColumn::SizeMode (PathColumn::*)() const )&PathColumn::getSizeMode ) .def( "setSizeMode", &PathColumn::setSizeMode, ( arg( "sizeMode" ) ) ) ; From 8ffd4c591edfbc364f4281c0da3ff2234fb8206c Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 31 Jul 2024 09:44:22 +0100 Subject: [PATCH 2/3] PathColumn : Add `instanceCreatedSignal()` This will allow external code to connect to signals to customise column behaviours, no matter how columns are created. Most clients are expected to use `PathListingWidget` signals instead, but this new signal will be especially useful where ideally the functionality would be an integral part of a C++ column, but it needs to be implemented in Python. I debated various ways of ensuring that `instanceCreatedSignal()` was emitted at an appropriate time, after the column was constructed and when its reference count was non-zero : 1. Requiring the most-derived class to emit the signal manually. This would require all intermediate classes to have two constructors - an emitting one and a non-emitting one. It would also be error-prone, and require all custom columns in existence to be updated. 2. Adding a `makeIntrusive()` function to IECore, and having that run a custom "post constructor" that would emit the signal. I think we should add `makeIntrusive()` anyway, but it would be non-trivial to use that with `boost::python`, requiring us to eschew `boost::python::init()` and use the more complex `boost::python::make_contructor()` instead. It's also error-prone because people can forget to call `makeIntrusive()`, unless we make all constructors protected to force its use, and grant friendship to it from every class so that it can work. 3. Similar to the above, but using a `PathColumn::create()` function to construct subclasses. Again, this doesn't help with the Python bindings. And it's more boilerplate. 4. The custom `intrusive_ptr_add_ref()` overload. This works without intervention for all existing code, including the Python bindings. It has one weakness as documented - if you first assign to `RefCountedPtr` rather than `ColumnPtr` then we don't emit the signal. But that is extremely unlikely, and will be made even more unlikely if we encourage everyone to use `makeIntrusive()` in the future (in a simpler version without the post constructor). Option 4 requires no changes to existing code and has only a very minor downside, so to me seemed to be the clear winner. --- Changes.md | 4 ++- include/GafferUI/PathColumn.h | 25 ++++++++++++++ python/GafferUITest/PathColumnTest.py | 43 ++++++++++++++++++++++++ src/GafferUI/PathColumn.cpp | 6 ++++ src/GafferUIModule/PathColumnBinding.cpp | 2 ++ 5 files changed, 79 insertions(+), 1 deletion(-) diff --git a/Changes.md b/Changes.md index e72a143ee0d..1dd77077831 100644 --- a/Changes.md +++ b/Changes.md @@ -48,7 +48,9 @@ API - A `DeprecationWarning` is now emitted by `_plugConnections()`. Use `_blockedUpdateFromValues()` instead. - NodeGadget, ConnectionGadget : Added `updateFromContextTracker()` virtual methods. - Path : Added `inspectionContext()` virtual method. -- PathColumn : Added `contextMenuSignal()`, allowing the creation of custom context menus. +- PathColumn : + - Added `contextMenuSignal()`, allowing the creation of custom context menus. + - Added `instanceCreatedSignal()`, providing an opportunity to connect to the signals on _any_ column, no matter how it is created. Breaking Changes ---------------- diff --git a/include/GafferUI/PathColumn.h b/include/GafferUI/PathColumn.h index 4d806967f94..bb660de5549 100644 --- a/include/GafferUI/PathColumn.h +++ b/include/GafferUI/PathColumn.h @@ -166,6 +166,14 @@ class GAFFERUI_API PathColumn : public IECore::RefCounted, public Gaffer::Signal using ContextMenuSignal = Gaffer::Signals::Signal>; ContextMenuSignal &contextMenuSignal(); + /// Creation + /// ======== + + /// Signal emitted whenever a new PathColumn is created. This provides + /// an opportunity for the customisation of columns anywhere, no matter how + /// they are created or where they are hosted. + static PathColumnSignal &instanceCreatedSignal(); + private : PathColumnSignal m_changedSignal; @@ -302,4 +310,21 @@ class MenuDefinition }; +/// Overload for the standard `intrusive_ptr_add_ref` defined in RefCounted.h. +/// This allows us to emit `instanceCreatedSignal()` once the object is fully +/// constructed and it is safe for slots (especially Python slots) to add +/// additional references. +/// +/// > Caution : This won't be called if you assign a new PathColumn to +/// > RefCountedPtr rather than PathColumnPtr. Don't do that! +inline void intrusive_ptr_add_ref( PathColumn *column ) +{ + bool firstRef = column->refCount() == 0; + column->addRef(); + if( firstRef ) + { + PathColumn::instanceCreatedSignal()( column ); + } +} + } // namespace GafferUI diff --git a/python/GafferUITest/PathColumnTest.py b/python/GafferUITest/PathColumnTest.py index 89d78df4f75..9fc2fddb2cd 100644 --- a/python/GafferUITest/PathColumnTest.py +++ b/python/GafferUITest/PathColumnTest.py @@ -40,6 +40,7 @@ import IECore +import GafferTest import GafferUI import GafferUITest @@ -136,5 +137,47 @@ def testIconPathColumnConstructors( self ) : self.assertEqual( c.headerData().value, "label" ) self.assertEqual( c.headerData().toolTip, "help!" ) + def testInstanceCreatedSignal( self ) : + + cs = GafferTest.CapturingSlot( GafferUI.PathColumn.instanceCreatedSignal() ) + + column1 = GafferUI.StandardPathColumn( "l1", "p1" ) + column2 = GafferUI.StandardPathColumn( "l2", "p2" ) + column3 = GafferUI.StandardPathColumn( "l3", "p3" ) + + self.assertEqual( cs, [ ( column1, ), ( column2, ), ( column3, ) ] ) + + def testInstanceCreatedSignalWithPythonColumn( self ) : + + class PythonColumn( GafferUI.PathColumn ) : + + def __init__( self ) : + + self.member = "preInitValue" + GafferUI.PathColumn.__init__( self ) + self.member = "postInitValue" + + columnCreated = None + + def instanceCreated( column ) : + + nonlocal columnCreated + columnCreated = column + + # We can query the full derived type of the column in `instanceCreated()`. + self.assertIsInstance( column, PythonColumn ) + # But we see the object in the state in which is called the base + # class `__init__()`. Column subclasses which need to be seen in a + # fully constructed state should do all their initialisation before + # calling the base class `__init__()`. Hopefully this is not too + # onerous. + self.assertEqual( column.member, "preInitValue" ) + + connection = GafferUI.PathColumn.instanceCreatedSignal().connect( instanceCreated, scoped = True ) + column = PythonColumn() + + self.assertIs( column, columnCreated ) + self.assertEqual( column.member, "postInitValue" ) + if __name__ == "__main__": unittest.main() diff --git a/src/GafferUI/PathColumn.cpp b/src/GafferUI/PathColumn.cpp index fd2a45795df..70d21da61cc 100644 --- a/src/GafferUI/PathColumn.cpp +++ b/src/GafferUI/PathColumn.cpp @@ -115,6 +115,12 @@ PathColumn::ContextMenuSignal &PathColumn::contextMenuSignal() return m_contextMenuSignal; } +PathColumn::PathColumnSignal &PathColumn::instanceCreatedSignal() +{ + static PathColumnSignal g_instanceCreatedSignal; + return g_instanceCreatedSignal; +} + ////////////////////////////////////////////////////////////////////////// // StandardPathColumn ////////////////////////////////////////////////////////////////////////// diff --git a/src/GafferUIModule/PathColumnBinding.cpp b/src/GafferUIModule/PathColumnBinding.cpp index 5d2952641a4..c4ae0d5e99a 100644 --- a/src/GafferUIModule/PathColumnBinding.cpp +++ b/src/GafferUIModule/PathColumnBinding.cpp @@ -504,6 +504,8 @@ void GafferUIModule::bindPathColumn() .def( "buttonReleaseSignal", &PathColumn::buttonReleaseSignal, return_internal_reference<1>() ) .def( "buttonDoubleClickSignal", &PathColumn::buttonDoubleClickSignal, return_internal_reference<1>() ) .def( "contextMenuSignal", &PathColumn::contextMenuSignal, return_internal_reference<1>() ) + .def( "instanceCreatedSignal", &PathColumn::instanceCreatedSignal, return_value_policy() ) + .staticmethod( "instanceCreatedSignal" ) .def( "getSizeMode", (PathColumn::SizeMode (PathColumn::*)() const )&PathColumn::getSizeMode ) .def( "setSizeMode", &PathColumn::setSizeMode, ( arg( "sizeMode" ) ) ) ; From 5ea05f7672cf76a886e47581c5af1c6956593563 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 7 Aug 2024 17:29:40 +0100 Subject: [PATCH 3/3] PathColumnBinding : Allow MenuItems to retain PathListingWidget --- include/GafferUI/PathColumn.h | 7 +++++- src/GafferUIModule/PathColumnBinding.cpp | 28 ++++++++++++++---------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/include/GafferUI/PathColumn.h b/include/GafferUI/PathColumn.h index bb660de5549..ab7989c9325 100644 --- a/include/GafferUI/PathColumn.h +++ b/include/GafferUI/PathColumn.h @@ -164,6 +164,7 @@ class GAFFERUI_API PathColumn : public IECore::RefCounted, public Gaffer::Signal ButtonSignal &buttonDoubleClickSignal(); using ContextMenuSignal = Gaffer::Signals::Signal>; + /// To retain `widget` for use in MenuItem commands, use `PathListingWidgetPtr( &widget )`. ContextMenuSignal &contextMenuSignal(); /// Creation @@ -272,11 +273,13 @@ IE_CORE_DECLAREPTR( FileIconPathColumn ) /// C++ interface for the `GafferUI.PathListingWidget` Python class. Provided for /// use in PathColumn event signals, so that event handling may be implemented /// from C++ if desired. -class PathListingWidget +class PathListingWidget : public IECore::RefCounted { public : + IE_CORE_DECLAREMEMBERPTR( PathListingWidget ) + using Columns = std::vector; virtual void setColumns( const Columns &columns ) = 0; virtual Columns getColumns() const = 0; @@ -287,6 +290,8 @@ class PathListingWidget }; +IE_CORE_DECLAREPTR( PathListingWidget ) + /// C++ interface for the `IECore.MenuDefinition` Python class. Provided for use /// in `PathColumn::contextMenuSignal()`, so that event handling may be /// implemented from C++ if desired. diff --git a/src/GafferUIModule/PathColumnBinding.cpp b/src/GafferUIModule/PathColumnBinding.cpp index c4ae0d5e99a..2db8d7fad7e 100644 --- a/src/GafferUIModule/PathColumnBinding.cpp +++ b/src/GafferUIModule/PathColumnBinding.cpp @@ -71,13 +71,15 @@ class PathListingWidgetAccessor : public GafferUI::PathListingWidget public : PathListingWidgetAccessor( object widget ) - : m_widget( widget ) + : m_widget( + boost::python::handle<>( PyWeakref_NewRef( widget.ptr(), nullptr ) ) + ) { } - object widget() + object widget() const { - return m_widget; + return m_widget(); } void setColumns( const Columns &columns ) override @@ -88,13 +90,13 @@ class PathListingWidgetAccessor : public GafferUI::PathListingWidget { pythonColumns.append( c ); } - m_widget.attr( "setColumns" )( pythonColumns ); + widget().attr( "setColumns" )( pythonColumns ); } Columns getColumns() const override { IECorePython::ScopedGILLock gilLock; - object pythonColumns = m_widget.attr( "getColumns" )(); + object pythonColumns = widget().attr( "getColumns" )(); Columns columns; boost::python::container_utils::extend_container( columns, pythonColumns ); return columns; @@ -119,13 +121,13 @@ class PathListingWidgetAccessor : public GafferUI::PathListingWidget pythonSelection = pythonList; } - m_widget.attr( "setSelection" )( pythonSelection ); + widget().attr( "setSelection" )( pythonSelection ); } Selection getSelection() const override { IECorePython::ScopedGILLock gilLock; - object pythonSelection = m_widget.attr( "getSelection" )(); + object pythonSelection = widget().attr( "getSelection" )(); extract e( pythonSelection ); if( e.check() ) { @@ -141,7 +143,9 @@ class PathListingWidgetAccessor : public GafferUI::PathListingWidget private : - // The Python PathListingWidget object. + // A `weakref` for the Python PathListingWidget object. We use a + // weak reference to avoid `PathListingWidget->Menu->MenuDefinition->PathListingWidget` + // reference cycles when a C++ MenuItem stores a PathListingWidgetPtr. object m_widget; }; @@ -385,9 +389,9 @@ struct ButtonSignalCaller { // C++-based slots are passed a PathListingWidgetAccessor which gives them limited // access to the functionality of the Python PathListingWidget. - PathListingWidgetAccessor accessor( widget ); + PathListingWidget::Ptr accessor = new PathListingWidgetAccessor( widget ); IECorePython::ScopedGILRelease gilRelease; - return s( path, accessor, event ); + return s( path, *accessor, event ); } }; @@ -412,10 +416,10 @@ struct ContextMenuSignalCaller { static void call( PathColumn::ContextMenuSignal &s, PathColumn &column, object pathListingWidget, object menuDefinition ) { - PathListingWidgetAccessor pathListingWidgetAccessor( pathListingWidget ); + PathListingWidget::Ptr pathListingWidgetAccessor = new PathListingWidgetAccessor( pathListingWidget ); MenuDefinitionAccessor menuDefinitionAccessor( menuDefinition ); IECorePython::ScopedGILRelease gilRelease; - s( column, pathListingWidgetAccessor, menuDefinitionAccessor ); + s( column, *pathListingWidgetAccessor, menuDefinitionAccessor ); } };