From fe40136fd9f0f4e60ded191c760bda9ec7f84a4d Mon Sep 17 00:00:00 2001 From: "YAMIDARK\\Jun An" Date: Wed, 10 Jan 2018 00:15:59 +0800 Subject: [PATCH] UndoableCommand: Update `redo()` restore previous view before executing 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. --- .../address/logic/commands/UndoableCommand.java | 14 ++++++++++++++ .../java/systemtests/DeleteCommandSystemTest.java | 12 ++++++++++++ .../java/systemtests/EditCommandSystemTest.java | 12 ++++++++++++ 3 files changed, 38 insertions(+) diff --git a/src/main/java/seedu/address/logic/commands/UndoableCommand.java b/src/main/java/seedu/address/logic/commands/UndoableCommand.java index 1ba888ead594..79ba7ce886fb 100644 --- a/src/main/java/seedu/address/logic/commands/UndoableCommand.java +++ b/src/main/java/seedu/address/logic/commands/UndoableCommand.java @@ -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> previousPredicate = Optional.empty(); protected abstract CommandResult executeUndoableCommand() throws CommandException; @@ -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 @@ -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; " @@ -53,6 +66,7 @@ protected final void redo() { @Override public final CommandResult execute() throws CommandException { saveAddressBookSnapshot(); + savePredicateSnapshot(); return executeUndoableCommand(); } } diff --git a/src/test/java/systemtests/DeleteCommandSystemTest.java b/src/test/java/systemtests/DeleteCommandSystemTest.java index 4a12f622d5a3..97a12159fc66 100644 --- a/src/test/java/systemtests/DeleteCommandSystemTest.java +++ b/src/test/java/systemtests/DeleteCommandSystemTest.java @@ -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 */ diff --git a/src/test/java/systemtests/EditCommandSystemTest.java b/src/test/java/systemtests/EditCommandSystemTest.java index 71796d3fa19a..43a1ccf8a7a4 100644 --- a/src/test/java/systemtests/EditCommandSystemTest.java +++ b/src/test/java/systemtests/EditCommandSystemTest.java @@ -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()); @@ -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 */