ModelManager#addPerson(ReadOnlyPerson) should not update filtered list

[Zhiyuan-Amos]

addPerson(ReadOnlyPerson) adds the ReadOnlyPerson to the address book
and updates the filtered list to display all persons.

This is unnecessary coupling as the Model should only concern itself with 
adding a ReadOnlyPerson to the address book; it's the job of the commands 
to perform the updating and refreshing of the filtered list.

Let's update this method to no longer update the filtered list.

[Zhiyuan-Amos]

@yamgent issue validation tq ^^

[damithc]

Yes, I think the fact that filtered list should change when a person is added should not be known to the model.

[yamgent]

Let's update this method to not update the filtered list.

I am going to make a wild guess that the intention of the original developer was that, if the user filtered the list, and the new person happens to fit the query used in the last find command, it would be shown in the filtered list (otherwise the user will not be able to see that the new person was added). This may take a bit more time to implement properly, so the original developers simplified the logic by just resetting the filtered list to show everyone.

If we are updating this method to not update the filtered list, then some other classes will have to handle that (and if possible, handle the corner case that I have mentioned in the first paragraph too).

[Zhiyuan-Amos]

If we are updating this method to not update the filtered list, then some other classes will have to handle that (and if possible, handle the corner case that I have mentioned in the first paragraph too).

EditCommand handles the updating of the filtered list to show all. As such, it seems like AddCommand should be the one telling the model's filtered list to show all as well. :) This handles your corner case too.

[yamgent]

The "corner case" meant the scenario when your list is actually filtered (and not showing all person), so the ideal action shouldn't even be to reset the list to show all, but only to add on to the filtered list such that the new person is shown, without making other persons (that was hidden by a previous 'find') visible again.

Anyway this seems to be another case of "the filtered list shouldn't be in the model" problem. In an ideal situation, commands shouldn't even be responsible for updating something that is more related to the UI.

[Zhiyuan-Amos]

Ah I get what you are saying now. @damithc what are your thoughts on Wang Leng's comment that "the ideal action shouldn't even be to reset the list to show all, but only to add on to the filtered list such that the new person is shown, without making other persons (that was hidden by a previous 'find') visible again."?

Anyway this seems to be another case of "the filtered list shouldn't be in the model" problem. In an ideal situation, commands shouldn't even be responsible for updating something that is more related to the UI.

Yup, seems like it :P

[damithc]

Yes it's better not to reset the filtered list view when adding a new person, as long as there is good feedback to reassure user the person added is exactly as the user intended. One benefit of resetting the filtered list is that the new person is guaranteed to be in the new list and and the user can visually confirm the person is added correctly.