Undo for scrubbed values should only be set at scrub-start and scrub-end.

[bentoogood]

Currently every time a value is modified during a virtual slider scrub that change is placed in the undo queue.

Users would like to be able to scrub a value up and down, then let go, then press ctrl-z to return to the state before they started scrubbing.

[andrewkaufman]

Same is true for arrow up/down I believe. So if you hold up to go from 1.01 to 1.09 you'll have 8 undos to get back where you were.

[davidsminor]

NB: currently, fixing this would desynchronize the maya/gaffer undo queues in "graffer", so we'd need to fix issue #103 as well

[johnhaddon]

I can see a couple of ways of going about this, and wouldn't mind some feedback on them before getting started.

One way is to make it entirely the responsibility of the UI components not to make too many undoable events. This would be achieved by having them only set the value in an undoable once they think the user is done with what they're doing. So all the scrubbing and up/down actions would set the value in a non-undoable way, and then when focus is lost or the scrubbing ends, a single undoable setValue() call would be made. This is complicated slightly by the need to remember the original value, set back to that (non-undoably) and then make the final call. It also means that potentially multiple UI components will need to implement the same stuff, and that it will be impossible to avoid multiple undos when the user uses two controls (up/down in a numeric input followed by dragging a slider for instance). Doing things this way wouldn't require a fix for #103 as I believe our current hack would work still.

The other way would be to implement concatenation of the Action instances held by the ScriptNode. Each undoable event is represented as an Action with doAction() and undoAction() methods, and these are stored in an undo queue on the ScriptNode. We could add an Action::concatenate( const Action &otherAction ) method that would allow the new action to be concatenated with the previous one in the queue, leaving just one action in the undo queue. Concatentation would be limited so only setValue() events on the same plug were concatenated, and probably use a timestamp to prevent events which are too distant in time from being concatenated. This wouldn't place any burden on the UI elements at all, and would allow concatentation of events from multiple UI sources. I don't know how this would interact with #103 - it basically becomes impossible to say when an action has finished, because at any time after another one might be concatenated with it.

I had been attempting to keep the Action class simple, and avoid having to subclass it for each of the undoable actions, but various other tickets (#20 for instance) are beginning to suggest we might need to make it slightly more complex anyway. So I'm leaning towards the second option as being the more "proper" way of going about things, as it removes burden from the UI code and moves things in a direction where we've made a start on some other tickets. My worry is doing it in a way that can help with #103 rather than make it even trickier.

Thoughts?

[davidsminor]

Hello,

Well, from my perspective, I don't really need to know when an action has finished. All I need to be able to do for #103 is to execute some code exactly once for every time this happens:

m_script->m_undoIterator = m_script->m_undoList.end();

I think that could work with the action::concatenate() approach couldn't it? You'd set the value on a plug for the first time, it'd bung an extra action on the undo queue, call my little "undo queue has incremented" signal, and then just carry on adding to the most recent action (which I wouldn't really need to care about).

So yeah, for the action::concatenate thing gets my vote. You'd probably want to minimize the amount of fiddlng a third party has to do to write their own UI component anyway wouldn't you. Unless there's an argument for them being able to control that behaviour

David

[andrewkaufman]

If David is happy with his signal, I'm happy with less fiddly UI code and smarter behind the scenes action handling, so concatenate would get my vote too.

What impact does this have on the CompoundAction idea we had discussed? Does this replace the need for that entirely? If not, is there still room to go that direction if we start down this road?

[danieldresser]

I don't have much of a stake in this, so don't pay too much attention to me, but one comment:

Concatentation would be limited so only setValue() events on the same plug were concatenated, and probably use a timestamp to prevent events which are too distant in time from being concatenated.

Even if we're using concatenation, would it make sense to have the UI elements put some sort of flag on the action when a slider drag finishes, so that the next action won't be concatenated? I'm just wondering whether it would be better to keep the result more deterministic, rather than relying on a timeout. I guess the question is what we want when you combine hotkeys and sliders. Regardless, it sounds like the action concatenation approach would allow you to experiment with different options for some of these details.

[johnhaddon]

Just making a few implementation notes for myself - no need to reply (unless you have the solution). Scrubbing a numeric field is a very simple example where concatentation is easily implemented. But what if we're scrubbing the value slider in a ColorPlugValueWidget? That will generate setValue calls for r, then g, then b. So the sequence of calls will be interleaved and break concatenation. Same goes when dragging nodes around the node graph - there'll be an x, y pair for each node in sequence. We need to consider concatenation not just of single actions, but of the whole series of actions in each undoable event. Perhaps this will be made easier by a CompoundAction? And perhaps we only do the concatenation into the undo stack at the end of the UndoContext(), having already performed all the actions singly?

[johnhaddon]

And as far as Daniel's question goes, I think yes, and we could perhaps deal with it by passing a concatenate yes/no value into the UndoContext constructor.

[johnhaddon]

David, if I add an undoAddedSignal() then would it be OK if I removed the actionSignal()?

[johnhaddon]

I need to modify the NumericWidget so that the valueChangedSignal will be passed an additional argument describing why the value changed (setValue call, up/down cursor nudging, virtual sliding, or straight text entry), so that the NumericPlugValueWidget can make sure that undo events from different editing methods aren't merged. Is that OK with everyone? Could you check with Paulo that he doesn't need backwards compatibility there?

[davidsminor]

'ello. Yeah, I did a quick grep through my code, and it looks like I'm only using the actionSignal for that undo queue stuff, so it'd be ok for me if you removed it

[andrewkaufman]

Paulo said he's not using that signal right now, so go ahead and change it