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

OptionTweaks / Spreadsheet / Render Pass Editor : Make use of metadata registered for options #5871

Merged
merged 5 commits into from
May 29, 2024

Conversation

murraystevenson
Copy link
Contributor

This adds the ability for OptionTweaks to make use of globally registered metadata on tweak value plugs, allowing tweaks to present a specific plugValueWidget, presets, etc based on metadata registered for the option being tweaked. In addition we also allow metadata to be automatically forwarded to the default row of spreadsheet columns.

The motivation here is to present a consistent UI for edits made with editors such as the Render Pass Editor, where the same widgets and presets should be presented at all levels, from the editor itself, through to spreadsheets within the Edit Scope processors and finally the OptionTweak nodes making the edits.

@murraystevenson murraystevenson self-assigned this May 28, 2024
@johnhaddon
Copy link
Member

Thanks Murray, this makes the RenderPassEditor feel much more useable. I've made a few comments inline but they're mostly trivial, or likely where I've misunderstood something...

@johnhaddon
Copy link
Member

Oh, there's one small downside to this that I forgot to mention : Ctrl+Return doesn't close the pop up editor for set expression in the RenderPassEditor, whereas before Return was doing. I think we can probably fix this in PlugPopup.__firstTextWidget() by just checking for something with a textWidget() method rather than checking against specific PlugValueWidget types?

startup/GafferSceneUI/standardOptions.py Show resolved Hide resolved
startup/GafferSceneUI/renderPassOptions.py Outdated Show resolved Hide resolved
python/GafferUITest/SpreadsheetUITest.py Show resolved Hide resolved
python/GafferUI/SpreadsheetUI/_Metadata.py Outdated Show resolved Hide resolved
python/GafferUI/SpreadsheetUI/_Metadata.py Show resolved Hide resolved
python/GafferUI/SpreadsheetUI/_Metadata.py Outdated Show resolved Hide resolved
python/GafferUI/SpreadsheetUI/_Metadata.py Show resolved Hide resolved
@murraystevenson
Copy link
Contributor Author

Oh, there's one small downside to this that I forgot to mention : Ctrl+Return doesn't close the pop up editor for set expression in the RenderPassEditor, whereas before Return was doing. I think we can probably fix this in PlugPopup.__firstTextWidget() by just checking for something with a textWidget() method rather than checking against specific PlugValueWidget types?

Improving the textWidget discovery in PlugPopup.__firstTextWidget() is definitely worth doing and I've made the suggested change in dcf661f, but it doesn't solve Ctrl+Return to close. Return to close the popup ends up being handled handled for the other widgets here:

def __keyPress( self, widget, event ) :
if event.key == "Return" :
self.close()
but the Return keypress is swallowed by MultiLineTextWidget:
if event.key=="Enter" or ( event.key=="Return" and event.modifiers==event.Modifiers.Control ) :
self.__activatedSignal( self )
self._emitEditingFinished()
return True

As an alternate take, I've added a commit to close PlugPopup based on the activatedSignal of the currently focused child widget in 0d14a72, see if that feels better to you, or if it triggers a better idea for implementation...

Look for `textWidget()` attr rather than specific classes, so we can find `GafferSceneUI.SetExpressionPlugValueWidget`. Connect to `activatedSignal()` so we can close automatically when <kbd>Ctrl</kbd>+<kbd>Enter</kbd> is pressed in MultiLineTextWidgets.
@johnhaddon johnhaddon merged commit 8653221 into GafferHQ:1.4_maintenance May 29, 2024
5 checks passed
@johnhaddon
Copy link
Member

Thanks for the updates - I've squashed them in and merged.

I've added a commit to close PlugPopup based on the activatedSignal of the currently focused child widget

Doh! I'd been assuming that we were connecting to activatedSignal to implement the closing already! LGTM, after going down the "surely we can just connect in __init__ and not __focusChanged" road and discovering that life wasn't that simple. I just changed one minor thing - passing scoped = True so that we don't keep accumulating connections if focus is lost and regained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants