PersonListPanel#personListView: revert type change #796 #818

Zhiyuan-Amos · 2018-02-04T00:43:53Z

Fixes #796.

personListView is of type ListView<PersonCard>. We use EasyBind library
to map the Person objects to PersonCard objects. The mapping / applying
of Function<? super F, ? extends E> occurs only when either get(int) or
getRemoved() methods are called[1]; the creation of PersonCard objects
occur only when either of these methods are called.

The ClearCommand deletes all Person objects. When we have large number
of persons and getRemoved() is called, all the corresponding cards
will be created prior to them being removed. As creation of PersonCard
objects are costly, the app slows down tremendously.

Let's revert ListView<PersonCard> to ListView<Person>.
PersonListViewCell will create the PersonCard object. According to
javadoc for Cell that "it is not feasible to create an actual Cell for
every single item in the control. We represent extremely large data
sets using only very few Cells. Each Cell is "recycled", or reused."[2]
A Cell is created for every row in the ListView that is displayed on
the scene i.e. that is visible to the user, and additional Cells may be
created for efficient scroll handling.[3] This implies that there will
only be about 10 Cells at any point in time since the default screen
size is only large enough to display 10 Cells. Therefore, there will
only be about 10 PersonCard objects at any point. Creation of 10
PersonCard objects is performed in negligible time, thus the app no
longer slows down. This commit is performed by reverting from
75cbfa3.

Also, since PersonCard objects are only created when the card is
displayed on the list view, let's teach
GuiTestAssert#assertListMatching(PersonListPanelHandle, Person...) and
AddressBookSystemTest#assertSelectedCardChange(Index) to navigate to
the card that they are verifying before performing the verification.

[1]: https://github.com/TomasMikula/EasyBind/blob/master/src/main/java/org/fxmisc/easybind/MappedList.java
[2]: https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/Cell.html
[3]: https://stackoverflow.com/questions/18159303/listview-in-javafx-adds-multiple-cells

Zhiyuan-Amos · 2018-02-04T00:53:00Z

src/test/java/seedu/address/ui/PersonListPanelTest.java

-            assertCardDisplaysPerson(expectedPerson, actualCard);
-            assertEquals(Integer.toString(i + 1) + ". ", actualCard.getId());
+            assertEquals(expectedPerson, actualPerson);
+            // Because we do not have the actual card, we cannot compare the index on the card


This is a side effect of changing from ListView<PersonCard> to ListView<Person>; we cannot get the actual PersonCard and the corresponding PersonCardHandle :/ Not quite sure how to fix this though. Prior to 75cbfa3, the following code is being used to retrieve the PersonCard nodes.

private Set<Node> getAllCardNodes() { return guiRobot.lookup(CARD_PANE_ID).queryAll(); }

However, this code doesn't really get all the card nodes, because the PersonCard objects are only created when required; if we have 9999 PersonCard objects and the ListView currently displays from indices 1 to 5, then only the first 5 PersonCard objects are created. As such, we cannot get the 1000th PersonCard node, for example. So in this case, there's dependency on line 37 (personListPanelHandle.navigateToCard(TYPICAL_PERSONS.get(i))): By navigating to the card, you ensure that the PersonCard object is created, so you are able to find it using getAllCardNodes().

Not sure if there's a better way to do this.

@yamgent need your thought on this too :)

As spoken, I guess the dependency is necessary, otherwise the person card verification is nonsense. :P

CanIHasReview-bot · 2018-02-04T00:54:28Z

v1

@Zhiyuan-Amos submitted v1 for review.

[v1 1/3] PersonListPanel#personListView: revert type change
[v1 2/3] PersonCard#person: update visibility to private
[v1 3/3] Gradle: remove EasyBind dependency

(📚 Archive)

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2018-02-04T02:23:25Z

Ah, forgot to add regression tests.

CanIHasReview-bot · 2018-02-04T10:14:08Z

v2

@Zhiyuan-Amos submitted v2 for review.

[v2 1/3] PersonListPanel#personListView: revert type change
[v2 2/3] PersonCard#person: update visibility to private
[v2 3/3] Gradle: remove EasyBind dependency

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

Checkout this PR version locally

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

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

yamgent · 2018-02-05T11:12:33Z

When I first saw the number of line changes, I had a shock of my life. :P

I am not sure whether we should explore generating 9999persons.xml on the fly locally, rather than uploading the big file to the repository...

yamgent · 2018-02-06T03:03:08Z

Btw is the revert based on a certain commit? (i.e. you did a git revert <sha>)

Zhiyuan-Amos · 2018-02-06T05:06:16Z

When I first saw the number of line changes, I had a shock of my life. :P
I am not sure whether we should explore generating 9999persons.xml on the fly locally, rather than uploading the big file to the repository...

Oh yes you are right. In DeveloperGuide we have this:

Open a console and run the command gradlew processResources (Mac/Linux: ./gradlew processResources). It should finish with the BUILD SUCCESSFUL message.
This will generate all resources required by the application and tests.

So we should generate the .xml file as part of processResources?

Btw is the revert based on a certain commit? (i.e. you did a git revert )

Yup it's based on a certain commit. Do I need to mention it as part of commit message? Cos there were additional changes apart from just reverting.

yamgent · 2018-02-08T04:47:07Z

Yup it's based on a certain commit. Do I need to mention it as part of commit message? Cos there were additional changes apart from just reverting.

As spoken, yes, you might also want to mention the additional changes made after reverting the commit, so that we can verify and understand your flow.

Zhiyuan-Amos · 2018-02-08T04:47:08Z

I am not sure whether we should explore generating 9999persons.xml on the fly locally, rather than uploading the big file to the repository...

Since it's 1MB, I wonder if it's considered as too big and whether we can just upload it to GitHub :P

yamgent

Minor commit message amendements:

[2/3]

PersonCard#person is no longer accessed by the world.
no longer accessed by anyone

[3/3]

Let’s remove it. Also, let’s update README to reflect this change.
... update README**.md**...?

pyokagan · 2018-02-08T11:57:49Z

@yamgent

I am not sure whether we should explore generating 9999persons.xml on the fly locally, rather than uploading the big file to the repository...

Yes, imagine what happens when we add a new field to the Person class and need to update all 9999 entries in the test file. :-)

CanIHasReview-bot · 2018-02-08T13:49:48Z

v3

@Zhiyuan-Amos submitted v3 for review.

[v3 1/3] PersonListPanelHandle#navigateToCard(Person): update logic
[v3 2/3] PersonListPanel#personListView: revert type change
[v3 3/3] Gradle: remove EasyBind dependency

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

Checkout this PR version locally

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

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

eugenepeh

[2/3]
Commit message nits:

The ClearCommand deletes all Person objects. getRemoved() is called,
and when we have 9999 persons in the addressbook, all the corresponding
cards are created.
When we have large number of persons and getRemoved() is called, all the corresponding cards were created prior to the deletion.
As creation of PersonCard objects are costly, the
app slows down tremendously.

Does this sounds better?

eugenepeh · 2018-02-12T09:11:08Z

src/test/data/PersonListPanelTest/generate9999persons.py

+    f.write('<email>a@a</email>\n')
+    f.write('<address>a</address>\n')
+    f.write('</persons>\n')
+f.write('</addressbook>\n')


shouldn't this file deserve its own commit prior to this commit?
That way reviewer can compare to the performance before and after as well.

Oh I thought that what was mentioned in the issue is sufficient; the issue describes the problem while the PR resolves the problem? ^^

Hmm okay... then I will have to copy this file to the previous commit to see the improvement >_>

eugenepeh

[1/3]

Here I abit don't get it. Doesn't the selection of the card triggers the selected event?

Zhiyuan-Amos · 2018-02-12T09:26:56Z

Here I abit don't get it. Doesn't the selection of the card triggers the selected event?

Yup the selection of the card will automatically post a card selected event. There's a difference between navigate and select: navigation causes your list view to shift to the card, while select... selects the card. XD

Zhiyuan-Amos · 2018-02-12T09:30:46Z

@eugenepeh I edited your commit message suggestion slightly, ok with you?

The ClearCommand deletes all Person objects. When we have large number of persons and getRemoved() is called, all the corresponding cards ~~were~~ will be created prior to them being removed ~~deletion~~. As creation of PersonCard objects are costly, the app slows down tremendously.

eugenepeh · 2018-02-12T09:35:49Z

Yup the selection of the card will automatically post a card selected event. There's a difference between navigate and select: navigation causes your list view to shift to the card, while select... selects the card. XD

In that case, do we still need the selection change event? o_o

Zhiyuan-Amos · 2018-02-12T09:44:10Z

In that case, do we still need the selection change event? o_o

Oh yes, an event is posted when a card is selected is because of this code in PersonListPanel:

addressbook-level4/src/main/java/seedu/address/ui/PersonListPanel.java

Lines 44 to 52 in eaa0cfb

    
           private void setEventHandlerForSelectionChangeEvent() { 
        
               personListView.getSelectionModel().selectedItemProperty() 
        
                       .addListener((observable, oldValue, newValue) -> { 
        
                           if (newValue != null) { 
        
                               logger.fine("Selection in person list panel changed to : '" + newValue + "'"); 
        
                               raise(new PersonPanelSelectionChangedEvent(newValue)); 
        
                           } 
        
                       }); 
        
           }

The reason why I didn't want navigateToCard(Person) to select the card is that in [2/3], this method will be called at the start of the system tests to navigate through all the cards (see paragraph 4 of [2/3]). If I select the card during navigation, then it causes the BrowserPanel to load someone's page, which causes the default state of the app to be modified.

So that's why I need commit [1/3]. However, I chose not to put the above reason as part of commit message in [1/3] because there's no need to do so, since there's sufficient reasons for removing it on its own already.

eugenepeh · 2018-02-12T09:48:04Z

The reason why I didn't want navigateToCard(Person) to select the card is that in [2/3], this method will be called at the start of the system tests to navigate through all the cards (see paragraph 4 of [2/3]). If I select the card during navigation, then it causes the BrowserPanel to load someone's page, which causes the default state of the app to be modified.

Hmm okay... but in that case, the selectionchanged will be redundant now isn't it? Is there any other methods that cause selectionchanged other than user click on the card itself?

Zhiyuan-Amos · 2018-02-12T10:10:40Z

Hmm okay... but in that case, the selectionchanged will be redundant now isn't it? Is there any other methods that cause selectionchanged other than user click on the card itself?

As in whether we need this class PersonPanelSelectionChangedEvent? We still need it because BrowserPanel catches this event to load the person :P

The following code is called when user executes SelectCommand :P

addressbook-level4/src/main/java/seedu/address/ui/PersonListPanel.java

Lines 57 to 62 in eaa0cfb

    
           private void scrollTo(int index) { 
        
               Platform.runLater(() -> { 
        
                   personListView.scrollTo(index); 
        
                   personListView.getSelectionModel().clearAndSelect(index); 
        
               }); 
        
           }

yamgent · 2018-02-13T11:29:22Z

Sorry I missed out on this PR, go ahead and make the changes first.

Zhiyuan-Amos · 2018-02-13T14:50:53Z

@eugenepeh

[1/3] Here I abit don't get it. Doesn't the selection of the card triggers the selected event?

Should I still update [1/3] commit message? ^^

CanIHasReview-bot · 2018-03-17T14:32:29Z

v11

@Zhiyuan-Amos submitted v11 for review.

[v11 1/3] PersonListPanelHandle#navigateToCard(Person): update logic
[v11 2/3] PersonListPanel#personListView: revert type change
[v11 3/3] Gradle: remove EasyBind dependency

(📚 Archive) (📈 Interdiff between v10 and v11)

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2018-03-17T14:37:21Z

Edit: I increased the test timings as the performance test was sometimes failing on Travis.

CanIHasReview-bot · 2018-03-17T14:42:58Z

v12

@Zhiyuan-Amos submitted v12 for review.

[v12 1/3] PersonListPanelHandle#navigateToCard(Person): update logic
[v12 2/3] PersonListPanel#personListView: revert type change
[v12 3/3] Gradle: remove EasyBind dependency

(📚 Archive) (📈 Interdiff between v11 and v12)

Checkout this PR version locally

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

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

yamgent

No other objections.

Zhiyuan-Amos · 2018-03-18T00:59:38Z

@damithc for your review again as I've made quite a bit of changes since your previous approval.

damithc

A cosmetic nit. Can merge after that.

damithc · 2018-03-18T02:52:23Z

src/test/java/seedu/address/ui/PersonListPanelTest.java

+     * 2. If {@code TestTimedOutException} is thrown after the logs are printed, then the failure occurs during the
+     * creation and deletion of the person cards.
+     */
+    // Causes test to fail-fast; guiRobot#interact(Runnable) only returns after the Runnable completes execution.


The comment can be clearer: what causes test to fail fast? the timeout? e.g.,

The timeout makes the test fail fast. Without it, the test can stall for a long time because ...

Also, I'm not sure if javadoc tools allows normal comments between javadoc and the method.

Also, I'm not sure if javadoc tools allows normal comments between javadoc and the method.

Yup, verified that having normal comments between javadoc and the method won't interfere with the generation of the javadoc.

Also, @damithc I've updated the header comments with the html tags to format it to look as above, otherwise it will be in a single paragraph. Is it ok?

Edit: ignore the double 1. 1. and 2. 2., I've fixed the header comments to display only a single 1. and 2..

Looks good 👍

damithc · 2018-03-18T12:52:20Z

src/test/java/seedu/address/ui/PersonListPanelTest.java

+     *     <li>
+     *         If {@code TestTimedOutException} is thrown before the logs are printed, then the failure occurs during
+     *         the preparation of the test (creating the xml file containing the large number of persons).
+     *     </li>


Like this will work too (fewer lines needed):

* <li>If {@code TestTimedOutException} is thrown before the logs are printed, then the failure occurs during * the preparation of the test (creating the xml file containing the large number of persons).</li>

CanIHasReview-bot · 2018-03-18T13:38:11Z

v13

@Zhiyuan-Amos submitted v13 for review.

[v13 1/3] PersonListPanelHandle#navigateToCard(Person): update logic
[v13 2/3] PersonListPanel#personListView: revert type change
[v13 3/3] Gradle: remove EasyBind dependency

(📚 Archive) (📈 Interdiff between v12 and v13)

Checkout this PR version locally

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

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

navigateToCard(Person) navigates to the card containing the person passed in as parameter, and selects the card. The selection of the card is an unexpected behaviour as it is not clear from the naming of the method. Also, there’s no need for this method to select the card. Let’s update this method to no longer select the card that it navigates to.

personListView is of type ListView<PersonCard>. We use EasyBind library to map the Person objects to PersonCard objects. The mapping / applying of Function<? super F, ? extends E> occurs only when either get(int) or getRemoved() methods are called[1]; the creation of PersonCard objects occur only when either of these methods are called. The ClearCommand deletes all Person objects. When we have large number of persons and getRemoved() is called, all the corresponding cards will be created prior to them being removed. As creation of PersonCard objects are costly, the app slows down tremendously. Let's revert ListView<PersonCard> to ListView<Person>. PersonListViewCell will create the PersonCard object. According to javadoc for Cell that "it is not feasible to create an actual Cell for every single item in the control. We represent extremely large data sets using only very few Cells. Each Cell is "recycled", or reused."[2] A Cell is created for every row in the ListView that is displayed on the scene i.e. that is visible to the user, and additional Cells may be created for efficient scroll handling.[3] This implies that there will only be about 10 Cells at any point in time since the default screen size is only large enough to display 10 Cells. Therefore, there will only be about 10 PersonCard objects at any point. Creation of 10 PersonCard objects is performed in negligible time, thus the app no longer slows down. This commit is performed by reverting from 75cbfa3. Also, since PersonCard objects are only created when the card is displayed on the list view, let's teach GuiTestAssert#assertListMatching(PersonListPanelHandle, Person...) and AddressBookSystemTest#assertSelectedCardChange(Index) to navigate to the card that they are verifying before performing the verification. [1]: https://github.com/TomasMikula/EasyBind/blob/master/src/main/java/org/fxmisc/easybind/MappedList.java [2]: https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/Cell.html [3]: https://stackoverflow.com/questions/18159303/listview-in-javafx-adds-multiple-cells

EasyBind is no longer used in our code base. Let’s remove it. Also, let’s update README.md to reflect this change.

CanIHasReview-bot · 2018-03-19T13:02:53Z

v14

@Zhiyuan-Amos submitted v14 for review.

[v14 1/3] PersonListPanelHandle#navigateToCard(Person): update logic
[v14 2/3] PersonListPanel#personListView: revert type change
[v14 3/3] Gradle: remove EasyBind dependency

(📚 Archive) (📈 Interdiff between v13 and v14)

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2018-03-19T13:03:43Z

Changelog: Rebased off master after merging #837, otherwise Travis fails.

Zhiyuan-Amos commented Feb 4, 2018

View reviewed changes

CanIHasReview-bot added the s.ToReview label Feb 4, 2018

Zhiyuan-Amos force-pushed the 796-regression branch from caff5ec to 092ddb4 Compare February 4, 2018 10:02

yamgent self-requested a review February 6, 2018 03:03

yamgent suggested changes Feb 8, 2018

View reviewed changes

Zhiyuan-Amos force-pushed the 796-regression branch 2 times, most recently from 57e1668 to 593723c Compare February 8, 2018 13:44

CanIHasReview-bot requested a review from yamgent February 8, 2018 13:49

eugenepeh reviewed Feb 12, 2018

View reviewed changes

yamgent added s.Ongoing and removed s.ToReview labels Feb 13, 2018

Zhiyuan-Amos force-pushed the 796-regression branch from ee9c2ff to 72a2a11 Compare March 17, 2018 14:27

Zhiyuan-Amos force-pushed the 796-regression branch from 72a2a11 to 8c55051 Compare March 17, 2018 14:36

yamgent approved these changes Mar 17, 2018

View reviewed changes

damithc approved these changes Mar 18, 2018

View reviewed changes

Zhiyuan-Amos force-pushed the 796-regression branch from 8c55051 to 283daf8 Compare March 18, 2018 12:31

Zhiyuan-Amos added s.ToMerge and removed s.ToReview labels Mar 18, 2018

Zhiyuan-Amos force-pushed the 796-regression branch from 283daf8 to 933751b Compare March 18, 2018 12:34

damithc reviewed Mar 18, 2018

View reviewed changes

Zhiyuan-Amos force-pushed the 796-regression branch from 933751b to cc4f1d1 Compare March 18, 2018 13:08

CanIHasReview-bot added s.ToReview and removed s.ToMerge labels Mar 18, 2018

Zhiyuan-Amos added s.ToMerge and removed s.ToReview labels Mar 19, 2018

Zhiyuan-Amos added 3 commits March 19, 2018 20:50

Gradle: remove EasyBind dependency

0e9b9d0

EasyBind is no longer used in our code base. Let’s remove it. Also, let’s update README.md to reflect this change.

Zhiyuan-Amos force-pushed the 796-regression branch from cc4f1d1 to 0e9b9d0 Compare March 19, 2018 12:53

CanIHasReview-bot added s.ToReview and removed s.ToMerge labels Mar 19, 2018

Zhiyuan-Amos merged commit a4ef522 into se-edu:master Mar 19, 2018

Zhiyuan-Amos deleted the 796-regression branch March 19, 2018 13:32

Zhiyuan-Amos mentioned this pull request Mar 27, 2018

PersonListPanelTest: fix flaky test #863

Merged

PersonListPanel#personListView: revert type change #796 #818

PersonListPanel#personListView: revert type change #796 #818

Conversation

Zhiyuan-Amos commented Feb 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Feb 4, 2018

v1

Zhiyuan-Amos commented Feb 4, 2018

CanIHasReview-bot commented Feb 4, 2018

v2

yamgent commented Feb 5, 2018

yamgent commented Feb 6, 2018

Zhiyuan-Amos commented Feb 6, 2018

yamgent commented Feb 8, 2018

Zhiyuan-Amos commented Feb 8, 2018

yamgent left a comment

Choose a reason for hiding this comment

pyokagan commented Feb 8, 2018

CanIHasReview-bot commented Feb 8, 2018

v3

eugenepeh left a 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

eugenepeh left a comment • edited

Choose a reason for hiding this comment

Zhiyuan-Amos commented Feb 12, 2018

Zhiyuan-Amos commented Feb 12, 2018

eugenepeh commented Feb 12, 2018 • edited

Zhiyuan-Amos commented Feb 12, 2018

eugenepeh commented Feb 12, 2018

Zhiyuan-Amos commented Feb 12, 2018

yamgent commented Feb 13, 2018

Zhiyuan-Amos commented Feb 13, 2018

CanIHasReview-bot commented Mar 17, 2018

v11

Zhiyuan-Amos commented Mar 17, 2018 • edited

CanIHasReview-bot commented Mar 17, 2018

v12

yamgent left a comment

Choose a reason for hiding this comment

Zhiyuan-Amos commented Mar 18, 2018

damithc 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 Mar 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 18, 2018

v13

CanIHasReview-bot commented Mar 19, 2018

v14

Zhiyuan-Amos commented Mar 19, 2018

Zhiyuan-Amos commented Feb 4, 2018 •

edited

eugenepeh left a comment •

edited

eugenepeh commented Feb 12, 2018 •

edited

Zhiyuan-Amos commented Mar 17, 2018 •

edited

Zhiyuan-Amos Mar 18, 2018 •

edited