Extend Undoable commands to include find and list commands as well

[eugenepeh]

find and list commands are not undo-able.

This is not intuitive to new users, and may cause them to undo a command which they do not wish to undo. For e.g. a user may key in the commands to add a new person, then followed by using find to filtered the list. Upon issuing an undo command, AddressBook undo the add command instead. Also, by extending Undoable commands to include find and list commands, it helps to resolve issue mentioned in #737 as well.

Let's make find and list command undoable to make AddressBook more user-friendly and resolve bugs that may occur due to unexpected command sequences.

Note: please refer to #792 for the recent bug fix due to #737

[damithc]

🤔 Not sure about intuitiveness argument because some users expect only mutating commands to be undoable. However, I don't mind making all commands undoable if the implementation is not overly complicated, ideally, if the implementation is even simpler/cleaner than the current one. But note that if we take this route we commit ourselves to making all future commands undoable.

[Zhiyuan-Amos]

We can't really make all commands undoable, such as HelpCommand :P HistoryCommand is kinda weird to be undoable as well though.

[eugenepeh]

🤔 Not sure about intuitiveness argument because some users expect only mutating commands to be undoable.

True. From my point of view, I think its reasonable for some users to think that find/list is undo-able because it mutates the view of the addressbook. The fact that issue #737 arises sort of back up the fact that making only mutating commands undoable is insufficient.

We can't really make all commands undoable, such as HelpCommand :P HistoryCommand is kinda weird to be undoable as well though.

I think Help and History is fine because it doesn't mutate the view/state of the addressbook.

Another reason, I suggest this change is also because currently undo and redo sets the view to display all persons. This is rather weird in the situation that when a user issue this command sequence

find x delete 1 (oops I deleted the wrong person) undo (this sets the view back to all persons) <- here user would have to enter the command find x again before issuing the correct delete index

Also by making find and list commands undo-able, I think it helps in nested find development, allows the nested find to be undo-able as well.

[eugenepeh]

Another reason, I suggest this change is also because currently undo and redo sets the view to display all persons. This is rather weird in the situation that when a user issue this command sequence find x delete 1 (oops I deleted the wrong person) undo (this sets the view back to all persons) <- here user would have to enter the command find x again before issuing the correct delete index

Oops, something wrong with my above reasoning here... it has nothing to do with making find/list undo-able.

btw @Zhiyuan-Amos , whats the rationale behind resetting the view to show all persons after every undo and redo?

[Zhiyuan-Amos]

resetting the view to show all persons after every undo and redo?

I think it's just cos other commands that update the backing list of PersonListPanel will reset the view to show all persons (i.e. AddCommand, EditCommand, ClearCommand. Except for DeleteCommand). Since undo and redo also update the backing list, as such the view is being reset as well.

[damithc]

True. From my point of view, I think its reasonable for some users to think that find/list is undo-able because it mutates the view of the addressbook. The fact that issue #737 arises sort of back up the fact that making only mutating commands undoable is insufficient.

Makes sense. That means we are expanding the definition of 'mutating' to include UI mutations. We'll still have two types of commands: undoable, and not undoable.