Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PathColumn : Add contextMenuSignal() and instanceCreatedSignal() #5985

Merged
merged 3 commits into from
Aug 23, 2024

Commits on Aug 7, 2024

  1. PathColumn : Add contextMenuSignal()

    This allows columns to contribute to their own context menu, combined with the menu items defined by `PathListingWidget.columnContextMenuSignal()`.
    johnhaddon committed Aug 7, 2024
    Configuration menu
    Copy the full SHA
    06c0274 View commit details
    Browse the repository at this point in the history
  2. 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.
    johnhaddon committed Aug 7, 2024
    Configuration menu
    Copy the full SHA
    8ffd4c5 View commit details
    Browse the repository at this point in the history

Commits on Aug 9, 2024

  1. Configuration menu
    Copy the full SHA
    5ea05f7 View commit details
    Browse the repository at this point in the history