-
Notifications
You must be signed in to change notification settings - Fork 214
SceneInspector Revamp : Small UI Improvements #6531
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
SceneInspector Revamp : Small UI Improvements #6531
Conversation
Rather than returning the full path, we instead just return the name. This makes it simpler to drag attribute names and the like.
Otherwise expansion is lost when switching to/from diff mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Murray - LGTM, apart from the weird off-by-one error in CI.
|
||
self.waitForIdle( 1000 ) | ||
|
||
# Our two Stretch columns should equally share the available space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a test failure here on CI :
Error: FAIL: testStretchColumnWidthsWhenUpdatingColumns (GafferUITest.PathListingWidgetTest.PathListingWidgetTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/__w/gaffer/gaffer/build/python/GafferUITest/PathListingWidgetTest.py", line 1654, in testStretchColumnWidthsWhenUpdatingColumns
self.assertEqual(
AssertionError: 482 != 483
I'm seeing the same thing on my local machine, except with 456 != 457
. I presume it worked on your machine, in which case I wonder if it is related to whatever window size the window manager has come up with. I also wonder if this relates to the todo I added previously in testColumnWidthsWhenRemovingLastColumn
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, it appears to be an intermittent failure. I thought it might be a Qt5/Qt6 thing given only linux-gcc11-platform23 failed on CI, but I can repro it here with both sets of dependencies by repeating the test a few times.
Will do a little bit more digging, but if it looks too thorny I'll put another todo with a delta in the comparison for now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's related to the window size and the width of the non-stretch column at index 0. In my case the width of the non-stretch column could change between test runs*, while the window size stayed consistent. This resulted in the available space changing from an even number to an odd number between runs, and the runs with an odd amount of available space failed as _TreeView.__resizeStretchColumns()
currently gives each stretch column equal integer widths and leaves behind the remaining pixel. I've pushed an update adding a delta and a todo for now.
*increasing the value of the self.waitForIdle()
calls would stabilise the width of the 0th column between test runs, but at a width that would always result in an odd amount of available space on my workstation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Thanks for investigating. We can come back for that single pixel when we haven't got bigger fish to fry. LGTM.
Usually when we resize `Stretch` columns we preserve the ratio of column widths as the user may have chosen to adjust them before resizing the PathListingWidget. Though when we change the columns in an existing PathListingWidget, those existing widths are less relevant as the number of, order of, and columns themselves could be entirely different so we now ignore any existing header widths and instead give each stretch column an equal share of the available space. This is a little heavy-handed but prevents situations such as where calling `setColumns()` to switch from one stretch column to two would result in the first new stretch column being given the majority of the available space.
81e3254
to
3acbb40
Compare
A few bits and pieces addressing some of the remaining UI feedback from #6515.