StyleSheet : Reserve space for `valueChanged` icon #5737

ericmehl · 2024-03-18T20:02:22Z

This fixes the valueChanged icon overlap with Transform tool orientation plugs. It would initially overlap the left side of the text when changing the orientation from Parent. If you then changed to another tool and back to the first, it would be fixed when Qt laid you the widgets again. By always reserving the space, the icon always appears correctly.

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.

johnhaddon · 2024-03-19T11:54:00Z

Thanks Eric. Although this does fix things as advertised, I'm seeing a few unwanted side-effects in other areas of the UI...

Cropping of the labels on the various Query node outputs :

Less bad, but we now have an indent on the labels in the spline editor, which breaks the top-to-bottom flow :

I found these just by searching for places we used LabelPlugValueWidget - you might want to check for others.

For the Query nodes I think there's a pretty simple fix - these are output plugs so they will never ever have the green dot applied to them (see _hasUserValue()). So we can just not ever have the extra padding for those. For the spline editor I'm less sure - maybe we try putting the labels to the left of the widgets? Or just accept the indent?

I also think we could probably be a bit more frugal with the space anyway. The icon seems to contain an integral 3 pixel gap on the left, but then I think with the padding we're making a bigger (6 pixel?) gap on the right. Might be worth dropping that to 3, or maybe even dropping them both to 2?

CI seems to have gone bananas, and I can't figure out why. The PR is for 1.3_maintenance but there are CI runs using the new build platform that only applies to 1.4. Hopefully that also accounts for why they're failing in an utterly bizarre way too. Could you maybe retarget this to main to see what happens? I think that might be the better place for it anyway, since it is changing layouts subtly in a way that could break things.

ericmehl · 2024-03-25T22:22:55Z

I touched up the icon and its padding so it's a little more condensed now. I went with 2px on each side, though the icon doesn't strictly align to the pixel grid so it's more like 1.5px on each side. Not sure if that alignment is something that needs changing.

I also put in a scheme for identifying labels for output plugs and removing the indentation for those. It seems to be helping the query plugs.

Last is the color spline - I tried putting the label and value on the same line. I think it looks better, but not 100% convinced. It shrinks the available space for the color sliders a little. I haven't figured out how to get the alignment the same for the "Position" and "Value" labels - _qtWidget().setMinimumWidth() and _qtWidget.setFixedWidth() don't seem to work, but I'll keep trying on that.

Fingers crossed for CI. I initially had it targeting main by accident and switched it to 1.3_maintenance, so maybe the CI didn't get rebooted properly.

It's now rebased on main, and targeted to main.

Not quite ready for a full review, I want to look at a wider variety of plugs and also sort out the alignment of the spline color values first.

ericmehl · 2024-03-26T21:15:23Z

I think the spline plugs are looking pretty good now, I sorted most of the spacing issue. Here's the color spline with the latest commit :

and the float spline is the same as before :

Ideally the green dot would align to the left, but I'm not sure how to achieve that without resorting to the style sheet, which feels like introducing too much complexity for too little benefit. But I'll take any tips!

I also surveyed the plug types I could think that might have formatting tweaks or glitches and all are looking good.

Ready for a new look!

johnhaddon

Thanks Eric. This is looking good aesthetics-wise I think. I've made a few inline suggestions for potentially tidying up the implementation a little, and for getting the dots aligned in the RampPlugValueWidget...

johnhaddon · 2024-03-27T12:27:43Z

python/GafferUI/LabelPlugValueWidget.py

+		if any( p.direction() == Gaffer.Plug.Direction.In for p in self.getPlugs() ) :
+			self.__label._qtWidget().setProperty( "gafferValueChanged", GafferUI._Variant.toVariant( False ) )


Can we do this in __setValueChanged() instead of here, so that everything is all in one place?

I have a potential change for this in 0aedb34. It's nice to get that all in one place, but it does require a setStyleSheet() call in order to recognized the padding-left change.

It doesn't seem to have caused jumpy layouts or other issues, but I don't know if there are knock-on effects from setStyleSheet().

johnhaddon · 2024-03-27T12:43:07Z

python/GafferUI/RampPlugValueWidget.py

+			self.__positionLabel = GafferUI.LabelPlugValueWidget( plug.pointXPlug( 0 ) )
+			self.__positionField = GafferUI.NumericPlugValueWidget( plug.pointXPlug( 0 ) )


Because we're inside a with column block here, these widgets are going to first be parented to that column before being pulled out and parented elsewhere by the addChild() calls below. I find the structured (with) style of UI building much more readable than the manual addChild() method, because you can see the structure of the UI in the structure of the code. So I'd suggest that the lesser evil would be to use with when constructing the containers below, even if it means two lines that create the position widgets.

It's definitely possible to give a fixed width to the labels - the trick is to do what PlugWidget does, and set the width on the label inside the widget : self.__positionLabel.label()._qtWidget().setFixedWidth( 400 ). I think it would be worth doing this to get the green dots to align.

Oh nice, thanks for that tip on label()._qtWidget(), it's helpful. I added that in 5cdd19a

I find the structured (with) style of UI building much more readable than the manual addChild() method

I agree, and definitely shouldn't be added and then removed from the parent container. I'll make that change, pending what we ultimately go with for the container choice discussed below.

johnhaddon · 2024-03-27T12:47:32Z

python/GafferUI/RampPlugValueWidget.py

+			if isinstance( plug.pointYPlug( 0 ), Gaffer.FloatPlug ) :
+				row = GafferUI.ListContainer( orientation = GafferUI.ListContainer.Orientation.Horizontal, spacing = 4 )
+
+				row.addChild( self.__positionLabel, verticalAlignment = GafferUI.VerticalAlignment.Top )
+				row.addChild( self.__positionField, verticalAlignment = GafferUI.VerticalAlignment.Top )
+
+				row.addChild( self.__valueLabel, verticalAlignment = GafferUI.VerticalAlignment.Top )
+				row.addChild( self.__valueField, verticalAlignment = GafferUI.VerticalAlignment.Top )
+			else :
+				grid = GafferUI.GridContainer( spacing = 4 )


Maybe you could use a single with GridContainer() for both cases, and then the only thing that would differ for the float and color modes would be the indices of the widgets within the grid?

That was my initial approach, but I wasn't super happy with the layout for float splines. Here's what that looks like :

It seems odd to me that it doesn't fill the width, but maybe that's just me being used to the old style. Maybe there's a way to expand the widgets, setSizePolicy() sounds like a good place to try.

ericmehl · 2024-03-28T17:49:47Z

Ok, now that I know how to set the width of the label, the spline plug layouts got a lot easier. The latest push has everything squashed down and is ready for a new review.

johnhaddon · 2024-04-02T08:56:26Z

python/GafferUI/LabelPlugValueWidget.py

 		if valueChanged == self.__getValueChanged() :
 			return

 		self.__label._qtWidget().setProperty( "gafferValueChanged", GafferUI._Variant.toVariant( valueChanged ) )
 		self.__label._repolish()
+		self.__label._setStyleSheet()


This will be the only place in Gaffer where we've needed this on anything other than a top-level widget - all other widgets just inherit from their parents. It doesn't seem to be necessary here (testing on Linux), and in fact actually seems to cause clipping of the labels in the ColorSpline UI. What problems were you seeing without it?

Without _setStyleSheet() I don't get the space reserved for the green dot, so it overlaps with the label.

I found some references online implying that padding would not be taken into account in Qt's polish() method. It is a little vague, but if the Qt documentation's mention of not changing geometry includes padding, then that sounds like intended behavior.

I don't seem to have any clipping of labels (on Windows) :

Resolved in b25b038 and d3aaa2f where I added metadata to control whether or not the icon is shown, and hiding it for spline plugs.

johnhaddon · 2024-04-02T08:58:24Z

python/GafferUI/RampPlugValueWidget.py

-				if isinstance( plug.pointYPlug( 0 ), Gaffer.FloatPlug ):
-					self.__valueField = GafferUI.NumericPlugValueWidget(
-						plug.pointYPlug( 0 ),
+				with GafferUI.ListContainer( orientation = GafferUI.ListContainer.Orientation.Horizontal, spacing = 4 ) :


I am finding the new layout for ColorSplines a little bit less satisfactory than the old one. I'm wondering if it would make sense to just have a way of turning off the green dots for certain widgets or plugs instead? The idea of a non-default value doesn't make much sense for spline positions/values, at least once you've started adding your own points.

Paging @murraystevenson for a second opinion.

Just noting that we talked about this in our weekly meeting and decided to hide the icon for spline position and value plugs. Done in d3aaa2f

ericmehl · 2024-04-02T22:04:28Z

I think I have a good implementation of what we talked about today. For input plugs, LabelPlugValueWidget now recognizes metadata entry called showValueChangedIndicator on the plug which it is labelling. If False, away goes the green dot as requested.

If it's an output plug, we never set the value gafferShowValueChangeIndicator in the first place, so they continue to never get a green dot, or padding that messes up the label.

And lastly, the RampPlugValueWidget uses this new power to hide the dot on the position and value plugs.

Ready for a new look.

johnhaddon

Sorry Eric, this seems to be the PR that refuses to die! I've made two new comments inline, which I think do need addressing. In the interests of getting a release out today, I've opened #5764 with fixups for those, and a couple of other optional commits for your consideration...I'll leave it to you and @murraystevenson which bits you take...

johnhaddon · 2024-04-03T11:56:57Z

python/GafferUI/LabelPlugValueWidget.py

@@ -72,6 +74,15 @@ def __init__( self, plug, horizontalAlignment=GafferUI.Label.HorizontalAlignment
 		self.__label._qtWidget().setFixedHeight( 20 )
 		layout.addWidget( self.__label._qtWidget() )

+		showIndicator = False
+		if all( p.direction() == Gaffer.Plug.Direction.In for p in self.getPlugs() ) :
+			showIndicator = Gaffer.Metadata.value( self.getPlug(), "showValueChangedIndicator" )


self.getPlug() will throw if the widget is created for more than a single plug. I'm not sure if we're currently using the multiple-plugs facility with LabelPlugValueWidget, but elsewhere in this file we are being careful to make sure it works.

johnhaddon · 2024-04-03T11:57:53Z

python/GafferUI/RampPlugValueWidget.py

@@ -83,6 +83,7 @@ def __init__( self, plug, **kw ) :
 				spacing = 4
 			) :

+				Gaffer.Metadata.registerValue( plug.pointXPlug( 0 ), "showValueChangedIndicator", False )


This is making metadata that is serialised with the file unnecessarily. We should be able to create a static registration instead.

ericmehl · 2024-04-03T18:46:32Z

Thanks for the fixes in #5764, @johnhaddon. I tried to squash down and push to your branch to merge, but I don't seem to have the right permissions. I pushed back to this branch with your fixes, and dropped the speculative commit where you used a NameLabel instead of LabelPlugValueWidget.

I also rebased onto #5765 to pick up that Windows build fix.

@murraystevenson, this is ready for a final check and merge assuming all is well.

murraystevenson · 2024-04-03T19:34:40Z

Thanks! Merging.

ericmehl changed the base branch from main to 1.3_maintenance March 18, 2024 20:55

ericmehl changed the base branch from 1.3_maintenance to main March 25, 2024 15:27

ericmehl force-pushed the valueChangedFix branch 2 times, most recently from 91438b3 to c222cb5 Compare March 26, 2024 20:05

johnhaddon mentioned this pull request Mar 27, 2024

Selection Modification #5720

Merged

4 tasks

johnhaddon reviewed Mar 27, 2024

View reviewed changes

ericmehl force-pushed the valueChangedFix branch 3 times, most recently from aa3eeab to 9bee911 Compare March 28, 2024 17:48

johnhaddon reviewed Apr 2, 2024

View reviewed changes

ericmehl force-pushed the valueChangedFix branch from 9bee911 to d3aaa2f Compare April 2, 2024 22:00

johnhaddon mentioned this pull request Apr 3, 2024

Fixes and alternates for #5737 #5764

Closed

johnhaddon reviewed Apr 3, 2024

View reviewed changes

ericmehl force-pushed the valueChangedFix branch from d3aaa2f to 87632d3 Compare April 3, 2024 18:40

ericmehl and others added 4 commits April 3, 2024 14:45

StyleSheet : Reserve space for valueChanged icon

08edd34

LabelPlugValueWidget : showValueChangedIndicator

87b58f8

SplinePlugValueWidget : Hide value changed icon

8a0181e

LabelPlugValueWidget : Add prefix for widget-specific metadata

e36cc50

ericmehl force-pushed the valueChangedFix branch from 87632d3 to e36cc50 Compare April 3, 2024 18:45

murraystevenson merged commit 993c8c7 into GafferHQ:main Apr 3, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StyleSheet : Reserve space for `valueChanged` icon #5737

StyleSheet : Reserve space for `valueChanged` icon #5737

ericmehl commented Mar 18, 2024

johnhaddon commented Mar 19, 2024

ericmehl commented Mar 25, 2024

ericmehl commented Mar 26, 2024

johnhaddon left a comment

johnhaddon Mar 27, 2024

ericmehl Mar 28, 2024 •

edited

Loading

johnhaddon Mar 27, 2024

johnhaddon Mar 27, 2024

ericmehl Mar 27, 2024

ericmehl Mar 27, 2024

johnhaddon Mar 27, 2024

ericmehl Mar 27, 2024

ericmehl commented Mar 28, 2024

johnhaddon Apr 2, 2024

ericmehl Apr 2, 2024

ericmehl Apr 2, 2024

johnhaddon Apr 2, 2024

ericmehl Apr 2, 2024

ericmehl commented Apr 2, 2024

johnhaddon left a comment

johnhaddon Apr 3, 2024

johnhaddon Apr 3, 2024

ericmehl commented Apr 3, 2024

murraystevenson commented Apr 3, 2024

		if any( p.direction() == Gaffer.Plug.Direction.In for p in self.getPlugs() ) :
		self.__label._qtWidget().setProperty( "gafferValueChanged", GafferUI._Variant.toVariant( False ) )

		self.__positionLabel = GafferUI.LabelPlugValueWidget( plug.pointXPlug( 0 ) )
		self.__positionField = GafferUI.NumericPlugValueWidget( plug.pointXPlug( 0 ) )

StyleSheet : Reserve space for valueChanged icon #5737

StyleSheet : Reserve space for valueChanged icon #5737

Conversation

ericmehl commented Mar 18, 2024

Checklist

johnhaddon commented Mar 19, 2024

ericmehl commented Mar 25, 2024

ericmehl commented Mar 26, 2024

johnhaddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmehl Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmehl commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmehl commented Apr 2, 2024

johnhaddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmehl commented Apr 3, 2024

murraystevenson commented Apr 3, 2024

StyleSheet : Reserve space for `valueChanged` icon #5737

StyleSheet : Reserve space for `valueChanged` icon #5737

ericmehl Mar 28, 2024 •

edited

Loading