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() to 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 1835928 commit 3608440
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 3608440

Please sign in to comment.