From 3608440a388631f382ed12af41fa6324bd022358 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() to 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 */