Skip to content
This repository has been archived by the owner on Mar 5, 2023. It is now read-only.

Commit

Permalink
UndoableCommand: Update redo() restore previous view before executing
Browse files Browse the repository at this point in the history
The method `undo()` sets the `Addressbook` view to show all
persons after restoring the `Addressbook` state.

This leads to `redo()` potentially executing its command
on the incorrect `Person` due to different indexing in
different views.

Let's update
* `UndoableCommand` to store the predicate for the current view
* `redo()` method to reset the view to its previous state
before executing its command again.
  • Loading branch information
yamidark committed Jan 14, 2018
1 parent 7bfe7e0 commit fe40136
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 0 deletions.
14 changes: 14 additions & 0 deletions src/main/java/seedu/address/logic/commands/UndoableCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,20 @@
import static seedu.address.commons.util.CollectionUtil.requireAllNonNull;
import static seedu.address.model.Model.PREDICATE_SHOW_ALL_PERSONS;

import java.util.Optional;
import java.util.function.Predicate;

import seedu.address.logic.commands.exceptions.CommandException;
import seedu.address.model.AddressBook;
import seedu.address.model.ReadOnlyAddressBook;
import seedu.address.model.person.ReadOnlyPerson;

/**
* Represents a command which can be undone and redone.
*/
public abstract class UndoableCommand extends Command {
private ReadOnlyAddressBook previousAddressBook;
private Optional<Predicate<ReadOnlyPerson>> previousPredicate = Optional.empty();

protected abstract CommandResult executeUndoableCommand() throws CommandException;

Expand All @@ -24,6 +29,13 @@ private void saveAddressBookSnapshot() {
this.previousAddressBook = new AddressBook(model.getAddressBook());
}

/**
* Stores the current state of {@code model#filteredPersonList#Predicate}
*/
private void savePredicateSnapshot() {
previousPredicate = Optional.ofNullable(model.getFilteredPersonListPredicate());
}

/**
* Reverts the AddressBook to the state before this command
* was executed and updates the filtered person list to
Expand All @@ -42,6 +54,7 @@ protected final void undo() {
protected final void redo() {
requireNonNull(model);
try {
previousPredicate.ifPresent(model::updateFilteredPersonList);
executeUndoableCommand();
} catch (CommandException ce) {
throw new AssertionError("The command has been successfully executed previously; "
Expand All @@ -53,6 +66,7 @@ protected final void redo() {
@Override
public final CommandResult execute() throws CommandException {
saveAddressBookSnapshot();
savePredicateSnapshot();
return executeUndoableCommand();
}
}
12 changes: 12 additions & 0 deletions src/test/java/systemtests/DeleteCommandSystemTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,23 @@ public void delete() {
/* ------------------ Performing delete operation while a filtered list is being shown ---------------------- */

/* Case: filtered person list, delete index within bounds of address book and person list -> deleted */
Model modelBeforeDeletingFirstFiltered = getModel();
showPersonsWithName(KEYWORD_MATCHING_MEIER);
Index index = INDEX_FIRST_PERSON;
assertTrue(index.getZeroBased() < getModel().getFilteredPersonList().size());
assertCommandSuccess(index);

/* Case: undo deleting the first person in the filtered list -> first person in filtered list restored */
command = UndoCommand.COMMAND_WORD;
expectedResultMessage = UndoCommand.MESSAGE_SUCCESS;
assertCommandSuccess(command, modelBeforeDeletingFirstFiltered, expectedResultMessage);

/* Case: redo deleting the first person in the filtered list -> first person in filtered list deleted again */
command = RedoCommand.COMMAND_WORD;
removePerson(modelBeforeDeletingFirstFiltered, index);
expectedResultMessage = RedoCommand.MESSAGE_SUCCESS;
assertCommandSuccess(command, modelBeforeDeletingFirstFiltered, expectedResultMessage);

/* Case: filtered person list, delete index within bounds of address book but out of bounds of person list
* -> rejected
*/
Expand Down
12 changes: 12 additions & 0 deletions src/test/java/systemtests/EditCommandSystemTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ public void edit() throws Exception {
/* ------------------ Performing edit operation while a filtered list is being shown ------------------------ */

/* Case: filtered person list, edit index within bounds of address book and person list -> edited */
model = getModel();
showPersonsWithName(KEYWORD_MATCHING_MEIER);
index = INDEX_FIRST_PERSON;
assertTrue(index.getZeroBased() < getModel().getFilteredPersonList().size());
Expand All @@ -109,6 +110,17 @@ public void edit() throws Exception {
editedPerson = new PersonBuilder(personToEdit).withName(VALID_NAME_BOB).build();
assertCommandSuccess(command, index, editedPerson);

/* Case: undo editing the first person in the filtered list -> first person in filtered list restored */
command = UndoCommand.COMMAND_WORD;
expectedResultMessage = UndoCommand.MESSAGE_SUCCESS;
assertCommandSuccess(command, model, expectedResultMessage);

/* Case: redo editing the first person in the filtered list -> first person in filtered list edited again */
command = RedoCommand.COMMAND_WORD;
expectedResultMessage = RedoCommand.MESSAGE_SUCCESS;
model.updatePerson(personToEdit, editedPerson);
assertCommandSuccess(command, model, expectedResultMessage);

/* Case: filtered person list, edit index within bounds of address book but out of bounds of person list
* -> rejected
*/
Expand Down

0 comments on commit fe40136

Please sign in to comment.