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

Editor, NodeToolbar, PlugLayout, PlugValueWidget : Remove setContext() #5952

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

johnhaddon
Copy link
Member

We have never used the flexibility this method was intended to provide, but it has been the source of unnecessary complexity and synchronisation bugs. Removing them now smooths the way for some "context aware" behaviours we're in the process of adding, whereby we want all the UI components to share the same context tracking utility, which necessitates sharing the same context.

We have never used the flexibility this method was intended to provide, but it has been the source of unnecessary complexity and synchronisation bugs.

Also rename `getContext()` to `context()`, and add compatibility shim to forward legacy `getContext()` calls to it.
When accessing the context for Editor, NodeToolbar, PlugLayout and PlugValueWidget. This means we are not relying on the compatibility shim, which is now solely for external code.

Note : Here I'm treating GafferCortexUI as "external" and hence haven't touched it - I'm hoping that it is now time for Image Engine to pick up the maintenance for this module.
This is an example of a failure to synchronise changes from `setContext()` in a higher-level component (the Viewer) to a lower-level component (the toolbar). The context we were passing was literally used for nothing, and would never have been updated if we called `Viewer.setContext()`. Better that this is all gone now.
Now `Editor.setContext()` has been removed, we can just acquire the Playback object in the constructor.
@johnhaddon johnhaddon self-assigned this Jul 10, 2024
Copy link
Contributor

@murraystevenson murraystevenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'll probably keep reaching for getContext() for a minute or two until I fully internalise the change, but more simplification more better!

@johnhaddon johnhaddon merged commit a0074d3 into GafferHQ:main Jul 15, 2024
5 checks passed
@johnhaddon johnhaddon deleted the removeContextSetters branch August 7, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants