-
Notifications
You must be signed in to change notification settings - Fork 205
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
CompoundEditor : Pinning UX improvements #3392
CompoundEditor : Pinning UX improvements #3392
Conversation
John mentioned the 'link' symbol does look like a Hamburger Menu, which is a good point. We can certainly find some other symbol for that decoration. Its also worth noting, this isn't instead of any improvements to the slightly confusing jumble that is the right hand side pinning button/swatches at present. I have some ideas forming for that though, just not quite there yet. |
Ruminating on this some in the background. A potentially better solution occurred to me, that would avoid the confusion of the
Does this feel like it makes any sense? |
Not sure people will understand |
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 Tom!
Only showing node names for non-default node sets seems like a very worthwhile improvement to me. It reduces clutter and visual churn for some common use cases, and gives us a better chance of providing the most useful information when we're short of space (the NodeEditor/ScenInspector panel seems to suffer particularly in that regard).
I'm not convinced by the introduction of the additional textual "icons" though. It's not just that the hamburger icon made we assume that it was a menu, since that can be fixed. Let me see if I can articulate my remaining concerns :
- It's more cluttered, and runs counter to the nice removal of clutter in the title itself.
- It's confusing that the icons used in the tab don't match the icons used in the pinning button. It makes it very hard to draw any connection between the two. If we're stuck using the font here, I don't see how we're going to fix that without a lot of effort.
- It's confusing that the icon colour doesn't reflect the driver/driven association as the pinning swatch does.
- It's frustrating that you can't interact with the icon in the same way as the primary pinning icon.
- Most of all though, I think my problem is that this is trying to cure a disease I don't suffer from. When I'm hunting through tabs, I'm generally just looking for "the one I pinned NodeX into", or glancing up to see that I'm looking at what I think I am. The simpler title lets me do that that fine.
So I suggest we tweak the __updateTitle()
naming and merge that first commit in isolation. If there's still interest in reflecting the driver/driven stuff in the title, then I do have one suggestion we might consider though : use the parentheses. Perhaps the driver might be "[ Sphere >"
and the driven might be "> Sphere ]
, or some variation on that theme? And perhaps we could adjust the font colour to match the swatch colours? I'm not actually sure I like this idea, but wanted to throw it out there. It feels like it might be less cluttered, and by not using an additional "icon", might be less confusing that it doesn't have any interactive functionality.
python/GafferUI/NodeSetEditor.py
Outdated
return result | ||
|
||
def __updateTitle( self ) : |
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.
I found this choice of names confusing, in that the function didn't actually update the title. Would __dirtyTitle()
be more a closer description of what is happening here?
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.
Good idea, thanks. Fixed in 7941677
4d1ac09
to
3a2988e
Compare
This helps make a tab's pinning status readable at a glance. As this adds a new title-dirty opportunity, this also requires the 'c++ object already deleted' workaround to avoid exceptions when switching layouts with linked editors. User facing changes: - Tab titles now only show node names when the editor is pinned.
3a2988e
to
7941677
Compare
Thanks for the input @johnhaddon and @andrewkaufman, I've updated the commit to remove the new icon idea, so it just affects the tab title based on pinning state. |
Thanks Tom - LGTM. Will merge when CI is done. |
[update: Just implements #3139 - linked stated pushed for future work]
Implements #3139, with the addition of a decoration to indicate whether an editor is linked or pinned in its title. Due to Qt limitations we can't use icons/graphics and are limited to something we can encode in the tab title, using the same font/color/size.
This now allows at-a-glance observation of pinning state for inactive tabs. It also makes it easier to tell if a tab is pinned/linked when windows are large - as you no longer need to look to the other side of the tab bar.
So the rules, which can be combined, are:
I experimented with a combined pinned/link decoration for linked editors who's driver is pinned, but it was hard to find a compact representation of this that worked well on both platforms.
As most editors also contain a display of which node/location they are showing, it feels loosing this from the tab title for un-pinned nodes doesn't compromising knowing what any specific editor is showing, for visible editors at least.
User facing changes :