Skip to content

Commit

Permalink
PathColumn : Add instanceCreatedSignal()
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
johnhaddon committed Aug 7, 2024
1 parent 06c0274 commit 8ffd4c5
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 1 deletion.
4 changes: 3 additions & 1 deletion Changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
----------------
Expand Down
25 changes: 25 additions & 0 deletions include/GafferUI/PathColumn.h
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,14 @@ class GAFFERUI_API PathColumn : public IECore::RefCounted, public Gaffer::Signal
using ContextMenuSignal = Gaffer::Signals::Signal<void ( PathColumn &column, PathListingWidget &widget, MenuDefinition &menuDefinition ), Gaffer::Signals::CatchingCombiner<void>>;
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;
Expand Down Expand Up @@ -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
43 changes: 43 additions & 0 deletions python/GafferUITest/PathColumnTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@

import IECore

import GafferTest
import GafferUI
import GafferUITest

Expand Down Expand Up @@ -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()
6 changes: 6 additions & 0 deletions src/GafferUI/PathColumn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,12 @@ PathColumn::ContextMenuSignal &PathColumn::contextMenuSignal()
return m_contextMenuSignal;
}

PathColumn::PathColumnSignal &PathColumn::instanceCreatedSignal()
{
static PathColumnSignal g_instanceCreatedSignal;
return g_instanceCreatedSignal;
}

//////////////////////////////////////////////////////////////////////////
// StandardPathColumn
//////////////////////////////////////////////////////////////////////////
Expand Down
2 changes: 2 additions & 0 deletions src/GafferUIModule/PathColumnBinding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<reference_existing_object>() )
.staticmethod( "instanceCreatedSignal" )
.def( "getSizeMode", (PathColumn::SizeMode (PathColumn::*)() const )&PathColumn::getSizeMode )
.def( "setSizeMode", &PathColumn::setSizeMode, ( arg( "sizeMode" ) ) )
;
Expand Down

0 comments on commit 8ffd4c5

Please sign in to comment.