Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close all windows on Application close, not just main one #184

Merged
merged 1 commit into from
Jan 16, 2014

Conversation

ivan-danilov
Copy link
Contributor

On Application.Close() instead of closing main window only it is better to close everything we can - there can be more than one window, e.g. in multi-threaded UI case.

@JakeGinnivan
Copy link
Member

This sounds like a good change. Though I am not sure of the possible impact due to change of behaviour. Wondering if this should be in a method CloseAllWindows() and possibly that method should try close all modal windows then close the rest?

@ivan-danilov
Copy link
Contributor Author

I thought what would happen in case of modal windows. Closing modal and then the rest wouldn't help because it is possible that one modal window A opens another modal one B - then in order to close them correctly we need to close B, then A then main window.
But the same problem was present in the original code where only main window closed. So my thought was that this change will not make things worse at least. And in our concrete case it'd make them better, because we have (very strange admittedly, but I can't change that easily) architecture where it is possible to have several UI threads, each managing its own set of windows. And closing main window doesn't end the application, because other thread owns another window, hence we were waiting for timeout each time and then White kills the process. With this change our app exits correctly.

In order to be as correct as possible, I believe we have to close all enabled windows in loop until either timeout reached or no opened windows left. I'd leave that as the next step if you don't mind :)

@JakeGinnivan
Copy link
Member

Sounds reasonable

Just one failing test, PostCloseMessage has to be made virtual.

 System.Exception : The following methods are not marked virtual: 
 TestStack.White.WindowsAPI.NativeWindow.PostCloseMessage

…er to close everything we can - there can be more than one window, e.g. in case of multi-threaded UI case.
@ivan-danilov
Copy link
Contributor Author

Fixed (by amending).

JakeGinnivan added a commit that referenced this pull request Jan 16, 2014
Close all windows on Application close, not just main one
@JakeGinnivan JakeGinnivan merged commit 515ee83 into TestStack:master Jan 16, 2014
@ivan-danilov ivan-danilov deleted the close-app branch January 17, 2014 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants