From 8379bf79c413c9395649617426473e6d1e3e6c8b Mon Sep 17 00:00:00 2001 From: "YAMIDARK\\Jun An" Date: Tue, 16 Jan 2018 20:39:02 +0800 Subject: [PATCH 1/5] ModelManager: Set default Predicate for filteredPersons A newly initialised ModelManager does not set a predicate in filteredPersons, thus it will have a null value set as the predicate. This could cause problems when we may need to get and use the predicate of filteredPersons as our code base has chosen not to deal with null value predicates (as revised in [1]). Let's update ModelManager constructor to set a show all persons as the default predicate in filteredPersons. [1] https://github.com/se-edu/addressbook-level4/commit/60753179a8875fd4b2af4da7ed43185284e210b7 --- src/main/java/seedu/address/model/ModelManager.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/main/java/seedu/address/model/ModelManager.java b/src/main/java/seedu/address/model/ModelManager.java index 095c831cfbf8..b1b57caea7c8 100644 --- a/src/main/java/seedu/address/model/ModelManager.java +++ b/src/main/java/seedu/address/model/ModelManager.java @@ -37,6 +37,8 @@ public ModelManager(ReadOnlyAddressBook addressBook, UserPrefs userPrefs) { this.addressBook = new AddressBook(addressBook); filteredPersons = new FilteredList<>(this.addressBook.getPersonList()); + // set a show all person as default predicate to prevent filteredPersons having a null value predicate + filteredPersons.setPredicate(PREDICATE_SHOW_ALL_PERSONS); } public ModelManager() { @@ -92,6 +94,15 @@ public ObservableList getFilteredPersonList() { return FXCollections.unmodifiableObservableList(filteredPersons); } + @Override + @SuppressWarnings("unchecked") + public Predicate getFilteredPersonListPredicate() { + Predicate toReturn = (Predicate) filteredPersons.getPredicate(); + // the predicate set inside filteredPersons should never be null + assert toReturn != null; + return toReturn; + } + @Override public void updateFilteredPersonList(Predicate predicate) { requireNonNull(predicate); From a038c1999c95a1a4f39a3046d2338cbbcc820966 Mon Sep 17 00:00:00 2001 From: "YAMIDARK\\Jun An" Date: Tue, 9 Jan 2018 00:49:01 +0800 Subject: [PATCH 2/5] Model: Add method to get Predicate of filteredPersons The Model does not contain any method to get the Predicate of its filteredPersons. As such other classes have no means to get the Predicate of the filteredPersons if needed. Let's add a method in Model to get the Predicate of its filteredPersons. --- src/main/java/seedu/address/model/Model.java | 3 +++ .../address/logic/commands/AddCommandTest.java | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/main/java/seedu/address/model/Model.java b/src/main/java/seedu/address/model/Model.java index 9c6aa4e766a0..1b0121562e45 100644 --- a/src/main/java/seedu/address/model/Model.java +++ b/src/main/java/seedu/address/model/Model.java @@ -39,6 +39,9 @@ void updatePerson(ReadOnlyPerson target, ReadOnlyPerson editedPerson) /** Returns an unmodifiable view of the filtered person list */ ObservableList getFilteredPersonList(); + /** Returns the predicate used in the filtered person list */ + Predicate getFilteredPersonListPredicate(); + /** * Updates the filter of the filtered person list to filter by the given {@code predicate}. * @throws NullPointerException if {@code predicate} is null. diff --git a/src/test/java/seedu/address/logic/commands/AddCommandTest.java b/src/test/java/seedu/address/logic/commands/AddCommandTest.java index b327ef2d805b..dcf3ff19be7d 100644 --- a/src/test/java/seedu/address/logic/commands/AddCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/AddCommandTest.java @@ -129,6 +129,12 @@ public ObservableList getFilteredPersonList() { return null; } + @Override + public Predicate getFilteredPersonListPredicate() { + fail("This method should not be called."); + return null; + } + @Override public void updateFilteredPersonList(Predicate predicate) { fail("This method should not be called."); @@ -148,6 +154,11 @@ public void addPerson(ReadOnlyPerson person) throws DuplicatePersonException { public ReadOnlyAddressBook getAddressBook() { return new AddressBook(); } + + @Override + public Predicate getFilteredPersonListPredicate() { + return PREDICATE_SHOW_ALL_PERSONS; + } } /** @@ -165,6 +176,11 @@ public void addPerson(ReadOnlyPerson person) throws DuplicatePersonException { public ReadOnlyAddressBook getAddressBook() { return new AddressBook(); } + + @Override + public Predicate getFilteredPersonListPredicate() { + return PREDICATE_SHOW_ALL_PERSONS; + } } } From 2ab6b312fdb9085531155de64bcf07087119b7c6 Mon Sep 17 00:00:00 2001 From: "YAMIDARK\\Jun An" Date: Tue, 16 Jan 2018 14:48:26 +0800 Subject: [PATCH 3/5] AddCommandTest: Add comments in ModelStub subclasses ModelStub subclasses override getAddressBook() and getFilteredPersonListPredicate() to return default values as they are called to save the current state before addPerson(ReadOnlyPerson) is called in the execution of an AddCommand. This may not be intuitive on first look to developers why we are overriding these additional methods. Let's add comments accordingly to document why we are overriding these additional methods. --- .../java/seedu/address/logic/commands/AddCommandTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/java/seedu/address/logic/commands/AddCommandTest.java b/src/test/java/seedu/address/logic/commands/AddCommandTest.java index dcf3ff19be7d..e6cb430dda9a 100644 --- a/src/test/java/seedu/address/logic/commands/AddCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/AddCommandTest.java @@ -150,6 +150,8 @@ public void addPerson(ReadOnlyPerson person) throws DuplicatePersonException { throw new DuplicatePersonException(); } + /* ------------ These methods below are called prior to calling addPerson(ReadOnlyPerson) ------------------- */ + @Override public ReadOnlyAddressBook getAddressBook() { return new AddressBook(); @@ -172,6 +174,8 @@ public void addPerson(ReadOnlyPerson person) throws DuplicatePersonException { personsAdded.add(new Person(person)); } + /* ------------ These methods below are called prior to calling addPerson(ReadOnlyPerson) ------------------- */ + @Override public ReadOnlyAddressBook getAddressBook() { return new AddressBook(); From 5003f94f40be55355cec45c0325ffd2a379733c7 Mon Sep 17 00:00:00 2001 From: "YAMIDARK\\Jun An" Date: Wed, 17 Jan 2018 14:40:32 +0800 Subject: [PATCH 4/5] TestUtil: Update methods to use filteredPersons Methods in TestUtil get its person and index from model.getAddressBook().getPersonList() which is the unfiltered person list. This cause problems when we build tests on a filtered list as we will get the incorrect person or index upon calling these methods in TestUtil. Let's update methods in TestUtil to get from model.getFilteredPersonList() which is the filtered person list instead. Refer to this discussion on why we were using model.getAddressBook().getPersonList() then and the need to change to model.getFilteredPersonList() now. https://github.com/se-edu/addressbook-level4/pull/792#issuecomment-358053595 --- src/test/java/seedu/address/testutil/TestUtil.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/java/seedu/address/testutil/TestUtil.java b/src/test/java/seedu/address/testutil/TestUtil.java index 29e457791c8b..973d4409a49c 100644 --- a/src/test/java/seedu/address/testutil/TestUtil.java +++ b/src/test/java/seedu/address/testutil/TestUtil.java @@ -35,20 +35,20 @@ public static String getFilePathInSandboxFolder(String fileName) { * Returns the middle index of the person in the {@code model}'s person list. */ public static Index getMidIndex(Model model) { - return Index.fromOneBased(model.getAddressBook().getPersonList().size() / 2); + return Index.fromOneBased(model.getFilteredPersonList().size() / 2); } /** * Returns the last index of the person in the {@code model}'s person list. */ public static Index getLastIndex(Model model) { - return Index.fromOneBased(model.getAddressBook().getPersonList().size()); + return Index.fromOneBased(model.getFilteredPersonList().size()); } /** * Returns the person in the {@code model}'s person list at {@code index}. */ public static ReadOnlyPerson getPerson(Model model, Index index) { - return model.getAddressBook().getPersonList().get(index.getZeroBased()); + return model.getFilteredPersonList().get(index.getZeroBased()); } } From 5b5efcefa9380c9cb8bbdedf889ab1be8d07d731 Mon Sep 17 00:00:00 2001 From: "YAMIDARK\\Jun An" Date: Wed, 10 Jan 2018 00:15:59 +0800 Subject: [PATCH 5/5] 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 could cause the redoing of commands that are index-based such as DeleteCommand and EditCommand to execute itself 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 restore to its previous view right before executing its command to ensure index-based Commands will execute on the correct Person * index-based CommandTests to include testing of undo()/redo() on filtered person list and any other affected tests --- .../logic/commands/UndoableCommand.java | 15 +++++++++++++++ .../logic/commands/RedoCommandTest.java | 18 +++++++++++++++--- .../logic/commands/UndoableCommandTest.java | 8 +++++++- .../seedu/address/testutil/TypicalPersons.java | 1 + .../systemtests/DeleteCommandSystemTest.java | 18 +++++++++++++++++- .../systemtests/EditCommandSystemTest.java | 12 ++++++++++++ 6 files changed, 67 insertions(+), 5 deletions(-) diff --git a/src/main/java/seedu/address/logic/commands/UndoableCommand.java b/src/main/java/seedu/address/logic/commands/UndoableCommand.java index 1ba888ead594..41c774e83854 100644 --- a/src/main/java/seedu/address/logic/commands/UndoableCommand.java +++ b/src/main/java/seedu/address/logic/commands/UndoableCommand.java @@ -4,15 +4,19 @@ import static seedu.address.commons.util.CollectionUtil.requireAllNonNull; import static seedu.address.model.Model.PREDICATE_SHOW_ALL_PERSONS; +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 Predicate previousPredicate; protected abstract CommandResult executeUndoableCommand() throws CommandException; @@ -24,6 +28,13 @@ private void saveAddressBookSnapshot() { this.previousAddressBook = new AddressBook(model.getAddressBook()); } + /** + * Stores the predicate used in {@code model#filteredPersons}. + */ + private void savePredicateSnapshot() { + previousPredicate = model.getFilteredPersonListPredicate(); + } + /** * Reverts the AddressBook to the state before this command * was executed and updates the filtered person list to @@ -42,6 +53,9 @@ protected final void undo() { protected final void redo() { requireNonNull(model); try { + // restore filtered person list to the same previous view to ensure command executes itself on the same + // correct {@code Person} as before + model.updateFilteredPersonList(previousPredicate); executeUndoableCommand(); } catch (CommandException ce) { throw new AssertionError("The command has been successfully executed previously; " @@ -53,6 +67,7 @@ protected final void redo() { @Override public final CommandResult execute() throws CommandException { saveAddressBookSnapshot(); + savePredicateSnapshot(); return executeUndoableCommand(); } } diff --git a/src/test/java/seedu/address/logic/commands/RedoCommandTest.java b/src/test/java/seedu/address/logic/commands/RedoCommandTest.java index 840fea5031d7..580cae44bfae 100644 --- a/src/test/java/seedu/address/logic/commands/RedoCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/RedoCommandTest.java @@ -34,13 +34,25 @@ public void setUp() { } @Test - public void execute() { - UndoRedoStack undoRedoStack = prepareStack( + public void execute() throws Exception { + UndoRedoStack undoRedoStackOne = prepareStack( + Arrays.asList(deleteCommandOne, deleteCommandTwo), Collections.emptyList()); + UndoRedoStack undoRedoStackTwo = prepareStack( Collections.emptyList(), Arrays.asList(deleteCommandTwo, deleteCommandOne)); + UndoCommand undoCommand = new UndoCommand(); RedoCommand redoCommand = new RedoCommand(); - redoCommand.setData(model, EMPTY_COMMAND_HISTORY, undoRedoStack); + undoCommand.setData(model, EMPTY_COMMAND_HISTORY, undoRedoStackOne); + redoCommand.setData(model, EMPTY_COMMAND_HISTORY, undoRedoStackTwo); Model expectedModel = new ModelManager(getTypicalAddressBook(), new UserPrefs()); + // run the commands first to save their states + deleteCommandOne.execute(); + deleteCommandTwo.execute(); + + // revert back to original state + undoCommand.execute(); + undoCommand.execute(); + // multiple commands in redoStack deleteFirstPerson(expectedModel); assertCommandSuccess(redoCommand, model, RedoCommand.MESSAGE_SUCCESS, expectedModel); diff --git a/src/test/java/seedu/address/logic/commands/UndoableCommandTest.java b/src/test/java/seedu/address/logic/commands/UndoableCommandTest.java index 41d97ab15c60..9446c8438d91 100644 --- a/src/test/java/seedu/address/logic/commands/UndoableCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/UndoableCommandTest.java @@ -36,7 +36,13 @@ public void executeUndo() throws Exception { } @Test - public void redo() { + public void redo() throws Exception { + // run the command once to save the states + dummyCommand.execute(); + + // revert back to original state + dummyCommand.undo(); + showFirstPersonOnly(model); // redo() should cause the model's filtered list to show all persons diff --git a/src/test/java/seedu/address/testutil/TypicalPersons.java b/src/test/java/seedu/address/testutil/TypicalPersons.java index 11144bfd356c..538932547aad 100644 --- a/src/test/java/seedu/address/testutil/TypicalPersons.java +++ b/src/test/java/seedu/address/testutil/TypicalPersons.java @@ -57,6 +57,7 @@ public class TypicalPersons { .build(); public static final String KEYWORD_MATCHING_MEIER = "Meier"; // A keyword that matches MEIER + public static final String KEYWORD_MATCHING_FIONA = "Fiona"; // A keyword that matches FIONA private TypicalPersons() {} // prevents instantiation diff --git a/src/test/java/systemtests/DeleteCommandSystemTest.java b/src/test/java/systemtests/DeleteCommandSystemTest.java index 4a12f622d5a3..dd03f2435f9f 100644 --- a/src/test/java/systemtests/DeleteCommandSystemTest.java +++ b/src/test/java/systemtests/DeleteCommandSystemTest.java @@ -4,10 +4,12 @@ import static seedu.address.commons.core.Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX; import static seedu.address.commons.core.Messages.MESSAGE_UNKNOWN_COMMAND; import static seedu.address.logic.commands.DeleteCommand.MESSAGE_DELETE_PERSON_SUCCESS; +import static seedu.address.model.Model.PREDICATE_SHOW_ALL_PERSONS; import static seedu.address.testutil.TestUtil.getLastIndex; import static seedu.address.testutil.TestUtil.getMidIndex; import static seedu.address.testutil.TestUtil.getPerson; import static seedu.address.testutil.TypicalIndexes.INDEX_FIRST_PERSON; +import static seedu.address.testutil.TypicalPersons.KEYWORD_MATCHING_FIONA; import static seedu.address.testutil.TypicalPersons.KEYWORD_MATCHING_MEIER; import org.junit.Test; @@ -60,11 +62,25 @@ 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 */ - showPersonsWithName(KEYWORD_MATCHING_MEIER); + Model modelBeforeDeletingFirstFiltered = getModel(); + showPersonsWithName(KEYWORD_MATCHING_FIONA); + Model modelAfterDeletingFirstFiltered = getModel(); 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(modelAfterDeletingFirstFiltered, index); + modelAfterDeletingFirstFiltered.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS); + expectedResultMessage = RedoCommand.MESSAGE_SUCCESS; + assertCommandSuccess(command, modelAfterDeletingFirstFiltered, 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..6700b89106f5 100644 --- a/src/test/java/systemtests/EditCommandSystemTest.java +++ b/src/test/java/systemtests/EditCommandSystemTest.java @@ -102,6 +102,7 @@ public void edit() throws Exception { /* Case: filtered person list, edit index within bounds of address book and person list -> edited */ showPersonsWithName(KEYWORD_MATCHING_MEIER); + model = getModel(); index = INDEX_FIRST_PERSON; assertTrue(index.getZeroBased() < getModel().getFilteredPersonList().size()); command = EditCommand.COMMAND_WORD + " " + index.getOneBased() + " " + NAME_DESC_BOB; @@ -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 */