Keyboard undo redo

[WNewhook]

Added undo and redo keyboard shortcuts. As a note, editing a label disables keyboard shortcuts so if you undo or redo into a point where you were doing that you can't use the keyboard to continue undoing - you need to use the UI. Not sure what to do about that. Thoughts?

[christopher-rodgers]

Yeah the keyboard shortcuts being cut while editing labels seems a bit clunky. Ideally when editing a label, you would still be able to undo and redo using the keyboard, but the other shortcuts would be locked out. It seems that's how it should work, not sure if there are logistical issues with that approach though.

[WNewhook]

The thing about that is the text entry boxes already have defined behaviour for ctrl+z and ctrl+y - it undoes/redoes any typing you've done in the box. I'm not certain if I'd be able to combine that behaviour with the editor's undo/redo... Maybe I can make it so keyboard undo/redo will skip any selections that have an open label, so instead of reopening the label and disabling the keyboard shortcuts it will just change the label back to what is was before. That way the text boxes' undo/redo will still work but they won't interfere with the editor's keyboard undo/redo. Think that would work?

[christopher-rodgers]

Yeah that sounds promising, give it a shot.

[WNewhook]

@christopher-rodgers @jillhancock @inexpensive Think I got it working. Take a look when you get a chance.

[theodore-norvell]

From the users point of view, adding a string node or editing an existing string node is one edit action and should correspond to one undo operation. E.g. if I do the following (0) (a) Add a string node (1) (b) Edit the string (2) (c) Press enter (3) (d) Click undo or press cntl-z

Then the state should be back at state (0).

What is happening is we make two checkpoints. (0) <-- checkpoint (a) Add a string node (1) <-- checkpoint (b) Edit the string (2) (c) Press enter (3) <-- checkpoint (d) Click undo or press cntl-z

I think the correct solution is to avoid making the checkpoint at state (1).

What I would suggest is rewriting "update" like this

export function update( sel : Selection, checkpoint : boolean = true ) : void {
            if( checkpoint ) {
                undostack.push(currentSelection);
                redostack.length = 0 ; }
            currentSelection = sel ;
            generateHTMLSoon();
    }

Then don't make a checkpoint on the changes that create open nodes. It might be a little hard to hunt these down since it sometimes only known to the tree manager whether an open node was created.

An alternative is to inspect the whole darn tree at the start of update to see if it has any open nodes. If so don't checkpoint.

[I'm getting less and less happy with this open node business. It seems a hack. It is a case modifying the model for reasons that only have to do with the view and the controller. I put it in for a good reason, but perhaps it wasn't the best idea.]

[theodore-norvell]

On further thought, what you have is probably ok. But there is some redundant code that could be factored into a subroutine

    function hasOpenNodes( sel : Selection ) : boolean { ... }

[theodore-norvell]

What is the reasoning behind the if(finished) condition?

It seems to me that if the while loop ends because the stack is empty then the currentSelection still need to be updated. If you don't want to update currentSelection, then the stacks should not be updated. I.e. if you change the stacks you need to change currentSelection (and vis-versa).

To put it another way any version of undo or redo function should preserve the value of the following sequence

        undoStack.concat( [currentSelection ] ).concat( reverseValueOf( redoStack ) )

[WNewhook]

I put that there thinking that it isn't possible for the loop to exit without finished being true, but thinking about it more that isn't really the case. And by that same logic it doesn't need the conditional anyway. I'll remove it.

[theodore-norvell]

Looks good now.