-
Notifications
You must be signed in to change notification settings - Fork 206
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
Inspector, TransformTool : Do not edit EditScopes when target is "None" #6047
Inspector, TransformTool : Do not edit EditScopes when target is "None" #6047
Conversation
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! I've made a couple of wording suggestions but code all looks good.
src/GafferSceneUI/Inspector.cpp
Outdated
// (they have selected "None" from the Menu) and the upstream edit is | ||
// inside _any_ EditScope. | ||
result->m_editFunction = fmt::format( | ||
"Edit is in an edit scope. Change scope to {} to edit.", |
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.
Suggestion :
Source is in an EditScope. Change scope to {} to edit.
I prefer Source because to me Edit implies a tweak to an already-existing value, but it's possible for the source to be the originator of the value (even when in an EditScope). It also reduces the repetition of edit, which I think helps readability a bit.
And I think in this case we're talking about an EditScope - the concrete node type. My thinking is that "edit scope" is for cases where we're referring to the current scope - either an EditScope or None.
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! I've switched over to the suggestion. I never really liked the repetition of "edit" so this does read more clearly to my eye.
src/GafferSceneUI/TransformTool.cpp
Outdated
{ | ||
// We don't allow editing if the user hasn't requested a specific scope | ||
// and the upstream edit is inside an EditScope. | ||
m_warning = "Transform edit is in an edit scope. Change scope to " + displayName( upstreamEditScope ) + " to edit"; |
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.
If my suggestion for wording on Inspector seems reasonabel, then I suggest we do the same 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.
As above, switched over to the suggestion.
0e0e1aa
to
cc9cca1
Compare
This PR adjusts the meaning of the "None" menu item in the EditScope UI. Previously, choosing "None" would edit the first upstream plug, wherever it was found. This could result in edits being made inside of any edit scope if it already contained a plug that could receive that edit. This resulted in users potentially inadvertently scattering their edits across multiple edit scopes while the target edit scope was set to "None".
With this change, "None" still allows editing the first upstream plug as long as it is not inside an edit scope, but now prevents editing if the first upstream plug is inside an edit scope and directs the user to change their scope, which we feel is clearer behaviour than the previous scatter-shot approach.