Undo Improvements #419
Undo Improvements #419
Conversation
This is in preparation for further work to allow consecutive undoable events to be merged into a single event.
This allows the actions performed by an UndoContext to be merged with the actions performed by the previous UndoContext. This will be used by the UI to merge drag type actions into a single action for the purposes of undo. By default, no merging is performed - it is only performed if the new mergeGroup parameter to the ndoContext matches the parameter from the previous UndoContext. Merging is currently only implemented for ValuePlug::setValue() calls, but I don't see any use cases for the other undoable actions to be mergeable right now. This commit also stops the ScriptNode::unsavedChanges() plug getting set when there is no current UndoContext.
The previous signal type would stop calling slots when one returned True - that's not suitable for a notification that several slots might need.
- Slider.positionChangedSignal() now passes a reason parameter describing why the positions changed. - Slider.changesShouldBeMerged() method returns True if two consecutive changes should be made one in terms of undo. - NumericSlider.valueChangedSignal() now passes a reason parameter describing why the values changed. - ColorChooser.colorChangedSignal() now passes a reason parameter describing why the color changed. - ColorPlugValueWidget and RampPlugValueWidget use the above to perform appropriate undo merging.
The mechanism used by ColorPlugValueWidget and RampPlugValueWidget was much better, so adopting it for NumericPlugValueWidget makes good sense. - Added NumericWidget.changesShouldBeMerged() method. - NumericWidget.valueChangedSignal() now passes a reason parameter describing why the value changed (documenting a previously undocumented commit because it needs to be documented in the Changes file for release).
|
I've updated this to do the right thing for the color and spline editors as well, and improved the mechanism I was using for the numeric entries too. Should be good to go. Note that there is an outstanding issue with the hue slider in that dragging across the full range of hues will break down into a few different undo events. At certain points along the hue line, the plugs affected (always a pair out of r, g and b) change, and at that point the undo merging breaks apart. This problem would also occur with a 2D or 3D manipulator if you moved it purely in one axis for a few changes in the middle of an otherwise diagonal move. I have a rough plan of how to deal with this, but I'd like to think it through a bit further before doing it, and it seems well outside the original scope of this ticket. |
|
Seems to work fine for my little play about (are there actually any nodes with ramps yet?). The only thing I don't like is that I have to de-select a field before I can undo any of it. So I'm shift dragging my color about, decide I don't like the change and want to go back to the original value before my next shift drag, but I have to stop and mouse click somewhere else before undoing. Even tabbing out to another field isn't enough. |
Because the TextWidget has its own little undo queue of text edits, it also handles the undo and redo keyboard shortcuts. This was preventing the genuine graph undo/redo shortcuts from being triggered when a NumericPlugValueWidget, PathPlugValueWidget or StringPlugValueWidget had the focus. The shortcuts are now ignored unless there are text editing undo/redo events available, and the PlugValueWidget clear the text editing undos whenever they transfer changes into the plug value. This allows undo to work correctly for both intermediate (not transferred to the plug) text edits in the TextWidget, and for edits to the plug value.
|
No nodes have ramps yet unless Daniel's made some shaders using them. Here's a little snippet you can run in the ScriptEditor to get one : I've added a further commit which should address your comments about the undo shortcut - the Qt text widgets have their own undo system so they were eating the shortcut. Take a look at the comments and commit log and let me know if what I've done seems a reasonable approach. The alternative is to totally disable any undo of intermediate (not committed to the plug) edits using the keyboard - basically to totally disable the native undo of the text widget. |
|
I like the way it works now. I think if we disabled the intermediate commits totally, we'd have trouble undoing partially typed paths and such. I'm happy to merge as is. By the way, constructing that Gaffer.SplinefColor3fPlug seems to freeze up on my machine (or is supposed to take a really long time?). I'll have some other folks around here test it out too. |
|
David gets the same freeze without the undo branch, just upstream master |
|
Cool - I think it's best this way too so glad it works for you. Without it I was undoing partially typed things and accidentally undoing the creation of the node I was testing on. If the Spline plug creation is taking a long time it's definitely broken in some way - it works instantaneously for me here. Can you paste this in and see if it works? That's the serialised result of the snippet that's not working for you. Has anyone tried the splines at your end yet? Does Daniel have a ramp coshader on the go? |
|
No, nobody has tried them here until now. Daniel was holding off on a ramp coshader until we install the latest master centrally, which I was holding off on since Ben is on leave for a bit. I'll open a separate ticket about the freeze. And yes, that code freezes also. |
This pull request fixes the virtual-slider-dragging-makes-multiple-undo-entries problem discussed in #400 and also adds ScriptNode::undoAddedSignal() to address #103. I went with all the suggestions in the discussion thread, implementing things properly on the API side, just using them from the UI side, and providing a mechanism to prevent merging of undo events and using that to prevent consecutive virtual-slidings from being smooshed into one.
The issues this references should be fully addressed as far as I know, and I'm now moving on to getting the colour picker working properly in a similar vein. That will mean adding a reason argument to the Slider.positionChangedSignal() - is everyone OK with that?
It's probably best to wait to merge this till I have the colour picker commits in there too, but I thought it was worth publishing my changes now the original issues are addressed in case there's any feedback...