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

Expose color chooser widgets #6003

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

ericmehl
Copy link
Collaborator

This reveals the new widgets added in and #5962, #5944 and #5999 and gives control over their visibility.

There's probably some discussion to have over the API introduced in f0258ed for getting and setting widget visibility in the Color Chooser, and how the state is stored.

When creating a color chooser, there are three places it's looking to see if there are settings for widget visibility :

  1. The Color3f / Color4f plug(s) the widget is associated with
  2. colorChooser:inlineOptions / colorChooser:dialogueOptions userDefault global metadata entry
  3. A per-session colorChosoer:inlineOptions / colorChooser:dialogueOptions sessionDefault global metadata entry

This way you get the state associated with the plug if there is one. If there's no opinion for the plug, we prioritize userDefault on the assumption that someone went to the trouble to set that in a startup script and we should honor that. If that does not exist, we use a per-session variable on the assumption that if a user turned off some elements for one plug's widget, there's a good chance they will want them off for other plugs as well.

And lastly, there are separate options metadata entries for the inline and dialogue options. Mixing them seems like the logic and expectations could become unclear, and my theory is that people tend to use the inline vs dialogue for different reasons and may want different UI elements visible in each.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

@ericmehl
Copy link
Collaborator Author

I made some changes today and squashed them into the original commits since they haven't been reviewed yet. They're cosmetic for the most part - tweaked some icons / indicators, added text entries to the menu for the color field component choices and made some code improvements.

Ready for review.

Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

Thanks Eric!

I didn't manage to spot the code change that removed the spacing between the numeric widgets and the sliders, but I think we should restore the original spacing. Butting up the rounded edge of the numeric field with the flat edge of the slider reads strangely and although it would be fixable by a flat join and a rounded right end of the slider, I think the widgets are distinct enough to deserve their original spacing. It also feels like we could do with a similar spacing on the left of the menu icon? I suspect you're looking for ways of getting a bit more horizontal space now we're squeezing the sliders from both sides, but I think we do need the spacing for clarity still.

My other comments are a mixed bag of API/metadata stuff and UX/bikeshedding stuff. For your sanity, it might be best to attack the former first as I think it's reasonably objective. The rest is undoubtedly subjective, and should be weighed up against other opinions, and - i think - some testing with real users.

Cheers...
John

Changes.md Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooserPlugValueWidget.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooserPlugValueWidget.py Outdated Show resolved Hide resolved
python/GafferUI/ColorSwatchPlugValueWidget.py Outdated Show resolved Hide resolved
@ericmehl ericmehl force-pushed the exposeColorChooserWidgets branch 3 times, most recently from 87bf541 to 4bce89b Compare August 23, 2024 20:16
@ericmehl
Copy link
Collaborator Author

ericmehl commented Aug 23, 2024

I pushed a stack of fixups to address a good portion of the comments this afternoon. I also added a small fix to deactivate the static component menu items if the color field is not visible : 5ddc788

I think the main things were down to is improving the layout for keeping the color field square, and generally getting user feedback on the changes overall and especially the choice mechanism of the static channel.

@ericmehl ericmehl mentioned this pull request Aug 23, 2024
4 tasks
@ericmehl ericmehl changed the base branch from 1.4_maintenance to main September 24, 2024 16:40
@ericmehl
Copy link
Collaborator Author

I updated the commit links to the fixups above and added a custom layout for the color chooser row to address the double-layout processing we were doing.

I also added a few commits to address user feedback :

  • b3082aa : Added "axes" to the menu to better explain what the component combination are affecting.
  • 87e721d and ce2d8fd : Added the ability to turn off RGB sliders (but not A which is always on).
  • 6aa17ea : Added a small space between the RGB, A, HSV and TMI channel groups.
  • 6aa17ea : Changed the interaction for picking which components are in the color field. You now click on a color you want to be in the field, and it will pop up a menu allowing you to choose from the combinations that use that color.
  • 6aa17ea : Added a tooltip to the component label to hint at how to change the components in the color field.
  • 2c92c30 : Changed the wording for saving defaults to emphasize that it's saving the layout, not the color values, as default.

I think that takes care of the code comments and most of the user-feedback for this PR. Would be great to get some interactive testing on the what-components-are-in-the-color-field interaction in particular.

Ready for a new look!

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.

Thanks Eric! Many apologies in advance for continuing the bikeshedding with a few inline UX-related comments after playing with these latest changes...

python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Show resolved Hide resolved
@murraystevenson murraystevenson mentioned this pull request Sep 26, 2024
4 tasks
@ericmehl ericmehl force-pushed the exposeColorChooserWidgets branch 5 times, most recently from 3119cd1 to b06aa05 Compare October 2, 2024 20:34
@ericmehl
Copy link
Collaborator Author

ericmehl commented Oct 2, 2024

I pushed a raft of new commits that address the comments above, which I've noted inline. There are a couple of additional commits too :

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.

Thanks Eric! Feels like we're pretty close to something we can ship in a 1.5 alpha. Few comments inline but nothing too major...

python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
Changes.md Outdated Show resolved Hide resolved
Changes.md Outdated Show resolved Hide resolved
Changes.md Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
.github/workflows/main/installArnold.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooserPlugValueWidget.py Outdated Show resolved Hide resolved
@ericmehl ericmehl force-pushed the exposeColorChooserWidgets branch 3 times, most recently from 15fcc7b to f448a56 Compare October 4, 2024 21:14
@ericmehl
Copy link
Collaborator Author

ericmehl commented Oct 4, 2024

All the latest notes are addressed in my most recent push. Hopefully very close and I can do a big squash!

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.

Thanks Eric! One last comment about the changelog, but feel free to address it while you're squashing everything down.

Changes.md Outdated Show resolved Hide resolved
The previous approach was causing they layout to often be resized
multiple times because we were resizing the color field widget within
the resize event. This creates a custom widget and custom layout to
achieve the same layout constraints without multiple resize events.
Expanding these icons by 2px makes them the same width as the gear icon
which is used on `ColorPlugValueWidget` and makes their left edges
aligned.
@ericmehl
Copy link
Collaborator Author

ericmehl commented Oct 7, 2024

I squashed everything down today and went through and edited the commits to make sure they make sense in isolation and do what they claim to do.

The only change I made was removing a few entries left over in _StyleSheet that aren't needed for the current way of setting the icons indicating which components are in the color field.

Checking against a backup branch from before all the rebasing shows only changes to the _StyleSheet as expected. After a round of interactive testing, I'm feeling good about this being ready to go!

@murraystevenson
Copy link
Contributor

Thanks! Merging!

@murraystevenson murraystevenson merged commit 3a56e6a into GafferHQ:main Oct 8, 2024
5 checks passed
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.

3 participants