nus-cs2103-AY2324S1 / pe-dev-response

0 stars 0 forks source link

Undo not working for edit commands #1136

Open nus-se-script opened 10 months ago

nus-se-script commented 10 months ago

Unable to undo an edit command.

image.png

step 1: edit 1 /phone 1234

image.png

step 2: 'undo'

image.png

problem: the phone number is not restored back to the original phone number.


[original: nus-cs2103-AY2324S1/pe-interim#954] [original labels: type.FunctionalityBug severity.Medium]

nknguyenhc commented 10 months ago

Team's Response

We acknowledge that this is a bug in our programme. The issue lies with the edit command. In our EditCommand.java, under the method execute:

    @Override
    public CommandResult execute(Model model) throws CommandException {
        assert model != null : "Model should not be null";
        List<Person> lastShownList = model.getDisplayedPersonList();

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

        Person personToEdit = lastShownList.get(index.getZeroBased());
        EditPersonDescriptor editPersonDescriptor = new EditPersonDescriptor(personToEdit);
        this.editAction.edit(editPersonDescriptor, index);
        Person editedPerson = editPersonDescriptor.toPerson();
        assert editedPerson != null : "Edited person should not be null";

        if (!personToEdit.isSame(editedPerson) && model.hasPerson(editedPerson)) {
            throw new CommandException(MESSAGE_DUPLICATE_PERSON);
        }

        model.setItem(personToEdit, editedPerson);
        model.updateDisplayedPersonList(PREDICATE_SHOW_ALL_PERSONS, null);
        return new CommandResult(String.format(MESSAGE_EDIT_PERSON_SUCCESS, Messages.format(editedPerson)));
    }

Notice that after the model replaces the old person with the edited person, the method calls model::updateDisplayedPersonList, which then refreshes the filter on the contact list. However, to accommodate for undo and redo, this call creates a new version of VersionedNetworkBook, hence in this one command execution, two new versions of VersionedNetworkBook are created. Hence it requires two undo command to undo one edit command. Besides, this contact list filter resetting is unnecessary and can cause confusion for users who might want to see the direct different right after they update. Removing this line of code shows the difference made by the edit command better, and resolves this bug.

Duplicate status (if any):

--