HelpWindow: May interfere with UI Components while active #727 #815

vivekscl · 2018-01-30T06:07:46Z

Fixes #727

On a side note, should this TODO be converted into a separate issue or is it already covered by #551?

/**
 * TODO: This test is incomplete as it is missing test cases.
 */
public class HelpCommandSystemTest extends AddressBookSystemTest {

CanIHasReview-bot · 2018-01-30T06:12:30Z

v1

@vivekscl submitted v1 for review.

[v1 1/1] HelpWindow: modify HelpWindow#show() to use Stage#show()

(📚 Archive)

Checkout this PR version locally

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

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

yamgent

The fix itself LGTM.

@Zhiyuan-Amos Do you think we should add a regression test for this?

pyokagan · 2018-01-30T13:50:22Z

According to the JavaFX documentation for Stage, showAndWait() should
be used if there is a need to block the caller until the modal stage is
closed. In this case, the caller should not be blocked as other UI
components have to function properly even after the help window is
opened.

I'm wondering though -- usually when the event loop is blocked (e.g. you put an infinite while loop into an event handler), the whole application would not respond at all.

The situation here is more peculiar though, since the UI components do seem to respond to input. e.g. The command box actually does allow us to input commands, and in fact does run them.

So, I'm wondering if there is a more in-depth explanation for these particularities.

(As for why I am requesting this, to quote one of the TEAMMATES principles: "We know what we are doing: For us, it is not enough to know something is broken, we should also know why it is broken. It is not enough to get something working, we should know how we got it to work.") ;-)

vivekscl · 2018-01-31T04:33:18Z

According to the JavaFX documentation on Stage#showAndWait():
Shows this stage and waits for it to be hidden (closed) before returning to the caller. This method temporarily blocks processing of the current event, and starts a nested event loop to handle other events. This method must be called on the FX Application thread.

The current event in this case would be the command box area since that's where the event was called from, only that area is blocked while the other events work fine as the other events are in a nested event loop. While the command box is blocked, the commands run fine and UI changes accordingly everywhere except at the command box.

On the other hand, the Stage#show() only sets the visibility of the stage in question to true and doesn't block any events, so it is the preferred method in this case.

Zhiyuan-Amos · 2018-02-01T01:53:26Z

On a side note, should this TODO be converted into a separate issue or is it already covered by #551?

Yup. That's why it remains unchecked in #551. :p

The commit message can be clearer. In the 2nd paragraph, you should state what you have mentioned above - Only when you type help in Command Box then the Command Box will not execute any commands because the processing of the command is blocked, whereas if you press F1 etc, the execution of further commands aren't blocked. So, you need to be more detailed in your commit message. You can take a look at Paul's commit messages here to see how to structure longer commit messages.

@Zhiyuan-Amos Do you think we should add a regression test for this?

Yup I think we can add a test in HelpCommandSystemTest, such that when you execute help command, you are still able to execute another simple command such as ExitCommand

pyokagan · 2018-02-01T02:19:55Z

@vivekscl

The current event in this case would be the command box area since that's where the event was called from, only that area is blocked while the other events work fine as the other events are in a nested event loop. While the command box is blocked, the commands run fine and UI changes accordingly everywhere except at the command box.

Hmm? According to my testing, other UI elements do not seem to update. e.g. try adding a person while the help window is opened. The status bar should update to reflect the fact that the address book was modified, but it does not (!?)

Zhiyuan-Amos · 2018-02-01T02:28:35Z

Hmm? According to my testing, other UI elements do not seem to update. e.g. try adding a person while the help window is opened. The status bar should update to reflect the fact that the address book was modified, but it does not (!?)

Yup just tested too.

If you use F1, command execution will update all UI components.
If you execute help, command execution will only update PersonListPanel. It seems that StatusBarFooter, ResultDisplay and BrowserPanel do not update because additional events are raised. Whereas PersonListPanel is updated because it is being binded to the Model; don't think there's any events raised to update the PersonListPanel.

pyokagan · 2018-02-01T02:33:13Z

Hint: The use of showAndWait(), by itself, isn't what is causing the problem (i.e. A vanilla JavaFX project will likely not have this problem), so the commit message is inaccurate. As the JavaFX mentions, showAndWait() starts a nested event loop precisely to allow other events to run including the same event type of the caller that was blocked. Hence, why the command box still allows us to run commands when the help window is open.

What actually happens is the use of showAndWait() plus some other factor in our code base. Of course, if we have factor A and B which causes the problem, if we remove factor A of course the problem disappears. But factor B still remains, and may combine with another factor to cause us trouble again in the future, which is why it needs to be properly investigated and mentioned in the commit message rather than being handwaved away.

pyokagan

I'll be eagerly awaiting the results of your investigation in the commit message :-)

vivekscl · 2018-02-01T12:11:35Z

Since it seems like the investigation will take a while, I'll post my progress here in the meantime.
After tracing the program execution after typing the help command, it gets stuck at Dispatcher.java where it enters the dispatchEvent method located beside the black arrow. After that, it enters a nested event loop in Stage#showAndWait(). As such it doesn't return back and the while loop doesn't terminate. If the while loop doesn't terminate, the dispatching.remove() method isn't called resulting in the if condition, !dispatching.get() to be false. If that is false, subsequent events will not be able to enter the while loop and be dispatched to their respective subscribers. Furthermore, thequeueForThread keeps increasing in size as it just collects the subsequent events as seen by the "size = 3" note at the side after attempting to delete a person. After closing the help window, the execution continues from the while loop, causing a mass execution of all the events that were in the queue.

I'm still finding that factor B. Hopefully I'll arrive at it soon 😄

Progress Part 2:
So the reason why clicking F1 or clicking Help from the help menu doesn't result in any interference with the UI components is because they don't post an event. Instead, they immediately call MainWindow#handleHelp(). Since no event is posted, the queue doesn't get stuck and as such, subsequent events work properly.

So in conclusion, that means the factor B is actually this part of the code?

But since we replace Stage#showAndWait() with Stage#show(), there is no entering of a nested loop and hence no waiting at the while loop. So Dispatcher.java dispatches events to the subscribers properly and hence, there is no need to change this factor B.

Furthermore, we can think about it the other way round. If we remove the ShowHelpRequestEvent and call the MainWindow#handleHelp(), then the queueForThread won't be clogged up by the nested event loop caused by Stage#showAndWait() and there will be no interference with the UI components as seen by pressing F1 instead. So removing factor B instead of factor A can solve the problem too.

CanIHasReview-bot · 2018-02-02T05:55:19Z

v2

@vivekscl submitted v2 for review.

[v2 1/1] HelpWindow: modify HelpWindow#show() to use Stage#show()

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

Checkout this PR version locally

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

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

vivekscl · 2018-02-02T05:57:37Z

@Zhiyuan-Amos Would it be better if the new test that was added was changed to a private method instead?
On a side note, this test fails for Stage#showAndWait() but passes for Stage#show().

Zhiyuan-Amos · 2018-02-02T06:04:46Z

Would it be better if the new test that was added was changed to a private method instead?

@vivekscl yes, you should put it in a separate method. The test should have more rigorous checks though i.e. verify that other components are updated too.

Also, take a look at your commit message on GitHub, there's some minor mistakes :P

Lastly:

At this point, it iterates through the subscribers and dispatches the event to each of
them. However, as Stage#showAndWait() temporarily blocks processing of
the current event and starts a nested event loop to handle other events,
the iteration of the subscribers does not terminate. As a result,
subsequent events are unable to enter the iteration and be dispatched
to their respective subscribers as these events are held up in the
queue.

This part seems a bit confusing. ><

pyokagan · 2018-02-02T13:55:49Z

@vivekscl From your commit message:

However, as Stage#showAndWait() temporarily blocks processing of
the current event and starts a nested event loop to handle other events,
the iteration of the subscribers does not terminate. As a result,
subsequent events are unable to enter the iteration and be dispatched
to their respective subscribers as these events are held up in the
queue

So, the nested event loop started by Stage#showAndWait() does not bother dispatching our events such as ShowHelpRequestEvent, NewResultAvailableEvent etc. However, it must be dispatching some kind of events, otherwise our UI would be completely frozen and we would not, for example, be able to press enter in the command box and run the command (Callbacks like CommandBox#handleCommandInputChanged() still get called). So, why does Stage#showAndWait() choose to dispatch events to callbacks such as CommandBox#handleCommandInputChanged(), and not the subscribers of ShowHelpRequestEvent, NewResultAvailableEvent etc.? In other words, what are those "other events" mentioned in your commit message, and what makes them so special compared to ShowHelpRequestEvent, NewResultAvailableEvent etc.?

pyokagan · 2018-02-02T13:59:42Z

Yes, as @Zhiyuan-Amos mentioned, the commit message (2nd and 3rd paragraphs) contains implementation details which are not important at the level of abstraction that we are looking at this issue from, and thus could be made more succinct.

Also, in the commit message:

https://docs.oracle.com/javacard/3.0.5/api/javacard/framework/service/Dispatcher.html

!???

vivekscl · 2018-02-02T15:14:13Z

@pyokagan

So, why does Stage#showAndWait() choose to dispatch events to callbacks such as CommandBox#handleCommandInputChanged(), and not the subscribers of ShowHelpRequestEvent, NewResultAvailableEvent

Only those with the events @Subscribe annotation are part of the subscriber queue that is being held up.The CommandBox#handleCommandInputChanged() does not use the eventBus to post events and as such, they do not enter the subscriber queue. If they do not enter the queue, they won't be held up and will continue to work.

Also, in the commit message:
https://docs.oracle.com/javacard/3.0.5/api/javacard/framework/service/Dispatcher.html
!???

Oh I thought that might be a useful reference if needed, but maybe it isn't since the implementation details are not that important 😅.

pyokagan · 2018-02-02T15:32:06Z

@vivekscl

The CommandBox#handleCommandInputChanged() does not use the eventBus to post events and as such, they do not enter the subscriber queue. If they do not enter the queue, they won't be held up and will continue to work.

Yes, that's the important point: We have two event systems in the code base -- the JavaFX event system, and the Guava EventBus system. ShowHelpRequestEvent, NewResultAvailableEvent etc are EventBus events, whereas CommandBox#handleCommandInputChanged() is called on a JavaFX event. These systems are not aware of each other, so it makes sense that when Stage#showAndWait() starts a nested JavaFX event loop, it will only dispatch JavaFX events.

So, your commit message will need to be very clear whether it is talking about EventBus events or JavaFX events.

The second piece of the puzzle (Factor B) is that the default Dispatcher used by EventBus, Dispatcher.perThreadDispatchQueue(), does not allow nested callback invocations, and instead chooses to queue them to be invoked after the current invoked callback completes. This could be fixed by using the Dispatcher.immediate() dispatcher, for example.

Oh I thought that might be a useful reference if needed, but maybe it isn't since the implementation details are not that important 😅.

No, the documentation linked is not Guava docs. I think you are looking for https://github.com/google/guava/blob/master/guava/src/com/google/common/eventbus/Dispatcher.java

pyokagan

Changes requested.

CanIHasReview-bot · 2018-02-04T08:16:56Z

v3

@vivekscl submitted v3 for review.

[v3 1/1] HelpWindow: modify HelpWindow#show() to use Stage#show()

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

Checkout this PR version locally

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

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

yamgent

Changes requested

CanIHasReview-bot · 2018-02-11T08:57:42Z

v9

@vivekscl submitted v9 for review.

[v9 1/1] HelpWindow: modify HelpWindow#show() to use Stage#show()

(📚 Archive) (📈 Interdiff between v8 and v9)

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2018-02-11T09:26:20Z

src/test/java/systemtests/HelpCommandSystemTest.java

+        executeCommand(DeleteCommand.COMMAND_WORD + " " + INDEX_FIRST_PERSON.getOneBased());
+        assertFalse(getStatusBarFooter().getSyncStatus().equals(StatusBarFooter.SYNC_STATUS_INITIAL));
+
+        //TODO: More tests to be added.


I wonder what other meaningful tests can we write though... :X

I think we can still test with other commands. For instance, after exiting the AB4, the help window still remains open. We can still test for that case.

executeCommand(HelpCommand.COMMAND_WORD); executeCommand(ExitCommand.COMMAND_WORD); assertHelpWindowOpen();

LOL that's an interesting interaction. This doesn't seem like what we want though. Can you raise an issue for it?

Edit: It doesn't work on my side; the help window closes. But yes, you are right that there are still additional tests to be written. Can you write this test after this PR? ^^

LOL that's an interesting interaction

Hahaha, I thought the same too. Wasn't sure if it was intended or bug at the time though. But yes, I'll raise an issue for it:+1:.

~~Can you fix that after this PR? ^^~~

~~Fix the help window issue or the additional test cases? 😅~~

Can you write this test after this PR? ^^

Alright 👍

Take note that I edited my comment; the help window closes actually :P Can you verify it again?

Fix the help window issue or the additional test cases?

I can't think of any other test cases now apart from the interaction with ExitCommand. If you can think of others, then you can add them in as well.

executeCommand(HelpCommand.COMMAND_WORD);
executeCommand(ExitCommand.COMMAND_WORD);
assertHelpWindowOpen();

I did verify it multiple times. Moreover, the test mentioned above actually passed in my case. So maybe its an operating system thing? I'm currently running Linux.

Hmm yup perhaps. I'm on MacOS. :)

So it does close but only when it is not in fullscreen mode. Since my default resolution was fullscreen I kept verifying that it didn't close. But once I changed the resolution to something smaller, the help window closes as expected. Maybe you can test it and see if it's the same for MacOS? I can update the issue I just raised if its verified 😄.

I did verify it multiple times. Moreover, the test mentioned above actually passed in my case. So maybe its an operating system thing? I'm currently running Linux.

So please do ignore my previous comment 😅.

//TODO: More tests to be added.

As per discussion above we could create issues for it. The comment doesn't seem to be really informative otherwise :P (and I am of the view that TODOs comments should only exist in the repo if there's a serious flaw in the codebase).

CanIHasReview-bot · 2018-02-12T06:59:56Z

v10

@vivekscl submitted v10 for review.

[v10 1/1] HelpWindow: modify HelpWindow#show() to use Stage#show()

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

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2018-02-12T09:21:17Z

@yamgent for your approval :)

pyokagan · 2018-02-13T14:19:03Z

src/test/java/systemtests/HelpCommandSystemTest.java

+        executeCommand(SelectCommand.COMMAND_WORD + " " + INDEX_FIRST_PERSON.getOneBased());
+        assertEquals("", getCommandBox().getInput());
+        assertCommandBoxShowsDefaultStyle();
+        assertNotEquals(getResultDisplay().getText(), HelpCommand.SHOWING_HELP_MESSAGE);


Should the first argument be HelpCommand.SHOWING_HELP_MESSAGE instead, since getResultDisplay().getText() is the "actual" value that we are testing?

pyokagan · 2018-02-13T14:19:13Z

src/test/java/systemtests/HelpCommandSystemTest.java

+        assertEquals("", getCommandBox().getInput());
+        assertCommandBoxShowsDefaultStyle();
+        assertNotEquals(getResultDisplay().getText(), HelpCommand.SHOWING_HELP_MESSAGE);
+        assertFalse(getBrowserPanel().getLoadedUrl().equals(BrowserPanel.DEFAULT_PAGE));


Why not use assertNotEquals()?

pyokagan · 2018-02-13T14:19:43Z

src/test/java/systemtests/HelpCommandSystemTest.java

+        // assert that the status bar too is updated correctly while the help window is open
+        // note: the select command tested above does not update the status bar
+        executeCommand(DeleteCommand.COMMAND_WORD + " " + INDEX_FIRST_PERSON.getOneBased());
+        assertFalse(getStatusBarFooter().getSyncStatus().equals(StatusBarFooter.SYNC_STATUS_INITIAL));


Why not use assertNotEquals()?

opened by typing help in the command box. For instance, the command box does not display the result message of subsequent commands. However, this does not occur when opening the help window by pressing F1 or pressing help from the help menu. This occurs due to a combination of using the default Dispatcher used by the Guava EventBus system, Dispatcher#perThreadDispatchQueue(), along with calling Stage#showAndWait(). Calling Stage#showAndWait(), starts a nested event loop to handle subsequent JavaFX events. However, the default Dispatcher used by the EventBus does not allow nested callback invocations, and instead queues them to be invoked after the current invoked callback, the MainWindow#handleShowHelpEvent() completes. As such, EventBus events are queued up in the meantime resulting in the subscribers of these events having to wait until the nested event loop is terminated (i.e. when the callback completes, after the help window is closed.) And so, the UI components will only receive these events and therefore update after the help window is closed. However, in the case of pressing F1 or pressing help from the help menu, MainWindow#handleHelp() is called immediately without posting the MainWindow#handleShowHelpEvent(). Since this event never enters the EventBus's queue, the nested event loop started by Stage#showAndWait() does not block it and subsequent EventBus events get dispatched as usual. Let's replace Stage#showAndWait() with Stage#show(), so that there will be no entering of a nested loop and hence no blockage of the Guava EventBus system. Likewise, this could be fixed by using the Dispatcher#immediate() dispatcher instead of the default dispatcher used by the Guava EventBus system. However, replacing Stage#showAndWait() with Stage#show() is more suitable in this case as it is sufficient to show the help window instead of starting a nested event loop to handle subsequent JavaFX events while waiting for the help window to be closed. JavaFX documentation for Stage: https://docs.oracle.com/javase/8/javafx/api/javafx/stage/Stage.html Code and documentation for the Guava Dispatcher: https://github.com/google/guava/blob/master/guava/src/com/google/common/eventbus/Dispatcher.java More information on the Guava EventBus system: https://github.com/google/guava/wiki/EventBusExplained

CanIHasReview-bot · 2018-02-13T15:47:21Z

v11

@vivekscl submitted v11 for review.

[v11 1/1] HelpWindow: modify HelpWindow#show() to use Stage#show()

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

Checkout this PR version locally

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

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

damithc · 2018-02-14T02:43:22Z

@Zhiyuan-Amos let's merge this as we have enough reviews and we need to release level 4 for this sem soon.

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

yamgent reviewed Jan 30, 2018

View reviewed changes

pyokagan suggested changes Feb 1, 2018

View reviewed changes

pyokagan added s.Ongoing and removed s.ToReview labels Feb 1, 2018

vivekscl force-pushed the 727-helpwindow-interferes-with-ui branch from 27a2e9d to 89ad167 Compare February 2, 2018 05:49

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Feb 2, 2018

CanIHasReview-bot requested a review from pyokagan February 2, 2018 05:55

pyokagan suggested changes Feb 2, 2018

View reviewed changes

pyokagan added s.Ongoing and removed s.ToReview labels Feb 2, 2018

vivekscl force-pushed the 727-helpwindow-interferes-with-ui branch from 89ad167 to a229cae Compare February 4, 2018 08:11

CanIHasReview-bot removed the s.Ongoing label Feb 4, 2018

yamgent suggested changes Feb 11, 2018

View reviewed changes

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

vivekscl force-pushed the 727-helpwindow-interferes-with-ui branch 2 times, most recently from e887637 to 6e67aa8 Compare February 11, 2018 08:50

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

CanIHasReview-bot requested a review from yamgent February 11, 2018 08:57

Zhiyuan-Amos reviewed Feb 11, 2018

View reviewed changes

vivekscl force-pushed the 727-helpwindow-interferes-with-ui branch from 6e67aa8 to 646a8fb Compare February 12, 2018 06:40

yamgent approved these changes Feb 13, 2018

View reviewed changes

pyokagan suggested changes Feb 13, 2018

View reviewed changes

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

vivekscl force-pushed the 727-helpwindow-interferes-with-ui branch from 646a8fb to 073e935 Compare February 13, 2018 15:29

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Feb 13, 2018

CanIHasReview-bot requested a review from pyokagan February 13, 2018 15:47

pyokagan approved these changes Feb 14, 2018

View reviewed changes

Zhiyuan-Amos requested a review from damithc February 14, 2018 02:37

damithc approved these changes Feb 14, 2018

View reviewed changes

eugenepeh added s.ToMerge and removed s.ToReview labels Feb 14, 2018

Zhiyuan-Amos merged commit 433390f into se-edu:master Feb 14, 2018

HelpWindow: May interfere with UI Components while active #727 #815

HelpWindow: May interfere with UI Components while active #727 #815

Conversation

vivekscl commented Jan 30, 2018

CanIHasReview-bot commented Jan 30, 2018

v1

yamgent left a comment

Choose a reason for hiding this comment

pyokagan commented Jan 30, 2018

vivekscl commented Jan 31, 2018

Zhiyuan-Amos commented Feb 1, 2018

pyokagan commented Feb 1, 2018

Zhiyuan-Amos commented Feb 1, 2018

pyokagan commented Feb 1, 2018

pyokagan left a comment

Choose a reason for hiding this comment

vivekscl commented Feb 1, 2018 • edited

CanIHasReview-bot commented Feb 2, 2018

v2

vivekscl commented Feb 2, 2018

Zhiyuan-Amos commented Feb 2, 2018

pyokagan commented Feb 2, 2018

pyokagan commented Feb 2, 2018

vivekscl commented Feb 2, 2018

pyokagan commented Feb 2, 2018 • edited

pyokagan left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Feb 4, 2018

v3

yamgent left a comment • edited

Choose a reason for hiding this comment

CanIHasReview-bot commented Feb 11, 2018

v9

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos Feb 11, 2018 • edited

Choose a reason for hiding this comment

vivekscl Feb 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivekscl Feb 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivekscl Feb 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Feb 12, 2018

v10

Zhiyuan-Amos commented Feb 12, 2018

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 13, 2018

v11

damithc commented Feb 14, 2018

vivekscl commented Feb 1, 2018 •

edited

pyokagan commented Feb 2, 2018 •

edited

yamgent left a comment •

edited

Zhiyuan-Amos Feb 11, 2018 •

edited

vivekscl Feb 11, 2018 •

edited

vivekscl Feb 11, 2018 •

edited

vivekscl Feb 11, 2018 •

edited