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

Inspector, TransformTool : Do not edit EditScopes when target is "None" #6047

Merged

Conversation

murraystevenson
Copy link
Contributor

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.

@murraystevenson murraystevenson self-assigned this Sep 18, 2024
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 Murray, LGTM! I've made a couple of wording suggestions but code all looks good.

// (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.",
Copy link
Member

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.

Copy link
Contributor Author

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.

{
// 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";
Copy link
Member

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.

Copy link
Contributor Author

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.

@murraystevenson murraystevenson merged commit 49a4fd1 into GafferHQ:main Sep 19, 2024
4 of 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.

2 participants