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

yamidark · 2018-01-10T15:17:07Z

Fixes #737

EditCommand and DeleteCommand stores the target index of the person to edit
or delete respectively.

As a result, redoing of these index-based commands will be executed on the incorrect
person due to different indexing in differing views.

Let's update EditCommand and DeleteCommand to find and store the personToEdit
or personToDelete respectively before it's first execution. This ensures that future
execution of the same command will be on the correct person.

CanIHasReview-bot · 2018-01-10T15:25:06Z

v1

@yamidark submitted v1 for review.

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/792/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Zhiyuan-Amos

[2/3] The rationale for having a requireNonNull(predicate) is over here: 6075317

Zhiyuan-Amos · 2018-01-10T23:45:56Z

I'm thinking of an alternate algorithm as suggested in #737 (comment).

EditCommand stores an Index and EditPersonDescriptor. Instead, let's store the Person at the particular index, and the EditPersonDescriptor.

DeleteCommand stores an Index. Instead, let's store the Person at the index.

That way, whenever you redo, you will be editing/deleting that particular person, and there's no need to consider both indices & the current predicate / view anymore.

What do you think? :)

yamidark · 2018-01-11T07:37:27Z

Yes, that alternate algorithm seems to be a cleaner way of resolving this bug.

Will start working on it.

Zhiyuan-Amos · 2018-01-11T12:40:08Z

@yamidark sorry for the false alarm in #792 (comment)! :(

yamidark · 2018-01-11T13:30:47Z

@Zhiyuan-Amos Nah, I should have done the check and confirmed the bug myself too.

So should I work on using #737 (comment) or continue with the current method?

Zhiyuan-Amos · 2018-01-11T14:13:36Z

Hmm after thinking further about it, I'm not too sure. Iirc, the developer team was thinking of making all commands undoable. I.e. if I execute: find A followed by list, then I execute undo, I will see the result of find A.

In this case, by having UndoableCommand store a previousPredicate, this new feature can be easily supported in the future.

But specifically for this PR, I noticed that you will have to call model.updateFilteredPersonList(previousPredicate) whenever you call redo(). So there's an unintended side effect that the view will be refreshed for a split second due to the change of the predicate, though it's not much of an issue now since you can't really see it because it happens very quickly.

The plus point of using the solution here is that there's no need to call model.updateFilteredPersonList(previousPredicate) whenever you call redo(); redoing works fine regardless of the state of the filtered list. That way, we solve the issue just by tweaking the Logic component alone, instead of having to couple it a little with the Model component. As such, I think I will favor the other algorithm more.

@damithc Prof, what do you think?

pyokagan · 2018-01-12T13:40:19Z

@Zhiyuan-Amos

So there's an unintended side effect that the view will be refreshed for a split second due to the change of the predicate, though it's not much of an issue now since you can't really see it because it happens very quickly.

I don't think that's true. A UI framework which re-renders on every trivial change of the model would lead to extremely bad performance. From what I can tell, JavaFX will only re-render once the application thread yields control to the event loop. In other words, if we run a command that gets stuck in an infinite loop, the UI won't update at all.

But this prompts me to the fact that performance could also be a factor to consider as well. So, I started to do some simple benchmarks with a large addressbook, only to find an old performance regression instead :-P Issue coming soon.

Anyway, I don't mind using another approach. I don't really have a horse in this race, and this is my last post on this matter.

Zhiyuan-Amos · 2018-01-12T14:13:38Z

I don't think that's true. A UI framework which re-renders on every trivial change of the model would lead to extremely bad performance. From what I can tell, JavaFX will only re-render once the application thread yields control to the event loop. In other words, if we run a command that gets stuck in an infinite loop, the UI won't update at all.

Wow that's insightful, thanks Paul ^^

@yamidark can you try doing the next iteration just by updating EditCommand & DeleteCommand alone? Would like to see whether this implementation is indeed simpler or not. Tq ^^

CanIHasReview-bot · 2018-01-14T04:17:40Z

v2

@yamidark submitted v2 for review.

[v2 1/2] DeleteCommand: Store `Person` to be deleted
[v2 2/2] EditCommand: Store `Person` to be edited

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/792/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Zhiyuan-Amos · 2018-01-14T13:07:51Z

@yamidark seems like the first iteration looks way neater because there's no need to store the boolean flags.

Thanks for your second iteration! :) You can revert back to your first iteration's code, update commit [2/3] as per #792 (review) and submit the 3rd iteration.

CanIHasReview-bot · 2018-01-14T16:54:10Z

v3

@yamidark submitted v3 for review.

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/792/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

eugenepeh

[2/2]

Shouldn't Undo be updated as well?

     protected final void undo() {
         requireAllNonNull(model, previousAddressBook);
         model.resetData(previousAddressBook);
         model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS);
     }

E.g. if I perform find John then find Mary, undo'ing find Mary will show all people again?

Take note of the last line in Redo method too

     model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS);

The javadoc for Redo should be updated as well

 /**
  * Executes the command and updates the filtered person
  * list to show all persons.
  */

Please include, in the commit message, the sequence that you're trying to fix too.

Zhiyuan-Amos

[1/2] Commit message: There's no need to back tick all code. We usually back tick only when the code contains multiple words (see c205d6f)

Commit title can be reworded to:
Model: Add method to get filteredPersonList's predicate

Then you can update the 1st and 3rd paragraph accordingly ^^

Also, for the 2nd paragraph, it's more of Other classes have no means to get..., rather than This prevents other classes from getting...

Zhiyuan-Amos · 2018-01-15T00:57:19Z

src/test/java/seedu/address/logic/commands/AddCommandTest.java

@@ -148,6 +154,9 @@ public void addPerson(ReadOnlyPerson person) throws DuplicatePersonException {
        public ReadOnlyAddressBook getAddressBook() {
            return new AddressBook();
        }
+
+        @Override
+        public Predicate<ReadOnlyPerson> getFilteredPersonListPredicate() { return PREDICATE_SHOW_ALL_PERSONS; }


Is there a reason why you put this method here instead of putting it in ModelStub, just like for getFilteredPersonListPredicate()?

@Zhiyuan-Amos The header comment of ModelStub mentions that all of its methods should be failing, so I thought it would be more consistent to do the same for getFilteredPersonListPredicate() to fail in ModelStub and then return a default value in its subclasses like getAddressBook() method.

Oh ok got it! ^^ On first look, it doesn't seem intuitive that both methods getAddressBook() and getFilteredPersonListPredicate() are needed here, since the header comments for this class says: A Model stub that always throw a DuplicatePersonException when trying to add a person. As such, it seems that we only need to override addPerson.

@eugenepeh should we have some kind of comments to denote that getAddressBook() and getFilteredPersonListPredicate() are required for AddCommand to execute?

Hmm @Zhiyuan-Amos , in such case, I think getFilteredPersonListPredicate() shouldn't even be called in the first place, since throwing DuplicatePersonException won't make AddCommand execute successfully. Hence @yamidark, you should make getFilteredPersonListPredicate() fails for ModelStubThrowingDuplicatePersonException

In this few scenarios, the main purpose is to test whether the functionality is working as per expectation. So if its not suppose to execute, we should override the default functionality to make sure it fails. For the ModelStubAcceptingPersonAdded, however, we can leave it as it is without overriding since

it doesn't not affect any of the test results

even if we integrate Undo/Redo into systemtest, the default functionality does not differ from what we're testing.

@eugenepeh Actually getFilteredPersonListPredicate() gets called before the AddCommand executes and the DuplicatePersonException gets thrown, since we need to save the "state" before actually executing the command, which is why I needed to override again getFilteredPersonListPredicate() to pass in ModelStubThrowingDuplicatePersonException and ModelStubAcceptingPersonAdded.

Yup, @eugenepeh both getAddressBook() and getFilteredPersonListPredicate() are called before addPerson() is called. So, we can't have getFilteredPersonListPredicate() failing :P

As such, should we leave comments? Maybe something like

~~~~~~~~~~~~~~~~~These methods below are called prior to calling addPerson(ReadOnlyPerson)~~~~~~

ahh i c, I thought it was AddCommand execute successfully > get predicate > save state.

In that case, do we even need to override it? It make sense that doing the same for the rest makes it more uniform but at the same time, it doesn't matter (to the test itself) as long as it returns something.

I would prefer retain the default functionality so that our tests cover more area.

Hmm if I'm not wrong, we should override all methods for a Stub so that no matter what implementations are being changed in ModelManager's methods, this test will be unaffected. I.e. The Stub should be independent of ModelManager.

ahh, oh yess... we're extending an empty ModelStub -my turn to get sacked for forgetting the fundamentals >_<-

Yes you're right, I guess the comments are fine then

Zhiyuan-Amos · 2018-01-15T00:57:28Z

src/test/java/seedu/address/logic/commands/AddCommandTest.java

@@ -165,6 +174,9 @@ public void addPerson(ReadOnlyPerson person) throws DuplicatePersonException {
        public ReadOnlyAddressBook getAddressBook() {
            return new AddressBook();
        }
+
+        @Override
+        public Predicate<ReadOnlyPerson> getFilteredPersonListPredicate() { return PREDICATE_SHOW_ALL_PERSONS; }


same as above ^^

Zhiyuan-Amos · 2018-01-15T01:03:41Z

src/main/java/seedu/address/model/ModelManager.java

@@ -92,6 +92,11 @@ public void updatePerson(ReadOnlyPerson target, ReadOnlyPerson editedPerson)
        return FXCollections.unmodifiableObservableList(filteredPersons);
    }

+    @Override
+    public Predicate<ReadOnlyPerson> getFilteredPersonListPredicate() {
+        return (Predicate<ReadOnlyPerson>) filteredPersons.getPredicate();


Since this method may return null, we should specify it in the header comments what does null return means.

eugenepeh · 2018-01-15T01:14:17Z

src/main/java/seedu/address/logic/commands/UndoableCommand.java

@@ -42,6 +54,7 @@ protected final void undo() {
    protected final void redo() {
        requireNonNull(model);
        try {
+            previousPredicate.ifPresent(model::updateFilteredPersonList);


btw @Zhiyuan-Amos , in the case where the state is used in both undo and redo, wouldn't it be confusing to have "previous" prefix in variable names?

Zhiyuan-Amos · 2018-01-15T01:31:27Z

E.g. if I perform find John then find Mary, undo'ing find Mary will show all people again?
btw @Zhiyuan-Amos , in the case where the state is used in both undo and redo, wouldn't it be confusing to have "previous" prefix in variable names?

If I'm reading you correctly, you're saying that model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS) in undo() should be changed to model.updateFilteredPersonList(previousPredicate)? Yup, in that case, the "previous" prefix can be confusing.

Though I'm not sure whether we should be doing that in this PR, since this PR should specifically fix the bug of wrong redo-ing, rather than deciding whether we should update the undo() to display all persons or the previous view ^^

eugenepeh · 2018-01-15T01:35:05Z

Though I'm not sure whether we should be doing that in this PR, since this PR should specifically fix the bug of wrong redo-ing, rather than deciding whether we should update the undo() to display all persons or the previous view ^^

icic. In my point of view, however, the main problem here is that the undo/redo doesn't set the "state" back to the previous one. I believe that by tackling that issue in this PR, we will resolve the existing issue with redo too. Hence, i think it is justifiable that we fix them all in this PR.

Zhiyuan-Amos · 2018-01-15T01:40:03Z

Hmm ok! I think these few choices work:

Fix undo only to show the view according to previousPredicate. That way, redo is bound to execute on the correct view.
Fix redo only to show the view according to previousPredicate before executing it.
Fix both undo and redo to show the view according to previousPredicate.

I think 1. seems good, it seems intuitive as well since by undo-ing, you reset the view to show the user the previous view as well. What do you think? ^^

Zhiyuan-Amos

[2/2] commit message: Remove back ticks as mentioned in the above comment.

Also, you can be more specific by mentioning that EditCommand & DeleteCommand executes on a particular index depending on the current view, which causes the possibility of error. This will then explain why only the tests for EditCommand & DeleteCommand are updated because they are the only commands which executes based on index.

Will review the tests some other time ^^

Zhiyuan-Amos · 2018-01-15T01:14:06Z

src/main/java/seedu/address/logic/commands/UndoableCommand.java


 /**
 * 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();


shouldn't use Optional as a field in classes :P previousPredicate can be of type Predicate<ReadOnlyPerson>.

I'm thinking that for ModelManager#getFilteredPersonListPredicate(), because FilteredList#getPredicate() returns null if there's no Predicate, so what we can do is (make this code neater, i just typed it out quickly :P):

if (filteredPersons.getPredicate() == null) { return PREDICATE_SHOW_ALL_PERSONS; } else { return (Predicate<ReadOnlyPerson>) filteredPersons.getPredicate(); }

This way, you don't have to deal with a null return.

That way, in UndoableCommand#redo(), you can simply:

model.updateFilteredPersonList(previousPredicate);

Zhiyuan-Amos · 2018-01-15T01:47:56Z

src/main/java/seedu/address/logic/commands/UndoableCommand.java

@@ -24,6 +29,13 @@ private void saveAddressBookSnapshot() {
        this.previousAddressBook = new AddressBook(model.getAddressBook());
    }

+    /**
+     * Stores the current state of {@code model#filteredPersonList#Predicate}


This looks like a violation of Law of Demeter :P There's no need to be that specific, maybe you can write something like Stores the predicate used by the model to filter the list of persons.

yamidark · 2018-01-15T10:15:56Z

Hmm ok! I think these few choices work:

Fix undo only to show the view according to previousPredicate. That way, redo is bound to execute on the correct view.

Fix redo only to show the view according to previousPredicate before executing it.

Fix both undo and redo to show the view according to previousPredicate.

I think 1. seems good, it seems intuitive as well since by undo-ing, you reset the view to show the user > the previous view as well. What do you think? ^^

I also thought of doing 1. initially and removing model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS) in redo() as I feel it would be more intuitive to the user for the view to be restored to before or after executing the command.

However, I thought that since I should be focusing only on the current issue in this PR hence I ended up doing 2. instead.

As such if fixing the issue of restoring the "state" after undo()/redo() should fall within this issue/PR, then I agree that 1. would be better and I can work on that too.

Zhiyuan-Amos · 2018-01-15T13:55:06Z

I also thought of doing 1. initially and removing model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS) in redo() as I feel it would be more intuitive to the user for the view to be restored to before or after executing the command.
However, I thought that since I should be focusing only on the current issue in this PR hence I ended up doing 2. instead.

We don't need to remove model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS) in redo() for the algorithm to work right?

undo() will look something like this:

 protected final void undo() {
     requireAllNonNull(model, previousAddressBook);
     model.resetData(previousAddressBook);
     model.updateFilteredPersonList(previousPredicate); // the change is only done in this line
 }

yamidark · 2018-01-15T14:46:00Z

We don't need to remove model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS) in redo() for the algorithm to work right?

@Zhiyuan-Amos Yes, that's right.

I just thought that restoring the view for undo() while leaving redo() to show all persons would be weird and perhaps confusing to users since DeleteCommand does not set the view to all persons after it's execution.

Zhiyuan-Amos · 2018-02-11T12:55:08Z

@yamidark [3/4] & [4/4] 3rd paragraphs do not match.

EditCommand stores the target index of the person to edit. As a result, redoing of EditCommand will be executed on the incorrect person due to different indexing in differing views. Let's update EditCommand#preprocessUndoableCommand() to find and store the personToEdit before its first execution. This ensures that future execution of the same command will be on the correct person, independent of the current view. Let's also update EditCommand#equals(Object) method to include checking of personToEdit. This allows other classes to know that EditCommand transits between 2 states, preprocessed and not-preprocessed.

DeleteCommandTest does not have any integration tests for the interaction with UndoCommand and RedoCommand on an unfiltered list. Let's add integration tests for the interaction of DeleteCommand with UndoCommand and RedoCommand on an unfiltered list.

EditCommandTest does not have any integration tests for the interaction with UndoCommand and RedoCommand on an unfiltered list. Let's add integration tests for the interaction of EditCommand with UndoCommand and RedoCommand on an unfiltered list.

CanIHasReview-bot · 2018-02-11T13:08:08Z

v18

@yamidark submitted v18 for review.

[v18 1/6] UndoableCommand: add preprocessUndoableCommand() method
[v18 2/6] CommandTestUtil: update showFirstPersonOnly(Model)
[v18 3/6] DeleteCommand#preprocessUndoableCommand(): find store personToDelete
[v18 4/6] EditCommand#preprocessUndoableCommand(): find and store personToEdit
[v18 5/6] DeleteCommandTest: add integration tests with Undo/RedoCommand
[v18 6/6] EditCommandTest: add integration tests with Undo/RedoCommand

(📚 Archive) (📈 Interdiff between v17 and v18)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/792/18/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

eugenepeh · 2018-02-11T16:03:47Z

Sorry @yamidark , some additional concerns of mine

Hmm for [5/6] & [6/6] executeUndoRedo_validIndexUnfilteredList_success, on second thought, does it actually provide any value in testing it?
If it works on the filteredlist, it should goes without saying that it will work on unfilteredlist if it's using same implementation.

Zhiyuan-Amos · 2018-02-11T22:43:16Z

@eugenepeh

You are right that if the filteredList test works, then it should seem that the unfilteredList will work as well.

How I see it is that we are recognizing that there are 2 states in which the list can be in: Either filtered or unfiltered. Which is why we choose to test for both states. Not too sure if this argument is strong enough though.

eugenepeh · 2018-02-12T02:20:41Z

How I see it is that we are recognizing that there are 2 states in which the list can be in: Either filtered or unfiltered. Which is why we choose to test for both states. Not too sure if this argument is strong enough though.

Hmm, okay. Not a very strong argument but I guess it is sounds legit.

Lastly, I would like to point out that if this commit is not going to be squash merged, the commit message for 5/6 and 6/6 needs to be updated.

DeleteCommandTest does not have any integration tests for the
interaction with UndoCommand and RedoCommand on an unfiltered list.
Let's add integration tests for the interaction of DeleteCommand with
UndoCommand and RedoCommand on an unfiltered list.

Integration tests were implemented in 3/6 & 4/6

yamidark · 2018-02-12T08:02:08Z

@eugenepeh [5/6] and [6/6] add integration tests on the unfiltered list as stated in the commit message, while [3/6] and [4/6] integration tests are on the filtered list.

eugenepeh · 2018-02-12T09:06:14Z

@eugenepeh [5/6] and [6/6] add integration tests on the unfiltered list as stated in the commit message, while [3/6] and [4/6] integration tests are on the filtered list.

Oh okay, my eye sight are getting worse >_<
In that case, no other comments. @Zhiyuan-Amos can go ahead and merge le

Zhiyuan-Amos · 2018-02-12T09:49:08Z

Hmm, okay. Not a very strong argument but I guess it is sounds legit.

Yup I know it's not a strong argument >< For now it's also for consistency, since for XCommandTest and XCommandSystemTest we perform tests on both filtered and unfiltered lists too. :)

Zhiyuan-Amos · 2018-02-13T01:07:59Z

Hurray @yamidark!

…se-edu#792) EditCommand and DeleteCommand stores the target index of the person to edit or delete respectively. As a result, redoing of these index-based commands will be executed on the incorrect person due to different indexing in differing views. Let's update EditCommand and DeleteCommand to find and store the personToEdit or personToDelete respectively before it's first execution. This ensures that future execution of the same command will be on the correct person.

CanIHasReview-bot added the s.ToReview label Jan 10, 2018

Zhiyuan-Amos reviewed Jan 10, 2018

View reviewed changes

Zhiyuan-Amos added s.Ongoing and removed s.ToReview labels Jan 10, 2018

yamidark force-pushed the 737-undo-redo-incorrect-filtered-list branch from 1a2c273 to 449969b Compare January 14, 2018 04:04

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Jan 14, 2018

yamidark force-pushed the 737-undo-redo-incorrect-filtered-list branch 2 times, most recently from 3608440 to fe40136 Compare January 14, 2018 16:47

eugenepeh suggested changes Jan 15, 2018

View reviewed changes

Zhiyuan-Amos reviewed Jan 15, 2018

View reviewed changes

eugenepeh reviewed Jan 15, 2018

View reviewed changes

Zhiyuan-Amos reviewed Jan 15, 2018

View reviewed changes

CanIHasReview-bot added s.ToReview and removed s.ToMerge labels Feb 11, 2018

se-edu deleted a comment Feb 11, 2018

Zhiyuan-Amos added s.ToMerge and removed s.ToReview labels Feb 11, 2018

Zhiyuan-Amos mentioned this pull request Feb 11, 2018

Rebase PRs used in suggested programming tasks #784

Closed

yamidark added 3 commits February 11, 2018 20:57

yamidark force-pushed the 737-undo-redo-incorrect-filtered-list branch from 2051721 to 4be7dd7 Compare February 11, 2018 12:58

CanIHasReview-bot added s.ToReview and removed s.ToMerge labels Feb 11, 2018

Zhiyuan-Amos added s.ToMerge and removed s.ToReview labels Feb 11, 2018

Zhiyuan-Amos merged commit ad1931d into se-edu:master Feb 13, 2018

yamidark deleted the 737-undo-redo-incorrect-filtered-list branch February 13, 2018 03:22

yamidark mentioned this pull request Feb 13, 2018

RedoCommandTest: remove bypassing of encapsulation #828

Closed

yamgent mentioned this pull request Feb 13, 2018

Implement remark command #599

Closed

yamidark mentioned this pull request Feb 21, 2018

AddressBook: remove master tag list #753 #825

Merged

yamidark mentioned this pull request Apr 11, 2018

Decouple UndoableCommand from ReadOnlyAddressBook #670 #861

Merged

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

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

Conversation

yamidark commented Jan 10, 2018 • edited Loading

CanIHasReview-bot commented Jan 10, 2018

v1

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Zhiyuan-Amos commented Jan 10, 2018 • edited Loading

yamidark commented Jan 11, 2018

Zhiyuan-Amos commented Jan 11, 2018

yamidark commented Jan 11, 2018

Zhiyuan-Amos commented Jan 11, 2018 • edited Loading

pyokagan commented Jan 12, 2018

Zhiyuan-Amos commented Jan 12, 2018

CanIHasReview-bot commented Jan 14, 2018

v2

Zhiyuan-Amos commented Jan 14, 2018

CanIHasReview-bot commented Jan 14, 2018

v3

eugenepeh left a comment • edited Loading

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

eugenepeh Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos commented Jan 15, 2018

eugenepeh commented Jan 15, 2018 • edited Loading

Zhiyuan-Amos commented Jan 15, 2018

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamidark commented Jan 15, 2018

Zhiyuan-Amos commented Jan 15, 2018 • edited Loading

yamidark commented Jan 15, 2018

Zhiyuan-Amos commented Feb 11, 2018

CanIHasReview-bot commented Feb 11, 2018

v18

eugenepeh commented Feb 11, 2018 • edited Loading

Zhiyuan-Amos commented Feb 11, 2018

eugenepeh commented Feb 12, 2018 • edited Loading

yamidark commented Feb 12, 2018

eugenepeh commented Feb 12, 2018

Zhiyuan-Amos commented Feb 12, 2018

Zhiyuan-Amos commented Feb 13, 2018

yamidark commented Jan 10, 2018 •

edited

Loading

Zhiyuan-Amos commented Jan 10, 2018 •

edited

Loading

Zhiyuan-Amos commented Jan 11, 2018 •

edited

Loading

eugenepeh left a comment •

edited

Loading

Zhiyuan-Amos Jan 15, 2018 •

edited

Loading

eugenepeh Jan 15, 2018 •

edited

Loading

eugenepeh commented Jan 15, 2018 •

edited

Loading

Zhiyuan-Amos commented Jan 15, 2018 •

edited

Loading

eugenepeh commented Feb 11, 2018 •

edited

Loading

eugenepeh commented Feb 12, 2018 •

edited

Loading