Inspector : Add support for disabling edits #6028
Conversation
There was a problem hiding this comment.
Thanks Murray!
API-wise this is feeling pretty good to me, but I've noted a couple of issues in the way we're surfacing it to the user. One I think is just a bug that I haven't been able to locate in the code, and the other is just a request for more pop-ups explaining the reasons behind D not working. Hopefully those make sense...
Cheers...
John
| /// Returns `true` if `disableEdit()` will disable the edit | ||
| /// at `source()`, and `false` otherwise. | ||
| bool canDisableEdit() const; | ||
| /// Disables the edit at `source()`. Throws if | ||
| /// `!canDisableEdit()` | ||
| void disableEdit() const; |
There was a problem hiding this comment.
We seem to be lacking the equivalent of nonEditableReason() here - the only way to find out why you can't disable an edit is to call disableEdit() and catch the exception.
There was a problem hiding this comment.
Ah yes, that would be useful and is necessary for the overall messaging improvements. Added in 6e14602.
After writing it, nonDisablableReason() still feels a bit clumsy to me, but fits with nonEditableReason()...
There was a problem hiding this comment.
Since it's an imaginary word, I suppose we can spell it how we want, but should it be nonDisableableReason() (extra e)? No matter how we spell it, I suspect I will always read it as "nonBlaBlaReason" :(
My only other thought was to pass a string *reasonNot to canDisableEdit(), but that gets awkward with the Python bindings. I think we can live with the awkward name...
There was a problem hiding this comment.
Changed to nonDisableableReason() and squashed.
|
Thanks for the input! I've addressed the comments inline and taken an overall run at improving the user messaging when disabling isn't possible. Might now be erring on the side of too much info, but we should now be covering the common cases... |
src/GafferSceneUI/Inspector.cpp
Outdated
| // the property. | ||
| result->m_editFunction = "No editable source found in history."; | ||
| } | ||
| result->m_disableEditFunction = "No disablable source found in history."; |
There was a problem hiding this comment.
Can we avoid "disablablabla" in the UI? Maybe "No editable source found in history" is OK for this too?
There was a problem hiding this comment.
Switched to "No editable source found in history" and squashed it in.
src/GafferSceneUI/Inspector.cpp
Outdated
| else if( node->ancestor<EditScope>() && node->ancestor<EditScope>() != result->m_editScope ) | ||
| { | ||
| result->m_disableEditFunction = fmt::format( | ||
| "Edit authored in EditScope ({}), set it as the target EditScope to disable.", |
There was a problem hiding this comment.
This isn't unique to this PR, but I find messages of this form are often hard to read because my EditScope is often called EditScope, leading to a sentence full of EditScope. Maybe that's just because I'm throwing scenes together for testing, and everyone else has better names? But I wonder if we should refer to "the scope in which you have chosen to edit" as the "edit scope" rather than the "EditScope", to help with this?
I also wonder if there could be clarity to be gained by relegating the changeable details to a second sentence, so the first sentence is always the same, and always very blunt.
Edit is not in the current edit scope. Change scope to <NameOfEditScope> to disable.
There was a problem hiding this comment.
Switched over to the suggested Edit is not in the current edit scope. Change scope to <NameOfEditScope> to disable.
Also took the opportunity to conform the Inspector's existing "EditScope (EditScope)" messages to the same style in c4ddc45.
src/GafferSceneUI/Inspector.cpp
Outdated
| } | ||
| else if( !node->ancestor<EditScope>() && result->m_editScope ) | ||
| { | ||
| result->m_disableEditFunction = "Edit was not authored in any EditScope, set target EditScope to (None) to disable."; |
There was a problem hiding this comment.
If we went with my suggestion above, I think this could follow the same form :
Edit is not in the current edit scope. Change scope to None to disable.
There was a problem hiding this comment.
Switched over to the suggested Edit is not in the current edit scope. Change scope to None to disable.
Tidy these to better match `include/GafferSceneUI/Private/Inspector.h`
This allows acquiring existing edits without the worry of creating a new edit in situations where one doesn't exist.
Derived inspectors can provide their own custom disable behaviour by implementing `disableEditFunction()`.
Otherwise the current tooltip persists when moving the cursor from a menu item with a tooltip to one without a tooltip.
As EditScope nodes are often named "EditScope#", we now refer to an "edit scope" to prevent sentences full of "EditScope". Preferring "The target edit scope EditScope..." to "The target EditScope (EditScope)...".
murraystevenson
force-pushed
the
inspectorDisableEdit
branch
from
d09d2a6
to
c4ddc45
Compare
This moves the potentially Inspector-specific "Disable Edit" functionality from InspectorColumn to the Inspectors themselves via the new
Inspector::Result::disableEdit(). Inspectors with custom disabling logic, such as the SetMembershipInspector, can provide their own by implementingdisableEditFunction().On the user-facing side, this PR allows toggling of edits disabled via the "Disable Edit" actions in the Light Editor and Render Pass Editor. The approach taken is to only allow edits disabled in the current session to be re-enabled to allow for investigative "how would my scene be affected by disabling this edit?" toggling, while attempting to avoid enabling edits the user may not expect to exist, such as previously unmodified Spreadsheet cells in EditScope processors.