HelpWindow: help window stays open even after exiting by clicking the close button #826 #832

yamidark · 2018-02-16T16:22:24Z

Fixes #826

The MainWindow does not have an event handler to handle external
requests (such as the close button or through the taskbar) to close
the MainWindow.

This may cause the app to not close properly since MainApp#stop() is
not called as the ExitAppRequestEvent is not raised. MainApp#stop() is
the method used to call Platform#exit() and System#exit(int) which
closes our app and all remaining open windows. Thus, if MainApp#stop()
is not called, the app may continue to run even after the MainWindow is
closed, causing existing HelpWindows to remain open even when the user
expects the app to stop.

According to the life-cycle section in the documentation on JavaFX
Application[1], the app will only finish and call MainApp#stop() when
the app either calls Platform#exit() or when the last window has been
closed. Thus, should there be any HelpWindows still opened, the app
will continue running, causing those HelpWindows to remain open.

Let's add an event handler for the onCloseRequest event in the root of
MainWindow.fxml. This event will call MainWindow#handleExit(), which
raises an ExitAppRequestEvent, when there is an external request to
close the app.

EmptyMainWindowHandle does not contain a close button, thus to test if
an ExitAppRequestEvent was raised, we fire a
WindowEvent#WINDOW_CLOSE_REQUEST event which will trigger the
onCloseRequest event.

[1] JavaFx documentation for Application:
https://docs.oracle.com/javafx/2/api/javafx/application/Application.html

CanIHasReview-bot · 2018-02-16T16:49:46Z

v1

@yamidark submitted v1 for review.

[v1 1/1] MainWindow: add new method setCloseButtonEvent()

(📚 Archive)

Checkout this PR version locally

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

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

vivekscl

Just a few nits on the commit message:
The title of the commit message could give a better overview instead? Like for example, MainWindow: add an event handler for the close button?

any other external method sounds a bit unclear to me.

This may cause the app to not close properly
To be more specific, only the HelpWindow doesn't close, right?

The 4th paragraph seems to have gone into too much detail such as explaining what handleExit() does.

yamidark · 2018-02-17T02:33:32Z

The title of the commit message could give a better overview instead? Like for example, MainWindow: add an event handler for the close button?

Kind of followed the style I have been using, where the commit title just states what I'm doing, and the other parts of the message explains what it is and why we need it.

any other external method sounds a bit unclear to me.

We technically can close the MainWindow via other means like closing it in the taskbar below which also result in the bug. Not too sure how to phrase this better though, maybe we can just give the 1 example I mentioned earlier?

This may cause the app to not close properly
To be more specific, only the HelpWindow doesn't close, right?

The app itself also doesn't stop running which is the reason why the HelpWindow does not close.
You can try just clicking the close button with HelpWindow open, and the logger on the console does not state that MainApp#stop() is called, only after the HelpWindow is closed does Mainapp#stop() get called.

The 4th paragraph seems to have gone into too much detail such as explaining what handleExit() does

Hmmm, yep probably don't need to go into detail what handleExit() does.

vivekscl · 2018-02-17T02:55:12Z

Kind of followed the style I have been using, where the commit title just states what I'm doing, and the other parts of the message explains what it is and why we need it.

Oh, in that case, it should be fine 👍

We technically can close the MainWindow via other means like closing it in the taskbar below which also result in the bug. Not too sure how to phrase this better though, maybe we can just give the 1 example I mentioned earlier?

You could add the example and phrase it like, There is no event handler for closing the MainWindow via external methods such as the close button or the taskbar.

The app itself also doesn't stop running which is the reason why the HelpWindow does not close.
You can try just clicking the close button with HelpWindow open, and the logger on the console does not state that MainApp#stop() is called, only after the HelpWindow is closed does Mainapp#stop() get called.

Oh, didn't notice that, my bad 😅

yamgent · 2018-02-18T06:58:01Z

src/main/java/seedu/address/ui/MainWindow.java

+     * Sets an event handler for the app to stop when the {@code MainWindow}'s close button is pressed.
+     */
+    private void setCloseButtonEvent() {
+        getPrimaryStage().setOnCloseRequest(event -> {


If some other parts of the application calls getPrimaryStage().close();, it does not trigger OnCloseRequest. Perhaps we should use setOnHiding instead? (Source)

If some other parts of the application calls getPrimaryStage().close();, it does not trigger OnCloseRequest. Perhaps we should use setOnHiding instead?

I wanted to differentiate between closing the app internally (via ExitCommand or the exit menu button) and externally (close button or from taskbar).
If we use setOnHiding, handleExit() actually gets called twice if we close it internally since setOnHiding would call handleExit() for the 2nd time afterMainApp.stop() calls mainWindow.hide() in ui.stop().

yamgent · 2018-02-18T07:02:22Z

Also I wonder whether we should perhaps use fxml to do our deed instead of writing some more code...

diff --git a/src/main/resources/view/MainWindow.fxml b/src/main/resources/view/MainWindow.fxml
index 1dadb95b..daf386d8 100644
--- a/src/main/resources/view/MainWindow.fxml
+++ b/src/main/resources/view/MainWindow.fxml
@@ -12,7 +12,7 @@
 <?import javafx.scene.layout.VBox?>

 <fx:root type="javafx.stage.Stage" xmlns="http://javafx.com/javafx/8" xmlns:fx="http://javafx.com/fxml/1"
-         minWidth="450" minHeight="600">
+         minWidth="450" minHeight="600" onHiding="#handleExit">
   <icons>
     <Image url="@/images/address_book_32.png" />
   </icons>

yamidark · 2018-02-18T15:37:16Z

Also I wonder whether we should perhaps use fxml to do our deed instead of writing some more code...

Wanted to do that too, but didn't know if it could be done and my MainWindow.fxml doesn't seem to open in Scenebuilder. Yep, doing this in the .fxml would definitely be better.

yamgent · 2018-02-19T05:02:40Z

my MainWindow.fxml doesn't seem to open in Scenebuilder

Ah okay. IIRC SceneBuilder has never been working even when I was a cs2103t student, so don't worry about it. :P SceneBuilder has strict requirements on how we structure our UI codes (e.g. controllers) and widgets (e.g. placeholders), so loading problems are probably not unheard of.

Anyway, you can also spot possible attributes in the JavaFX docs under Property Summary as well. onCloseRequest="#handleExit" is probably what you are looking for.

CanIHasReview-bot · 2018-02-19T07:13:41Z

v2

@yamidark submitted v2 for review.

[v2 1/1] MainWindow.fxml: update onCloseRequest event to call handleExit()

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

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2018-02-19T13:47:28Z

Can you call some method in guiRobot to move the cursor to click on the x button to close the window? That way we can write regression tests for it.

@vivekscl do take a look at the commit message and suggest improvements to it. I spotted a few bugs alrd :P

yamidark · 2018-02-19T16:30:52Z

Can you call some method in guiRobot to move the cursor to click on the x button to close the window? That way we can write regression tests for it.

Couldn't find any method to move it to the x button, tried manually shifting the cursor there by using getWidth() , but same problem as closing the window via ExitCommand or the menu item, the test class terminates after it is closed, so I don't think this can be tested :X.

Zhiyuan-Amos · 2018-02-20T01:10:31Z

but same problem as closing the window via ExitCommand or the menu item, the test class terminates after it is closed

Take a look at the 2nd answer and here as well.

It seems like the main thing we are testing for is that whenever we click on the x button, System#exit(int) is called (if I'm reading correctly, the reason why HelpWindow closes when we click on x button is because System#exit(int) is called right?).

As such, I think we need to create a MainAppTest which tests that all forms of exiting the app will result in calling System#exit(int).

yamidark · 2018-02-20T07:40:40Z

Take a look at the 2nd answer and here as well.

Probably should have searched for this System.exit() test :O.

Anyway, I tried this extending of the SecurityManager but the test now hangs instead of terminating after we try any methods to exit the app.
Seems like this is because we call Platform.exit() first.
Can't seem to find a way catch and stop this Platform.exit(), so for this test should we override MainApp.stop() in TestApp to not call Platform.exit() instead?

Zhiyuan-Amos · 2018-02-20T08:10:41Z

Can't seem to find a way catch and stop this Platform.exit(), so for this test should we override MainApp.stop() in TestApp to not call Platform.exit() instead?

I don't think we can override TestApp for this test alone right? I.e. this update will affect all the other tests as well.

Can we try System Rules (also in the link I posted above)?

yamidark · 2018-02-20T08:15:30Z

Can we try System Rules (also in the link I posted above)?

System Rules needs at least JUnit 4.9 while ours is 4.12(?) :X.
*Edit: whoops saw wrong, thought ours was 4.1.2, its 4.12 xD. Will look to see how to include this in.

yamidark · 2018-02-20T08:53:08Z

Just tested the System Rules, since the ExpectedSystemExit also uses an extended SecurityManager, the same problem occurs and the test hangs.

Zhiyuan-Amos · 2018-02-20T09:12:39Z

Hmm ok, document this down as part of the commit message then ^^

This may cause the app to not close properly since it will not raise a
ExitAppRequestEvent, in turn MainApp#stop() is not called and the app
may continue to run.

Commit message needs to be clearer

MainApp#stop() calls System#exit() (or is it Platform#exit()?) which then closes all the remaining windows.
App continues to run -> Causing existing windows to remain open.

CanIHasReview-bot · 2018-02-20T14:32:21Z

v3

@yamidark submitted v3 for review.

[v3 1/1] MainWindow.fxml: update onCloseRequest event to call handleExit()

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

Checkout this PR version locally

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

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

Zhiyuan-Amos

Showing the link to the API of SecurityManager without explaining in the commit message isn't helpful :P
No need for line break for website links, otherwise the hyperlink is unclickable.

let's wait for Vivek to review first.

Zhiyuan-Amos · 2018-02-21T00:49:47Z

Can't seem to find a way catch and stop this Platform.exit(), so for this test should we override MainApp.stop() in TestApp to not call Platform.exit() instead?

If you override the method in TestApp, then you aren't testing the actual class MainApp isn't it :P I.e. you have no guarantee whether MainApp is calling System#exit(int) or not.

Can we do unit test for MainWindow, particularly for the method #handleExit()? We just check that whenever you press x or press the Exit button, this ExitAppRequestEvent is raised.

Ideally, we should have an integration test between MainWindow & MainApp. However, that doesn't seem possible in this scenario.

yamidark · 2018-02-21T03:55:37Z

Showing the link to the API of SecurityManager without explaining in the commit message isn't helpful :P

Hmm, so I should explain how we can use SecurityManager to catch this System.exit(int), and maybe instead of the API link which isn't really useful I can show the link to the stackoverflow post?

No need for line break for website links, otherwise the hyperlink is unclickable.

Whoops, didn't notice that. Will update in next iteration.

If you override the method in TestApp, then you aren't testing the actual class MainApp isn't it :P I.e. you have no guarantee whether MainApp is calling System#exit(int) or not.

Now that you mention this, yep, dunno what I was thinking xD.

Can we do unit test for MainWindow, particularly for the method #handleExit()? We just check that whenever you press x or press the Exit button, this ExitAppRequestEvent is raised.

Will check to see how we can add this test. Should this be done in this PR though?

Zhiyuan-Amos · 2018-02-21T04:04:13Z

Hmm, so I should explain how we can use SecurityManager to catch this System.exit(int), and maybe instead of the API link which isn't really useful I can show the link to the stackoverflow post?

The link to the SO post is more helpful.

Can we do unit test for MainWindow, particularly for the method #handleExit()? We just check that whenever you press x or press the Exit button, this ExitAppRequestEvent is raised.

Will check to see how we can add this test. Should this be done in this PR though?

Yup it should be done in this PR since this PR updates MainWindow.fxml onCloseRequest="#handleExit", so you can write unit tests for this addition.

vivekscl

Some nits with the commit message
para 2:
Thus, if MainApp#stop() is not called, the app may continue to
run even after the MainWindow is closed, causing ~~any~~ existing
HelpWindows...

para 3:
For the first line maybe this sounds better?
According to the life-cycle section in the documentation on JavaFX Application[1]...

para 4:
The sentence structure is too long and complex. Maybe this sounds better?
Let's add an event handler for the onCloseRequest event in the root of MainWindow.fxml. This event will call MainWindow#handleExit() when raised by an external request to close the app.

para 5:
As a result, we are unable to write regression tests for this bug fix as the test classes terminates prematurely...

yamidark · 2018-02-23T12:27:17Z

You shouldn't setup a new stage but use mainWindow.getPrimaryStage() right?

The fireEvent(Event) method needs to be called in the FX user thread which is why I used the FxToolkit.

Perhaps we should leave a comment that WINDOW_CLOSE_REQUEST triggers the .fxml function.

Yea, will mention the MainWindow for the test does not have a X button too, and this WINDOW_CLOSE_REQUEST is used to simulate any external request to close the MainWindow as mentioned in earlier parts of the commit message.

Zhiyuan-Amos · 2018-02-23T12:30:25Z

The fireEvent(Event) method needs to be called in the FX user thread which is why I used the FxToolkit.

You can run this on the FX Thread by calling guiRobot#interact(() -> ...)

CanIHasReview-bot · 2018-02-23T16:51:27Z

v5

@yamidark submitted v5 for review.

[v5 1/1] MainWindow.fxml: update onCloseRequest event to call handleExit()

(📚 Archive) (📈 Interdiff between v4 and v5)

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2018-02-23T23:50:34Z

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

+        }
+
+        /**
+         * Closes the {@code MainWindow} using it's menu bar.


Zhiyuan-Amos · 2018-02-23T23:52:06Z

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

+    }
+
+    @Test
+    public void menuBar_exit_success() {


We are testing exit through menu bar, and the end result we are verifying for is ExitAppRequestEvent posted. So it should be:

exit_menuBarExitButton_ExitAppRequestEventPosted

Zhiyuan-Amos · 2018-02-23T23:52:51Z

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

+        }
+
+        /**
+         * Simulate an external request to close the {@code MainWindow}.


It's unclear what "external request" means.

Hmm, should I give examples like in this commit message (using the 'X' button or closing from taskbar?

It's better to leave it in the code so that future developers know what is this piece of code doing :P

Zhiyuan-Amos · 2018-02-23T23:55:07Z

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

+
+        private MainWindowHandleUnit(Stage stage) {
+            super(stage);
+            this.stage = stage;


this is a hackish way of doing things :P We can remove the need for this by storing stage as a variable in the test class alongside with mainWindow and mainWindowHandle.

yamidark · 2018-02-24T12:52:48Z

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

+    public void setUp() throws Exception {
+        FxToolkit.setupStage(stage -> {
+            mainWindow =
+                    new MainWindow(stage, new Config(), new UserPrefs(), new LogicManager(new ModelManager()));


@Zhiyuan-Amos Actually, should I extract this out as a method in the test class as prepareMainWindow(Stage), which should make the test cleaner and clearer?

Yup, looks good!

CanIHasReview-bot · 2018-02-24T15:01:00Z

v6

@yamidark submitted v6 for review.

[v6 1/1] MainWindow.fxml: update onCloseRequest event to call handleExit()

(📚 Archive) (📈 Interdiff between v5 and v6)

Checkout this PR version locally

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

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

Zhiyuan-Amos

I'm ok after these changes are made. I didn't take a look at the commit message though. @vivekscl can take a look again? Thanks!

E.g. 1st para:

The MainWindow has no specified event handler when there are external
requests to close the MainWindow, such as the close button or through
the taskbar.

The MainWindow does not have an event handler to handle external requests (such as clicking on the close button or though the taskbar) to close the MainWindow.

Zhiyuan-Amos · 2018-02-25T03:14:35Z

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

+    }
+
+    @Test
+    public void exit_menuBarExitButton_exitAppRequestEventPosted() {


Since we use the term close elsewhere (closeMainWindowExternally), let's rename exit to close.

Zhiyuan-Amos · 2018-02-25T03:15:06Z

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

+    /**
+     * {@code MainWindow} unit test class without any other components to test closing of the {@code MainWindow}.
+     */
+    private class MainWindowHandleUnit extends StageHandle {


This name is kinda weird haha. Let's rename it to EmptyMainWindowHandle.

Zhiyuan-Amos · 2018-02-25T03:15:55Z

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

+    }
+
+    /**
+     * {@code MainWindow} unit test class without any other components to test closing of the {@code MainWindow}.


Provides a handle for {@code MainWindow}. The components in {@code MainWindow} are not initialized. <- This header comment is more descriptive as it mentions that this is a handle for MainWindow.

Zhiyuan-Amos · 2018-02-25T03:16:09Z

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

+        }
+
+        /**
+         * Closes the {@code MainWindow} using its menu bar.


Let's be more specific: Closes the {@code MainWindow} by clicking on the menu bar's exit button.

Zhiyuan-Amos · 2018-02-25T03:16:50Z

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

+
+        /**
+         * Simulate an external request to close the {@code MainWindow} (e.g pressing the 'X' button on the
+         * {@code MainWindow} or closing the app through the taskbar).


Closes the {@code MainWindow} through an external request (e.g pressing the 'X' button on the.... We don't need to know that it's simulating an external request or not. :P

Zhiyuan-Amos · 2018-02-25T03:18:32Z

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

+    public void setUp() throws Exception {
+        FxToolkit.setupStage(stage -> {
+            this.stage = stage;
+            mainWindow = prepareMainWindow(stage);


I thought you meant extracting everything in this inner method as a private method :P We generally don't need to extract 1 sentence statements.

FxToolkit.setupStage(stage -> { this.stage = stage; mainWindow = new MainWindow(stage, new Config(), new UserPrefs(), new LogicManager(new ModelManager())); mainWindowHandle = new EmptyMainWindowHandle(stage); // leave 1 line here to show that the above is initialization of variables, while below is setting up the ui. stage.setScene(mainWindow.getRoot().getScene()); mainWindowHandle.focus(); });

CanIHasReview-bot · 2018-02-26T05:47:45Z

v7

@yamidark submitted v7 for review.

[v7 1/1] MainWindow.fxml: update onCloseRequest event to call handleExit()

(📚 Archive) (📈 Interdiff between v6 and v7)

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2018-02-26T09:01:07Z

@vivekscl can take a look? ^^

vivekscl

Some minor nits in the commit message:
1st para:
The MainWindow does not have an event handler to handle external requests
(such as the close button or through the taskbar) to close the
MainWindow.

2nd para:
This may cause the app to not close properly since MainApp#stop() is not called as the ExitAppRequestEvent is not raised.
MainApp#stop() is the method used to call...

Last para:
Did you mean MainWindowHandleUnitTest?
Maybe this phrasing sounds better?
... does not contain a close button, thus to test if an ExitAppRequestEvent was raised we fire a WindowEvent#WINDOW_CLOSE_REQUEST event which will trigger the onCloseRequest event.

CanIHasReview-bot · 2018-02-27T09:10:10Z

v8

@yamidark submitted v8 for review.

[v8 1/1] MainWindow.fxml: update onCloseRequest event to call handleExit()

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

Checkout this PR version locally

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

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

Zhiyuan-Amos

handle external
requests(such as the close button or through the taskbar)

Missing whitespace before the parentheses.

Let's add an event handler for the onCloseRequest event in the root of
MainWindow.fxml. This event will call MainWindow#handleExit() when
raised by an external request to close the app.

Can specify that this method raises ExitAppRequestEvent since you mentioned about ExitAppRequestEvent above.

Last paragraph missing period at the end of the sentence.

Let's wait for prof to review first :)

damithc

Looks good. Some nits only.

damithc · 2018-02-27T15:09:31Z

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

+import seedu.address.ui.testutil.EventsCollectorRule;
+
+/**
+ * Contains test for closing of the {@code MainWindow}.


This comment sounds like there is only one test?
Also, the class name doesn't indicate that the scope is limited to testing the 'close' feature only.

damithc · 2018-02-27T15:10:42Z

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

+
+    /**
+     * Provides a handle for {@code MainWindow}. The components in {@code MainWindow} are not initialized.
+     */


Provides is redundant?

The MainWindow does not have an event handler to handle external requests (such as the close button or through the taskbar) to close the MainWindow. This may cause the app to not close properly since MainApp#stop() is not called as the ExitAppRequestEvent is not raised. MainApp#stop() is the method used to call Platform#exit() and System#exit(int) which closes our app and all remaining open windows. Thus, if MainApp#stop() is not called, the app may continue to run even after the MainWindow is closed, causing existing HelpWindows to remain open even when the user expects the app to stop. According to the life-cycle section in the documentation on JavaFX Application[1], the app will only finish and call MainApp#stop() when the app either calls Platform#exit() or when the last window has been closed. Thus, should there be any HelpWindows still opened, the app will continue running, causing those HelpWindows to remain open. Let's add an event handler for the onCloseRequest event in the root of MainWindow.fxml. This event will call MainWindow#handleExit(), which raises an ExitAppRequestEvent, when there is an external request to close the app. EmptyMainWindowHandle does not contain a close button, thus to test if an ExitAppRequestEvent was raised, we fire a WindowEvent#WINDOW_CLOSE_REQUEST event which will trigger the onCloseRequest event. [1] JavaFx documentation for Application: https://docs.oracle.com/javafx/2/api/javafx/application/Application.html

CanIHasReview-bot · 2018-02-28T07:18:36Z

v9

@yamidark submitted v9 for review.

[v9 1/1] MainWindow.fxml: update onCloseRequest event to call handleExit()

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

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2018-02-28T11:56:27Z

@yamidark update the merge commit message ya :)

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

vivekscl reviewed Feb 16, 2018

View reviewed changes

yamgent reviewed Feb 18, 2018

View reviewed changes

yamidark force-pushed the 826-HelpWindow-stays-open-after-exit branch 4 times, most recently from 7a9043c to ce305ac Compare February 19, 2018 07:05

yamidark force-pushed the 826-HelpWindow-stays-open-after-exit branch from ce305ac to 61964c9 Compare February 20, 2018 14:17

Zhiyuan-Amos reviewed Feb 21, 2018

View reviewed changes

Zhiyuan-Amos mentioned this pull request Feb 21, 2018

Expand the scope of system tests #551

Open

6 tasks

vivekscl reviewed Feb 21, 2018

View reviewed changes

yamidark force-pushed the 826-HelpWindow-stays-open-after-exit branch from d190ee3 to c7f0cf8 Compare February 23, 2018 16:41

Zhiyuan-Amos reviewed Feb 23, 2018

View reviewed changes

yamidark commented Feb 24, 2018

View reviewed changes

yamidark force-pushed the 826-HelpWindow-stays-open-after-exit branch 2 times, most recently from be7e926 to b552913 Compare February 24, 2018 13:57

Zhiyuan-Amos approved these changes Feb 25, 2018

View reviewed changes

yamidark force-pushed the 826-HelpWindow-stays-open-after-exit branch 2 times, most recently from c08960e to 28f2c09 Compare February 26, 2018 05:16

vivekscl reviewed Feb 26, 2018

View reviewed changes

yamidark force-pushed the 826-HelpWindow-stays-open-after-exit branch from 28f2c09 to b233003 Compare February 27, 2018 08:40

Zhiyuan-Amos reviewed Feb 27, 2018

View reviewed changes

Zhiyuan-Amos requested a review from damithc February 27, 2018 09:24

damithc suggested changes Feb 27, 2018

View reviewed changes

yamidark force-pushed the 826-HelpWindow-stays-open-after-exit branch from b233003 to b883e8d Compare February 28, 2018 04:48

CanIHasReview-bot requested a review from damithc February 28, 2018 07:18

damithc approved these changes Feb 28, 2018

View reviewed changes

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

Zhiyuan-Amos merged commit 088db25 into se-edu:master Mar 2, 2018

yamidark deleted the 826-HelpWindow-stays-open-after-exit branch July 5, 2018 08:41

HelpWindow: help window stays open even after exiting by clicking the close button #826 #832

HelpWindow: help window stays open even after exiting by clicking the close button #826 #832

Conversation

yamidark commented Feb 16, 2018 • edited Loading

CanIHasReview-bot commented Feb 16, 2018

v1

vivekscl left a comment

Choose a reason for hiding this comment

yamidark commented Feb 17, 2018

vivekscl commented Feb 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamgent commented Feb 18, 2018

yamidark commented Feb 18, 2018

yamgent commented Feb 19, 2018

CanIHasReview-bot commented Feb 19, 2018

v2

Zhiyuan-Amos commented Feb 19, 2018 • edited Loading

yamidark commented Feb 19, 2018

Zhiyuan-Amos commented Feb 20, 2018 • edited Loading

yamidark commented Feb 20, 2018

Zhiyuan-Amos commented Feb 20, 2018

yamidark commented Feb 20, 2018 • edited Loading

yamidark commented Feb 20, 2018

Zhiyuan-Amos commented Feb 20, 2018

CanIHasReview-bot commented Feb 20, 2018

v3

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Zhiyuan-Amos commented Feb 21, 2018

yamidark commented Feb 21, 2018

Zhiyuan-Amos commented Feb 21, 2018

vivekscl left a comment • edited Loading

Choose a reason for hiding this comment

yamidark commented Feb 23, 2018

Zhiyuan-Amos commented Feb 23, 2018

CanIHasReview-bot commented Feb 23, 2018

v5

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

CanIHasReview-bot commented Feb 24, 2018

v6

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

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

CanIHasReview-bot commented Feb 26, 2018

v7

Zhiyuan-Amos commented Feb 26, 2018

vivekscl left a comment • edited Loading

Choose a reason for hiding this comment

CanIHasReview-bot commented Feb 27, 2018

v8

Zhiyuan-Amos left a comment • edited Loading

Choose a reason for hiding this comment

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

CanIHasReview-bot commented Feb 28, 2018

v9

Zhiyuan-Amos commented Feb 28, 2018

yamidark commented Feb 16, 2018 •

edited

Loading

Zhiyuan-Amos commented Feb 19, 2018 •

edited

Loading

Zhiyuan-Amos commented Feb 20, 2018 •

edited

Loading

yamidark commented Feb 20, 2018 •

edited

Loading

vivekscl left a comment •

edited

Loading

vivekscl left a comment •

edited

Loading

Zhiyuan-Amos left a comment •

edited

Loading