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

Issue with undo/redo and modifications to filtered lists #737 #792

Merged
merged 6 commits into from
Feb 13, 2018
Merged

Issue with undo/redo and modifications to filtered lists #737 #792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5c9b14d
UndoableCommand: add preprocessUndoableCommand() method
yamidark Jan 22, 2018
3825b99
CommandTestUtil: update showFirstPersonOnly(Model)
yamidark Jan 31, 2018
d840c89
DeleteCommand#preprocessUndoableCommand(): find store personToDelete
yamidark Jan 22, 2018
3563cea
EditCommand#preprocessUndoableCommand(): find and store personToEdit
yamidark Jan 22, 2018
ea2d584
DeleteCommandTest: add integration tests with Undo/RedoCommand
yamidark Jan 27, 2018
4be7dd7
EditCommandTest: add integration tests with Undo/RedoCommand
yamidark Jan 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions src/main/java/seedu/address/logic/commands/DeleteCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package seedu.address.logic.commands;

import static java.util.Objects.requireNonNull;

import java.util.List;
import java.util.Objects;

import seedu.address.commons.core.Messages;
import seedu.address.commons.core.index.Index;
Expand All @@ -24,22 +27,16 @@ public class DeleteCommand extends UndoableCommand {

private final Index targetIndex;

private Person personToDelete;

public DeleteCommand(Index targetIndex) {
this.targetIndex = targetIndex;
}


@Override
public CommandResult executeUndoableCommand() throws CommandException {

List<Person> lastShownList = model.getFilteredPersonList();

if (targetIndex.getZeroBased() >= lastShownList.size()) {
throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
}

Person personToDelete = lastShownList.get(targetIndex.getZeroBased());

public CommandResult executeUndoableCommand() {
requireNonNull(personToDelete);
try {
model.deletePerson(personToDelete);
} catch (PersonNotFoundException pnfe) {
Expand All @@ -49,10 +46,22 @@ public CommandResult executeUndoableCommand() throws CommandException {
return new CommandResult(String.format(MESSAGE_DELETE_PERSON_SUCCESS, personToDelete));
}

@Override
protected void preprocessUndoableCommand() throws CommandException {
List<Person> lastShownList = model.getFilteredPersonList();

if (targetIndex.getZeroBased() >= lastShownList.size()) {
throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
}

personToDelete = lastShownList.get(targetIndex.getZeroBased());
}

@Override
public boolean equals(Object other) {
return other == this // short circuit if same object
|| (other instanceof DeleteCommand // instanceof handles nulls
&& this.targetIndex.equals(((DeleteCommand) other).targetIndex)); // state check
&& this.targetIndex.equals(((DeleteCommand) other).targetIndex) // state check
&& Objects.equals(this.personToDelete, ((DeleteCommand) other).personToDelete));
}
}
28 changes: 18 additions & 10 deletions src/main/java/seedu/address/logic/commands/EditCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;

Expand Down Expand Up @@ -54,6 +55,9 @@ public class EditCommand extends UndoableCommand {
private final Index index;
private final EditPersonDescriptor editPersonDescriptor;

private Person personToEdit;
private Person editedPerson;

/**
* @param index of the person in the filtered person list to edit
* @param editPersonDescriptor details to edit the person with
Expand All @@ -68,15 +72,6 @@ public EditCommand(Index index, EditPersonDescriptor editPersonDescriptor) {

@Override
public CommandResult executeUndoableCommand() throws CommandException {
List<Person> lastShownList = model.getFilteredPersonList();

if (index.getZeroBased() >= lastShownList.size()) {
throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
}

Person personToEdit = lastShownList.get(index.getZeroBased());
Person editedPerson = createEditedPerson(personToEdit, editPersonDescriptor);

try {
model.updatePerson(personToEdit, editedPerson);
} catch (DuplicatePersonException dpe) {
Expand All @@ -88,6 +83,18 @@ public CommandResult executeUndoableCommand() throws CommandException {
return new CommandResult(String.format(MESSAGE_EDIT_PERSON_SUCCESS, editedPerson));
}

@Override
protected void preprocessUndoableCommand() throws CommandException {
List<Person> lastShownList = model.getFilteredPersonList();

if (index.getZeroBased() >= lastShownList.size()) {
throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
}

personToEdit = lastShownList.get(index.getZeroBased());
editedPerson = createEditedPerson(personToEdit, editPersonDescriptor);
}

/**
* Creates and returns a {@code Person} with the details of {@code personToEdit}
* edited with {@code editPersonDescriptor}.
Expand Down Expand Up @@ -119,7 +126,8 @@ public boolean equals(Object other) {
// state check
EditCommand e = (EditCommand) other;
return index.equals(e.index)
&& editPersonDescriptor.equals(e.editPersonDescriptor);
&& editPersonDescriptor.equals(e.editPersonDescriptor)
&& Objects.equals(personToEdit, e.personToEdit);
}

/**
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/seedu/address/logic/commands/UndoableCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ private void saveAddressBookSnapshot() {
this.previousAddressBook = new AddressBook(model.getAddressBook());
}

/**
* This method is called before the execution of {@code UndoableCommand}.
* {@code UndoableCommand}s that require this preprocessing step should override this method.
*/
protected void preprocessUndoableCommand() throws CommandException {}

/**
* Reverts the AddressBook to the state before this command
* was executed and updates the filtered person list to
Expand Down Expand Up @@ -53,6 +59,7 @@ protected final void redo() {
@Override
public final CommandResult execute() throws CommandException {
saveAddressBookSnapshot();
preprocessUndoableCommand();
return executeUndoableCommand();
}
}
31 changes: 28 additions & 3 deletions src/test/java/seedu/address/logic/commands/CommandTestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package seedu.address.logic.commands;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static seedu.address.logic.parser.CliSyntax.PREFIX_ADDRESS;
import static seedu.address.logic.parser.CliSyntax.PREFIX_EMAIL;
Expand All @@ -12,6 +13,9 @@
import java.util.Arrays;
import java.util.List;

import seedu.address.commons.core.index.Index;
import seedu.address.logic.CommandHistory;
import seedu.address.logic.UndoRedoStack;
import seedu.address.logic.commands.exceptions.CommandException;
import seedu.address.model.AddressBook;
import seedu.address.model.Model;
Expand Down Expand Up @@ -107,10 +111,13 @@ public static void assertCommandFailure(Command command, Model actualModel, Stri
}

/**
* Updates {@code model}'s filtered list to show only the first person in the {@code model}'s address book.
* Updates {@code model}'s filtered list to show only the person at the given {@code targetIndex} in the
* {@code model}'s address book.
*/
public static void showFirstPersonOnly(Model model) {
Person person = model.getAddressBook().getPersonList().get(0);
public static void showPersonAtIndex(Model model, Index targetIndex) {
assertTrue(targetIndex.getZeroBased() < model.getFilteredPersonList().size());

Person person = model.getFilteredPersonList().get(targetIndex.getZeroBased());
final String[] splitName = person.getName().fullName.split("\\s+");
model.updateFilteredPersonList(new NameContainsKeywordsPredicate(Arrays.asList(splitName[0])));

Expand All @@ -128,4 +135,22 @@ public static void deleteFirstPerson(Model model) {
throw new AssertionError("Person in filtered list must exist in model.", pnfe);
}
}

/**
* Returns an {@code UndoCommand} with the given {@code model} and {@code undoRedoStack} set.
*/
public static UndoCommand prepareUndoCommand(Model model, UndoRedoStack undoRedoStack) {
UndoCommand undoCommand = new UndoCommand();
undoCommand.setData(model, new CommandHistory(), undoRedoStack);
return undoCommand;
}

/**
* Returns a {@code RedoCommand} with the given {@code model} and {@code undoRedoStack} set.
*/
public static RedoCommand prepareRedoCommand(Model model, UndoRedoStack undoRedoStack) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can merge these methods.
prepareCommand(Class commandClass, Model model, UndoRedoStack undoRedoStack). Not too sure tho x.x

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will try this in my next iteration.

Copy link
Contributor Author

@yamidark yamidark Feb 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried something like this:

public static<T extends Command> T prepareUndoRedoCommand(Class<? extends Command> commandClass, Model model,
                                             UndoRedoStack undoRedoStack) throws Exception {
        Command command = commandClass.getConstructor().newInstance();
        command.setData(model, new CommandHistory(), undoRedoStack);
        return (T) command;
    }

Can't use prepareCommand as the name since DeleteCommandTest and EditCommandTest has it for their own commands.
Looks quite ugly to me though =x.
What's your opinion on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok let's revert this then.

RedoCommand redoCommand = new RedoCommand();
redoCommand.setData(model, new CommandHistory(), undoRedoStack);
return redoCommand;
}
}
91 changes: 83 additions & 8 deletions src/test/java/seedu/address/logic/commands/DeleteCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package seedu.address.logic.commands;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;
import static seedu.address.logic.commands.CommandTestUtil.assertCommandFailure;
import static seedu.address.logic.commands.CommandTestUtil.assertCommandSuccess;
import static seedu.address.logic.commands.CommandTestUtil.showFirstPersonOnly;
import static seedu.address.logic.commands.CommandTestUtil.prepareRedoCommand;
import static seedu.address.logic.commands.CommandTestUtil.prepareUndoCommand;
import static seedu.address.logic.commands.CommandTestUtil.showPersonAtIndex;
import static seedu.address.testutil.TypicalIndexes.INDEX_FIRST_PERSON;
import static seedu.address.testutil.TypicalIndexes.INDEX_SECOND_PERSON;
import static seedu.address.testutil.TypicalPersons.getTypicalAddressBook;
Expand All @@ -21,7 +24,8 @@
import seedu.address.model.person.Person;

/**
* Contains integration tests (interaction with the Model) and unit tests for {@code DeleteCommand}.
* Contains integration tests (interaction with the Model, UndoCommand and RedoCommand) and unit tests for
* {@code DeleteCommand}.
*/
public class DeleteCommandTest {

Expand Down Expand Up @@ -50,7 +54,7 @@ public void execute_invalidIndexUnfilteredList_throwsCommandException() throws E

@Test
public void execute_validIndexFilteredList_success() throws Exception {
showFirstPersonOnly(model);
showPersonAtIndex(model, INDEX_FIRST_PERSON);

Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());
DeleteCommand deleteCommand = prepareCommand(INDEX_FIRST_PERSON);
Expand All @@ -66,7 +70,7 @@ public void execute_validIndexFilteredList_success() throws Exception {

@Test
public void execute_invalidIndexFilteredList_throwsCommandException() {
showFirstPersonOnly(model);
showPersonAtIndex(model, INDEX_FIRST_PERSON);

Index outOfBoundIndex = INDEX_SECOND_PERSON;
// ensures that outOfBoundIndex is still in bounds of address book list
Expand All @@ -78,17 +82,88 @@ public void execute_invalidIndexFilteredList_throwsCommandException() {
}

@Test
public void equals() {
DeleteCommand deleteFirstCommand = new DeleteCommand(INDEX_FIRST_PERSON);
DeleteCommand deleteSecondCommand = new DeleteCommand(INDEX_SECOND_PERSON);
public void executeUndoRedo_validIndexUnfilteredList_success() throws Exception {
UndoRedoStack undoRedoStack = new UndoRedoStack();
UndoCommand undoCommand = prepareUndoCommand(model, undoRedoStack);
RedoCommand redoCommand = prepareRedoCommand(model, undoRedoStack);
Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, method name says validIndexUnfilteredList but you're working on FilteredPersonList?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If im not wrong, since we are doing assertion tests with the model and the integration tests for the interaction with the model, only the filteredPersonList should be visible and used by us even if its unfiltered.

@Zhiyuan-Amos May I have your opinion on this? Not too sure if using filteredPersonList may confuse students. If we change this to getAddressBook().getPersonList() it would then be inconsistent with the other unfiltered tests in EditCommandTest and DeleteCommandTest which may also confuse students. Maybe we raise another issue to change all this?

Copy link
Member

@eugenepeh eugenepeh Feb 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually what I mean is, the method name should be updated to FilteredList if you're working on `FilteredPersonList

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned below, we are working on the unfiltered list here.

Copy link
Member

@eugenepeh eugenepeh Feb 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm okay but its not intuitive enough to know whether we're working on the unfiltered or filtered list here because the exact same line of code
Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()); was used in [3/6].

I assume in order to set filter for the person list, we need to execute the code
showPersonAtIndex(model, INDEX_SECOND_PERSON);?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume in order to set filter for the person list, we need to execute the code
showPersonAtIndex(model, INDEX_SECOND_PERSON);?

Yep.

iirc, we placed the line Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()); for the filtered list right after we call showPersonAtIndex(model, INDEX_SECOND_PERSON); instead of at the top as we thought it would be clearer that we are working on the filtered list, and to prevent too much preparation line codes at the top.

Would putting this back on the top make it clearer like in execute_validIndexFilteredList_success()?
Or maybe we shift the line DeleteCommand deleteCommand = prepareCommand(INDEX_FIRST_PERSON); down to be together with the Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay actually my few concerns are:

  • model is a global variable. Using model.getFilteredPersonList() will carry over state from previous tests, if theres any.
  • The filter is set using the method showPersonAtIndex(model, INDEX_SECOND_PERSON); which is not very intuitive, by just looking at this implementation alone.
  • Method name contains validIndexUnfilteredList but using getFilteredPersonList() method adds on to the confusion.

I would say, it would be better if there is a getUnfilteredPersonList() method or comments that do the explanation. I would prefer the first over latter.

@Zhiyuan-Amos , what do you think?

Copy link
Contributor

@Zhiyuan-Amos Zhiyuan-Amos Feb 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method name contains validIndexUnfilteredList but using getFilteredPersonList() method adds on to the confusion.

Yup I think that using getFilteredPersonList() is quite confusing. But it seems that this issue should be resolved on the main code base side actually; Logic shouldn't be exposing that it's using a filtered list because there's no need to expose this information to the world. The View just simply needs a list to connect to.

So as of now, I'm ok with using getFilteredPersonList(), but we should rename this method eventually (separate issue).

DeleteCommand deleteCommand = prepareCommand(INDEX_FIRST_PERSON);
Model expectedModel = new ModelManager(model.getAddressBook(), new UserPrefs());

// delete -> first person deleted
deleteCommand.execute();
undoRedoStack.push(deleteCommand);

// undo -> reverts addressbook back to previous state and filtered person list to show all persons
assertCommandSuccess(undoCommand, model, UndoCommand.MESSAGE_SUCCESS, expectedModel);

// redo -> same first person deleted again
expectedModel.deletePerson(personToDelete);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include a test to check that the first person in filtered list and unfiltered list are different?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is executed on an unfiltered list, so the person being deleted should be on index 1 on both occasions. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, this test doesn't test for the issue earlier isnt it? where redo deletes the wrong person.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the regression test which handles the issue of redo deletes wrong person is in [3/6] and [4/6].
[5/6] and [6/6] are just extra integration tests that I added in, which is why they are on an unfiltered list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, my confusion comes from the part above.

assertCommandSuccess(redoCommand, model, RedoCommand.MESSAGE_SUCCESS, expectedModel);
}

@Test
public void executeUndoRedo_invalidIndexUnfilteredList_failure() {
UndoRedoStack undoRedoStack = new UndoRedoStack();
UndoCommand undoCommand = prepareUndoCommand(model, undoRedoStack);
RedoCommand redoCommand = prepareRedoCommand(model, undoRedoStack);
Index outOfBoundIndex = Index.fromOneBased(model.getFilteredPersonList().size() + 1);
DeleteCommand deleteCommand = prepareCommand(outOfBoundIndex);

// execution failed -> deleteCommand not pushed into undoRedoStack
assertCommandFailure(deleteCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);

// no commands in undoRedoStack -> undoCommand and redoCommand fail
assertCommandFailure(undoCommand, model, UndoCommand.MESSAGE_FAILURE);
assertCommandFailure(redoCommand, model, RedoCommand.MESSAGE_FAILURE);
}

/**
* 1. Deletes a {@code Person} from a filtered list.
* 2. Undo the deletion.
* 3. The unfiltered list should be shown now. Verify that the index of the previously deleted person in the
* unfiltered list is different from the index at the filtered list.
* 4. Redo the deletion. This ensures {@code RedoCommand} deletes the person object regardless of indexing.
*/
@Test
public void executeUndoRedo_validIndexFilteredList_samePersonDeleted() throws Exception {
UndoRedoStack undoRedoStack = new UndoRedoStack();
UndoCommand undoCommand = prepareUndoCommand(model, undoRedoStack);
RedoCommand redoCommand = prepareRedoCommand(model, undoRedoStack);
DeleteCommand deleteCommand = prepareCommand(INDEX_FIRST_PERSON);
Model expectedModel = new ModelManager(model.getAddressBook(), new UserPrefs());

showPersonAtIndex(model, INDEX_SECOND_PERSON);
Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());
// delete -> deletes second person in unfiltered person list / first person in filtered person list
deleteCommand.execute();
undoRedoStack.push(deleteCommand);

// undo -> reverts addressbook back to previous state and filtered person list to show all persons
assertCommandSuccess(undoCommand, model, UndoCommand.MESSAGE_SUCCESS, expectedModel);

expectedModel.deletePerson(personToDelete);
assertNotEquals(personToDelete, model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()));
// redo -> deletes same second person in unfiltered person list
assertCommandSuccess(redoCommand, model, RedoCommand.MESSAGE_SUCCESS, expectedModel);
}

@Test
public void equals() throws Exception {
DeleteCommand deleteFirstCommand = prepareCommand(INDEX_FIRST_PERSON);
DeleteCommand deleteSecondCommand = prepareCommand(INDEX_SECOND_PERSON);

// same object -> returns true
assertTrue(deleteFirstCommand.equals(deleteFirstCommand));

// same values -> returns true
DeleteCommand deleteFirstCommandCopy = new DeleteCommand(INDEX_FIRST_PERSON);
DeleteCommand deleteFirstCommandCopy = prepareCommand(INDEX_FIRST_PERSON);
assertTrue(deleteFirstCommand.equals(deleteFirstCommandCopy));

// one command preprocessed when previously equal -> returns false
deleteFirstCommandCopy.preprocessUndoableCommand();
assertFalse(deleteFirstCommand.equals(deleteFirstCommandCopy));

// different types -> returns false
assertFalse(deleteFirstCommand.equals(1));

Expand Down
Loading