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

Application ExitMode #1617

Closed
wants to merge 11 commits into from
Closed

Application ExitMode #1617

wants to merge 11 commits into from

Conversation

Gillibald
Copy link
Contributor

@Gillibald Gillibald commented May 26, 2018

  • What does the pull request do?
    This PR introduces a way to control when the application exits. You can either let the application exit explicitly or implicitly. Furthermore you can now access the main window and open Windows under the current application instance.
  • What is the current behavior?
    The current state calls exit when the main window was closed and there is no way to control this behavior
  • What is the updated/expected behavior with this PR?
    You are able to control when the application exits.
  • How was the solution implemented (if it's not obvious)?
    In the past all open windows where stored under Window and moved to Application with these changes. A ExitMode property was added to application and a SetExitMode method was added to the AppBuilder.

Checklist:

See #1565

@Gillibald
Copy link
Contributor Author

Should we rename OnExiting to OnShutdown and Exit to Shutdown to better reflect what is happening and better match wpf naming?

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this out by opening another MainWindow in the control catalog when a button is pressed, and setting SetExitMode(ExitMode.OnMainWindowClose);

However, I'm getting an exception when I close the first main window while the second is open:

System.ArgumentOutOfRangeException
  HResult=0x80131502
  Message=Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index
  Source=mscorlib
  StackTrace:
   at System.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument, ExceptionResource resource)
   at System.Collections.Generic.List`1.RemoveAt(Int32 index)
   at Avalonia.WindowCollection.RemoveAt(Int32 index) in D:\projects\AvaloniaUI\Avalonia\src\Avalonia.Controls\WindowCollection.cs:line 93

@Gillibald
Copy link
Contributor Author

Will have a look at it and add some unit tests that do exactly what you tried.

@Gillibald Gillibald changed the title Application ShutdownMode WIP: Application ShutdownMode May 31, 2018
Added unit tests
@Gillibald Gillibald changed the title WIP: Application ShutdownMode Application ExitMode Jun 1, 2018
@Gillibald
Copy link
Contributor Author

Have fixed the issue with OnMainWindowClose and added unit Tests for every mode. I dont understand why the formatting of some code is different. Maybe StyleCop did that.

Copy link
Contributor

@lindexi lindexi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WindowCollection.Remove the window should remove the Window.Closed

/// </summary>
/// <param name="token">The token to track</param>
public void Run(CancellationToken token)
{
Dispatcher.UIThread.MainLoop(token);

if (!IsExiting)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not exiting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is needed to call Exit if an error happened. We know that an error happened if Exit wasn't called explicitly so IsExiting isn't set.

}

/// <summary>
/// Exits the application
/// </summary>
public void Exit()
{
IsExiting = true;

while (Windows.Count > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I have tow Windows that this loop never exit.

I think you should write remove the first windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's wrong because this line always removes the first window of the collection until none is left. A close removes the window from the list. I will add unit tests to prove that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gillibald Good

/// <param name="window">The window.</param>
internal void Remove(Window window)
{
_windows.Remove(window);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you remove the window that you should remove the Closed.

window.Closed -= OnWindowClosed;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove the subscription. Good catch.

@Gillibald
Copy link
Contributor Author

Will close this and reopen with a clean commit history

@Gillibald Gillibald closed this Jun 6, 2018
@Sorien
Copy link
Contributor

Sorien commented Jun 6, 2018

if you can change commits history and force push to update your PR

@grokys grokys mentioned this pull request Jun 14, 2018
3 tasks
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.

4 participants