User should be allowed to Undo and Redo certain commands

[Zhiyuan-Amos]

in the event where the user may have accidentally entered an undesirable command, in which user is able to reverse the command easily.

I'm not sure whether this is the right place to discuss the implementation of Undo and Redo.
Reference: Command Pattern (refer to the section titled Undo and Redo)

Creating an abstract class ReversibleCommand which extends Command. ReversibleCommand contains an abstract method undo and a default method redo (which basically calls execute). Only commands that changes the model will inherit from ReversibleCommand i.e Add, Edit, Delete, Clear.

From the reference above, we can implement a historyArray in LogicManager, which upon every command execution, pushes the command into the historyArray. Each command will now have to store slightly more information:

1. Upon executing AddCommand, AddCommand also has to store the index of person added on the list. When executing undo, execute DeleteCommand at the index.

2. Upon executing DeleteCommand, store the details of the person being deleted. When executing undo, execute the equivalent AddCommand. However, over here we have to support adding at a particular index i.e we cannot solely execute AddCommand (new MoveCommand?). It's not necessary to have to execute AddCommand as well, since I think we can just manipulate the model to add the person at a particular index.

3. Upon executing ClearCommand, store the entire model. When executing undo, restore the entire model.

4. Upon executing EditCommand, store the original information of the person being edited. When executing undo, execute EditCommand using the original information.

Then, we have an index to point to the historyArray so we know which command to undo / redo next. An alternative implementation is to use undoStack and redoStack.

[Zhiyuan-Amos]

@damithc @pyokagan @yamgent need your opinions concerning this, thanks!

[damithc]

@Zhiyuan-Amos do some research on various approaches to implementing an undo command and share them here.

Some relevant points:

• Can/should we allow selective undo? i.e. pick a specific command from the history to 'reverse'. Probably not. Most apps don't do that. But it's worth a thought.
• Some commands don't mutate data but mutate the display. e.g. list/sort. Should we allow undoing of those commands?
• Perhaps a history command that displays a list of past commands can be an intermediate step toward an undo command. i.e. it will require storing of past commands which is also needed for undo.

Adding @MightyCupcakes @PierceAndy as they were interested in the undo command IIRC.

[Zhiyuan-Amos]

@damithc

1. General implementation of undo command

• The first way is mentioned above. This method seems to be the proper way of implementing undo command (following Command pattern).
• Another way (a simpler way) would be to store the entire state of the AddressBook after every execution, thus every undo / redo would be simply restoring the app to the previous / next states. It shouldn't affect performance that much since we don't usually execute that many commands in this app and the number of Persons in the AddressBook is usually quite little. However, the drawback is that this method doesn't allow selective undo.

2. Supporting selective undo
   Plus points:

• Gives user the flexibility to undo a specific command

  Negative points:

• Can be quite hard to implement. Consider the case: Add 10 persons -> Clear -> Undo the adding of the 5th person (i.e delete the 5th person). However, the 5th person has already been deleted in the Clear Command. What should the expected behaviour be?
  Another case: Adding a person -> Edit the person -> Undo the adding of the person (i.e delete the person). That way, the AddCommand is unable to find out who to actually delete, since the details have changed.
  I'm not sure giving the user the flexibility is worth the extra effort to consider all possible corner cases XD Maybe in AB Level 5?

3. Supporting undoing of display mutation. I think being able to undo Sort is good, however being able to undo list / select / find is unnecessary imo because it seems rather counterintuitive (e.g MS Word, you can't undo a Ctrl-F). A better way to "undo" a select, find etc would be to allow the user to press the up button, which will show the user the previous command he has entered.

4. History command sounds good. In the future, the combination of History command + being able to Undo multiple commands in a go (e.g Undo 5 undoes the last 5 commands) would be a good user story i.e allows user to know how many commands the user has to undo to return to the desired state.
   Though I am not sure if we should implement it now since it will delay the implementation of the undo / redo command. The History command appears to require some work in the UI as well: The ResultDisplay box seems to be too small so we have to make the box automatically expand (I'm not sure whether it's currently implemented that way already). Perhaps this can be done in parallel i.e first implement the historyArray / historyStack, then concurrently implement the History command and Undo / Redo command.

[yamgent]

The first way is mentioned above. This method seems to be the proper way of implementing undo command (following Command pattern).

I feel that an interface could be used instead of an abstract class, as it serves as a "contract" to say that this command is capable of being "undo"ed.

Also redo after undo would be re-executing the same command again, so don't think we need a redo()?

Another way (a simpler way) would be to store the entire state of the AddressBook after every execution, thus every undo / redo would be simply restoring the app to the previous / next states. It shouldn't affect performance that much since we don't usually execute that many commands in this app and the number of Persons in the AddressBook is usually quite little. However, the drawback is that this method doesn't allow selective undo.

This can take up quite a big portion of the memory if the AddressBook is of a certain size (e.g. 100 over entries...)

But you are right in that there will be quite a few corner cases that we need to consider for selective undo, and since we want to keep level 4 simple yet functional, we can probably do selective undo in level 5 instead.

[Zhiyuan-Amos]

I feel that an interface could be used instead of an abstract class, as it serves as a "contract" to say that this command is capable of being "undo"ed.

Also possible. Rationale for using an abstract class is because ReversibleCommand is a Command, and AddCommand etc are ReversibleCommands.

Also redo after undo would be re-executing the same command again, so don't think we need a redo()?

Yup you are right; each command doesn't need to implement redo.

[damithc]

@Zhiyuan-Amos as discussed, you can spend some time to do more research and experimentation and recommend an approach. There is no need to implement extra things like 'selective undo' or 'undo GUI mutations'. I mentioned them just so that we can consider them in our decision, all other things being equal. There is no need to complicate our design just to accommodate such 'unlikely' features.

[MightyCupcakes]

Can be quite hard to implement. Consider the case: Add 10 persons -> Clear -> Undo the adding of the 5th person (i.e delete the 5th person). However, the 5th person has already been deleted in the Clear Command. What should the expected behaviour be?
Another case: Adding a person -> Edit the person -> Undo the adding of the person (i.e delete the person). That way, the AddCommand is unable to find out who to actually delete, since the details have changed.
I'm not sure giving the user the flexibility is worth the extra effort to consider all possible corner cases XD Maybe in AB Level 5?

All commands that mutate the state of the AddressBook should be reversible. However, the clear command is quite a special case, it can be defined to be non-reversible

On the other hand, a find operation does not affect the state of the AddressBook at all.

History command sounds good. In the future, the combination of History command + being able to Undo multiple commands in a go (e.g Undo 5 undoes the last 5 commands) would be a good user story i.e allows user to know how many commands the user has to undo to return to the desired state.
Though I am not sure if we should implement it now since it will delay the implementation of the undo / redo command. The History command appears to require some work in the UI as well: The ResultDisplay box seems to be too small so we have to make the box automatically expand (I'm not sure whether it's currently implemented that way already). Perhaps this can be done in parallel i.e first implement the historyArray / historyStack, then concurrently implement the History command and Undo / Redo command.

I have thought about this but it is actually quite easy to implement. All successfully executed commands is placed in a stack held by the Model and you can implement a soft limit on the size of the stack or whatever you want to do with it. The hard part is when the user decides to undo something and you pop off the stack, you get a Command class and you have to decide exactly what is the reverse of this command.

[Zhiyuan-Amos]

@MightyCupcakes

All successfully executed commands is placed in a stack held by the Model

I'm not sure whether we should place it in the Model. I'm thinking of creating a new class called HistoryManager (and interface History), and LogicManager will have a History. Rationale being:

1. The job of Model is to store the state of the AddressBook, but historyStack / list of Commands that were executed previously doesn't seem to belong to the state of AddressBook.
2. Model has no need to understand the concept of Command i.e Command is a specific component of Logic.
3. Other components with access to Model can access the historyStack as well (unintended side effect). Whereas if we put it in LogicManager, then historyStack can be private, thus only LogicManager can access it.

The hard part is when the user decides to undo something and you pop off the stack

This can be solved by storing additional values in the 'Undoable' commands (for e.g in DeleteCommand, store the details of the Person being deleted). Then inside DeleteCommand#undo, call AddCommand with these details.

[yamgent]

However, the clear command is quite a special case, it can be defined to be non-reversible

Special because it takes up a huge chunk of memory? Otherwise it is possible to reverse the deletion of the entire address book...