PathColumn : Add contextMenuSignal() and instanceCreatedSignal()
#5985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
johnhaddon
added a commit
to johnhaddon/gaffer
that referenced
this pull request
Aug 2, 2024
This uses an `intrusive_ptr_add_ref` gambit first introduced in GafferHQ#5985, with the rationale for that over various alternatives being documented there. Fixes GafferHQ#915.
johnhaddon
added a commit
to johnhaddon/gaffer
that referenced
this pull request
Aug 2, 2024
This uses an `intrusive_ptr_add_ref` gambit first introduced in GafferHQ#5985, with the rationale for that over various alternatives being documented there. Fixes GafferHQ#915.
|
Summary of offline discussion : we need to change the PathListingWidget wrapper such that we can hold onto it and bake it into lambdas used in the MenuDefinition. Some sort of shared ownership pointer should do the trick... |
This allows columns to contribute to their own context menu, combined with the menu items defined by `PathListingWidget.columnContextMenuSignal()`.
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.
johnhaddon
force-pushed
the
pathColumnContextMenuSignal
branch
from
ad09e06
to
7fdfc41
Compare
I've added this in 7fdfc41, as well as rebasing the earlier commits to avoid the merge conflict. |
johnhaddon
force-pushed
the
pathColumnContextMenuSignal
branch
from
7fdfc41
to
5ea05f7
Compare
|
Updated the last commit to fix the test failures. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This provides further options for PathColumn customisation/extension. I anticipate
instanceCreatedSignal()being particularly useful to extend otherwise pure C++ columns with functionality that is easier to implement in Python.Separately I'm faffing around with some stuff to try to get shortcuts from the menus triggering automatically instead of being implemented separately via
keyPressSignal()as we're doing currently. If that works out, it will come in a separate PR...