Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update UG/DG and undo for training #188

Merged
merged 16 commits into from Nov 6, 2019
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
76 changes: 26 additions & 50 deletions docs/DeveloperGuide.adoc
Expand Up @@ -344,85 +344,61 @@ down the list of people.
** Cons: There may be 2 people with the same name and thus result in an error.

=== Undo / Redo feature
==== Proposed Implementation
The undo command enables users to undo their previous commands while the redo feature enables users to redo their undone commands.

The undo/redo mechanism is facilitated by `VersionedAddressBook`.
It extends `AddressBook` with an undo/redo history, stored internally as an `addressBookStateList` and `currentStatePointer`.
Additionally, it implements the following operations:
==== Undo Implementation

* `VersionedAddressBook#commit()` -- Saves the current address book state in its history.
* `VersionedAddressBook#undo()` -- Restores the previous address book state from its history.
* `VersionedAddressBook#redo()` -- Restores a previously undone address book state from its history.
The undo feature is facilitated by the HistoryManager. The commands stack and the addressBooks stack in the HistoryManager class are used to govern the undo feature.

These operations are exposed in the `Model` interface as `Model#commitAddressBook()`, `Model#undoAddressBook()` and `Model#redoAddressBook()` respectively.
The static commands stack keeps track of all the commands that have been executed by the user while the static addressBooks stack keeps track of the corresponding state of the address book following the executed command.

Given below is an example usage scenario and how the undo/redo mechanism behaves at each step.
Given below is an example usage scenario and how the undo mechanism behaves at each step.

Step 1. The user launches the application for the first time. The `VersionedAddressBook` will be initialized with the initial address book state, and the `currentStatePointer` pointing to that single address book state.
Step 1. The user launches the application for the first time. The HistoryManager will be initialised with the initial address book state pushed to the addressBook stack.

image::UndoRedoState0.png[]
image::initialStack.png[width=450]

Step 2. The user executes `delete 5` command to delete the 5th person in the address book. The `delete` command calls `Model#commitAddressBook()`, causing the modified state of the address book after the `delete 5` command executes to be saved in the `addressBookStateList`, and the `currentStatePointer` is shifted to the newly inserted address book state.
Step 2. The user executes the delete 3 command to delete the 3rd person in the Athletick list. The delete command will be pushed into the commands stack in and the new state of the address book (after executing the delete command) will be pushed to the addressBooks stack as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for addressBook and addressBooks being used? There is some inconsistency here.


image::UndoRedoState1.png[]
image::afterUndoStack.png[width=450]

Step 3. The user executes `add n/David ...` to add a new person. The `add` command also calls `Model#commitAddressBook()`, causing another modified address book state to be saved into the `addressBookStateList`.
Step 3. The user now decides that deleting the 3rd person in the list was a mistake, and decides to undo the action by executing the undo command. The undo command calls the undo method in the ModelManager which pops the latest Command from the commands stack and
the latest Address Book from the addressBooks stack. This removes the latest action caused by the command that wants to be undone. It then continues to peek at the addressBooks stack to retrieve the Address Book state before the undone Command was executed.

image::UndoRedoState2.png[]

[NOTE]
If a command fails its execution, it will not call `Model#commitAddressBook()`, so the address book state will not be saved into the `addressBookStateList`.

Step 4. The user now decides that adding the person was a mistake, and decides to undo that action by executing the `undo` command. The `undo` command will call `Model#undoAddressBook()`, which will shift the `currentStatePointer` once to the left, pointing it to the previous address book state, and restores the address book to that state.

image::UndoRedoState3.png[]

[NOTE]
If the `currentStatePointer` is at index 0, pointing to the initial address book state, then there are no previous address book states to restore. The `undo` command uses `Model#canUndoAddressBook()` to check if this is the case. If so, it will return an error to the user rather than attempting to perform the undo.
image::initialStack.png[width=450]

The following sequence diagram shows how the undo operation works:

image::UndoSequenceDiagram.png[]
image::UndoSQ.png[width=450]
Figure 1. Sequence diagram for undo feature

NOTE: The lifeline for `UndoCommand` should end at the destroy marker (X) but due to a limitation of PlantUML, the lifeline reaches the end of diagram.
==== Redo Implementation

The `redo` command does the opposite -- it calls `Model#redoAddressBook()`, which shifts the `currentStatePointer` once to the right, pointing to the previously undone state, and restores the address book to that state.
The redo feature is also facilitated by the HistoryManager. The undoneCommands stack and the undoneAddressBooks stack in the HistoryManager class are used to govern the redo feature.

[NOTE]
If the `currentStatePointer` is at index `addressBookStateList.size() - 1`, pointing to the latest address book state, then there are no undone address book states to restore. The `redo` command uses `Model#canRedoAddressBook()` to check if this is the case. If so, it will return an error to the user rather than attempting to perform the redo.

Step 5. The user then decides to execute the command `list`. Commands that do not modify the address book, such as `list`, will usually not call `Model#commitAddressBook()`, `Model#undoAddressBook()` or `Model#redoAddressBook()`. Thus, the `addressBookStateList` remains unchanged.

image::UndoRedoState4.png[]
The undoneCommands stack keeps track of commands that have been undone by the user while the undoneAddressBooks stack keeps track of the corresponding state of the address book following the undone command.

Step 6. The user executes `clear`, which calls `Model#commitAddressBook()`. Since the `currentStatePointer` is not pointing at the end of the `addressBookStateList`, all address book states after the `currentStatePointer` will be purged. We designed it this way because it no longer makes sense to redo the `add n/David ...` command. This is the behavior that most modern desktop applications follow.
How the redo feature works is very similar to how to undo feature work. The difference is that redo utilises the undoneCommands stack and the undoneAddressBook stack. As such, you can also refer to the diagrams in the Undo Implementation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor edit to

How the redo feature works is very similar to how the undo feature works.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good eye!


image::UndoRedoState5.png[]

The following activity diagram summarizes what happens when a user executes a new command:

image::CommitActivityDiagram.png[]

==== Design Considerations
This section describes the pros and cons of the current and other alternative implementations of the undo and redo features.

===== Aspect: How undo & redo executes

* **Alternative 1 (current choice):** Saves the entire address book.
** Pros: Easy to implement.
* **Alternative 1 (current choice):** Saves a deep copy of the entire address book after each command into a stack.
** Pros: Easy to implement, and easy for developers to understand.
** Cons: May have performance issues in terms of memory usage.
* **Alternative 2:** Individual command knows how to undo/redo by itself.
** Pros: Will use less memory (e.g. for `delete`, just save the person being deleted).
** Cons: We must ensure that the implementation of each individual command are correct.

===== Aspect: Data structure to support the undo/redo commands
====== Reason why we chose alternative 1:
Even though the memory usage of Alternative 2 is lesser, we do not feel that the benefit of the usage of lesser memory outweigh the cost of implementing the alternative.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor edit to

outweighs

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay thank you!


Furthermore, as we realise that each time the application starts, the memory of the states of the address book in both alternatives are cleared.
This means that the cost of having alternative 1 is significantly cheaper. As such, we decided to go with the first alternative.

* **Alternative 1 (current choice):** Use a list to store the history of address book states.
** Pros: Easy for new Computer Science student undergraduates to understand, who are likely to be the new incoming developers of our project.
** Cons: Logic is duplicated twice. For example, when a new command is executed, we must remember to update both `HistoryManager` and `VersionedAddressBook`.
* **Alternative 2:** Use `HistoryManager` for undo/redo
** Pros: We do not need to maintain a separate list, and just reuse what is already in the codebase.
** Cons: Requires dealing with commands that have already been undone: We must remember to skip these commands. Violates Single Responsibility Principle and Separation of Concerns as `HistoryManager` now needs to do two different things.
// end::undoredo[]

=== Adding/editing photo feature
==== Implementation
Expand Down
29 changes: 18 additions & 11 deletions docs/UserGuide.adoc
Expand Up @@ -97,7 +97,7 @@ attendance and performance.

Type the athlete's particulars in the format given below.

Format : `add n/NAME p/PHONE e/EMAIL a/ADDRESS [t/TAG]... [i/IMAGE]`
Format : `add n/NAME p/PHONE e/EMAIL g/GENDER a/ADDRESS [t/TAG]... [i/IMAGE]`

[NOTE]
====
Expand All @@ -108,7 +108,7 @@ You can include any number of tags (zero tags are also allowed) to an athlete an
is optional.
====

Example: `add n/John Doe p/98765432 e/johnd@example.com a/311, Clementi Ave 2, #02-25 t/backstroke i/john.png`
Example: `add n/John Doe p/98765432 e/johnd@example.com g/male a/311, Clementi Ave 2, #02-25 t/backstroke i/john.png`

image::beforeAdd.png[width=450]

Expand Down Expand Up @@ -636,26 +636,28 @@ This command restores Athletick to the state before the previous command was exe
[NOTE]
====
Take note that the `undo` feature only applies to undoable commands.
Undoable commands include: `add`, `delete`, `edit`, `clear`, `attendance` and `training`.
Undoable commands include: `add`, `delete`, `edit`, `clear` and `training`.

The `undo` command will not be able to undo commands that cannot be undone.
Let’s say you have executed a `list` command to list out all the athletes information in Athletick.
If you were to execute the `undo` command now, the `undo` command will fail because `list` is not an undoable command,
and that no doable commands were executed before this.

The `undo` command reverses previous commands in reverse chronological order.
Let’s say you have executed the `edit` command, followed by the `delete` command.
Executing `undo` now will first reverse the `delete` command.
Executing `undo` again will reverse the `edit` command.
Let’s say you have executed the `edit` command, followed by the `delete` command. If you were
to execute `undo` command now, you will reverse the `delete` command. Executing `undo` again
will then reverse the `edit` command.

The `undo` command will reverse the latest command that can be undone.
Let’s say you have executed the `delete` command, followed by the `list` command.
Since `list` command is not an undoable command, executing `undo` now will thus reverse the `delete` command.
Since `list` command is not an undoable command, if you were to execute `undo`
command now, you will thus reverse the `delete` command.
====

Let’s say you have accidentally deleted an athlete’s contact (Mohamad Ali) from your list.
Instead of having to re-enter Mohamad Ali’s contact information all over again,
you can easily restore all of Mohamad Ali’s details by `undo`-ing the `delete` command that you have just entered.
you can easily restore all of Mohamad Ali’s details by executing the command `undo` to
undo the `delete` command that you have just entered.

*What you should do*

Expand All @@ -665,6 +667,7 @@ Format : `undo`

image::undo.png[width=450]


*What you should see*

The result box will display the message “Undo Command Success” and you can check that Mohamad Ali’s
Expand All @@ -686,13 +689,17 @@ The `redo` command reverses previous `undo` commands in reverse chronological or
Let’s say that you have executed the `clear` command, followed by the `add` command.
Executing the `undo` command now will reverse the `add` command.
Executing the `undo` command again will reverse the `clear` command as well.
Following this, executing the `redo` command will reverse the last `undo` command and re-execute the `clear` command.
Executing the `redo` command again will reverse the second-last `undo` command and re-execute the `add` command.
Following this, executing the `redo` command will reverse the most recent `undo` command and re-execute the `clear`
command.
Executing the `redo` command again will reverse the second most recent `undo` command and re-execute the `add` command.

The `redo` command can only be executed immediately after a `undo` command.
====

Let’s say you have executed the `delete` command to delete Mohamad Ali from your list.
You may undo this action and restore Mohamad Ali’s information by executing the `undo` command. (See Undoing a previous command.)


Then, if you decide that you want the contact to remain deleted after all,
you may very quickly do so by executing the `redo` command to reverse the `undo` command that you had just executed.

Expand All @@ -707,7 +714,7 @@ image::redo.png[width=450]

The result box will display the message “Redo Command Success” and Mohamad Ali is once again gone from the list!

image::afterUndo.png[width=450]
image::afterRedo.png[width=450]

=== Upcoming features

Expand Down
Binary file added docs/images/afterUndoStack.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added docs/images/initialStack.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added docs/images/undoSQ.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/AddCommand.java
Expand Up @@ -74,4 +74,8 @@ public boolean equals(Object other) {
|| (other instanceof AddCommand // instanceof handles nulls
&& toAdd.equals(((AddCommand) other).toAdd));
}
@Override
public String toString() {
return "Add Person Command";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you included the Person details here (eg. maybe even just the Name), could it be possible to include more details in the UndoCommand success other than just Add Person Command undone?

Same for the other commands that have specific details (eg. Delete and Edit), so the user is more clear on what in particular has been undone.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give it a shot

}
}
Expand Up @@ -74,4 +74,8 @@ public CommandResult execute(Model model) throws CommandException {
public boolean isUndoable() {
return false;
}
@Override
public String toString() {
return "Attendance Command";
}
}
Expand Up @@ -58,4 +58,8 @@ public boolean equals(Object other) {
public boolean isUndoable() {
return false;
}
@Override
public String toString() {
return "Calendar Command";
}
}
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/ClearCommand.java
Expand Up @@ -25,4 +25,8 @@ public CommandResult execute(Model model) {
public boolean isUndoable() {
return true;
}
@Override
public String toString() {
return "Clear Command";
}
}
Expand Up @@ -25,5 +25,8 @@ public abstract class DeleteCommand extends Command {
public boolean isUndoable() {
return true;
}

@Override
public String toString() {
return "Delete Command";
}
}
Expand Up @@ -49,4 +49,8 @@ public boolean equals(Object other) {
|| (other instanceof DeletePersonCommand // instanceof handles nulls
&& targetIndex.equals(((DeletePersonCommand) other).targetIndex)); // state check
}
@Override
public String toString() {
return "Delete Person Command";
}
}
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/EditCommand.java
Expand Up @@ -254,4 +254,8 @@ && getAddress().equals(e.getAddress())
&& getTags().equals(e.getTags());
}
}
@Override
public String toString() {
return "Edit Command";
}
}
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/EventCommand.java
Expand Up @@ -52,4 +52,8 @@ public boolean equals(Object other) {
|| (other instanceof EventCommand // instanceof handles nulls
&& toAdd.equals(((EventCommand) other).toAdd));
}
@Override
public String toString() {
return "Add Event Command";
}
}
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/ExitCommand.java
Expand Up @@ -19,4 +19,8 @@ public CommandResult execute(Model model) {
public boolean isUndoable() {
return false;
}
@Override
public String toString() {
return "Exit Command";
}
}
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/FilterCommand.java
Expand Up @@ -63,4 +63,8 @@ public boolean equals(Object other) {
|| (other instanceof FilterCommand // instanceof handles nulls
&& predicate.equals(((FilterCommand) other).predicate)); // state check
}
@Override
public String toString() {
return "Filter Command";
}
}
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/FindCommand.java
Expand Up @@ -42,4 +42,8 @@ public boolean equals(Object other) {
|| (other instanceof FindCommand // instanceof handles nulls
&& predicate.equals(((FindCommand) other).predicate)); // state check
}
@Override
public String toString() {
return "Find Command";
}
}
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/HelpCommand.java
Expand Up @@ -22,4 +22,8 @@ public CommandResult execute(Model model) {
public boolean isUndoable() {
return false;
}
@Override
public String toString() {
return "Help Command";
}
}
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/ListCommand.java
Expand Up @@ -23,4 +23,8 @@ public CommandResult execute(Model model) {
public boolean isUndoable() {
return false;
}
@Override
public String toString() {
return "List Command";
}
}
Expand Up @@ -84,4 +84,8 @@ private Record createRecord() {
public boolean isUndoable() {
return false;
}
@Override
public String toString() {
return "Add Performance Command";
}
}
25 changes: 20 additions & 5 deletions src/main/java/seedu/address/logic/commands/RedoCommand.java
Expand Up @@ -9,20 +9,35 @@
*/
public class RedoCommand extends Command {
public static final String COMMAND_WORD = "redo";
public static final String MESSAGE_SUCCESS = "Redo Command Success";
public static final String MESSAGE_FAILURE = "Redo Command Failure: You have not "
+ "undone any commands. As such, you are unable to redo any commands.";
public static final String MESSAGE_SUCCESS = "Redo ";
public static final String MESSAGE_FAILURE_EMPTY_STACK = "Redo Command Failure: No available "
+ "commands to be redone.";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changing to No available commands to redo.

public static final String MESSAGE_FAILURE = "Redo Command Failure:"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good improvement in the error message. just a point regarding grammar: should be "an" instead of "a"

+ " Redo command can only be executed after a undo Command.";
@Override
public CommandResult execute(Model model) throws CommandException {
HistoryManager history = new HistoryManager();
if (history.isRedoneEmpty()) {
return new CommandResult(MESSAGE_FAILURE_EMPTY_STACK);
} else if (!(history.getLatestCommand() instanceof UndoCommand)) {
return new CommandResult(MESSAGE_FAILURE);
} else {
Command redoneCommand = model.redo();
if (redoneCommand instanceof TrainingCommand) {
return new CommandResult(MESSAGE_SUCCESS + redoneCommand
+ " Success!", ((TrainingCommand) redoneCommand).getDate(), model);
} else {
return new CommandResult(MESSAGE_SUCCESS + redoneCommand

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider showing the user what command has been redone for more clarity.

+ " Success!");
}
}
model.redo();
return new CommandResult(MESSAGE_SUCCESS);
}
@Override
public boolean isUndoable() {
return false;
}
@Override
public String toString() {
return "Redo Command";
}
}
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/SelectCommand.java
Expand Up @@ -57,4 +57,8 @@ public boolean equals(Object other) {
|| (other instanceof SelectCommand // instanceof handles nulls
&& targetIndex.equals(((SelectCommand) other).targetIndex)); // state check
}
@Override
public String toString() {
return "Select Command";
}
}
4 changes: 4 additions & 0 deletions src/main/java/seedu/address/logic/commands/SortCommand.java
Expand Up @@ -24,4 +24,8 @@ public CommandResult execute(Model model) {
public boolean isUndoable() {
return false;
}
@Override
public String toString() {
return "Sort Command";
}
}
Expand Up @@ -69,7 +69,10 @@ public List<Index> getIndexList() {
public boolean isUndoable() {
return true;
}

@Override
public String toString() {
return "Add Training Command";
}
/**
* Checks with the model if person indexes are valid.
*/
Expand Down