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

Add ClosingBehavior api to Window #14621

Merged
merged 1 commit into from
Feb 18, 2024
Merged

Add ClosingBehavior api to Window #14621

merged 1 commit into from
Feb 18, 2024

Conversation

emmauss
Copy link
Contributor

@emmauss emmauss commented Feb 16, 2024

What does the pull request do?

Adds the following api to the Window class;

public class Window 
{
  /// <summary>
  /// Gets or sets a value indicating how the <see cref="Closing"/> event behaves in the presence
  /// of child windows.
  /// </summary>
  public WindowClosingBehavior ClosingBehavior {get; set;}
}

/// <summary>
/// Describes how the <see cref="Window.Closing"/> event behaves in the presence of child windows.
/// </summary>
public enum WindowClosingBahavior 
{
  /// <summary>
  /// When the owner window is closed, the child windows' <see cref="Window.Closing"/> event
  /// will be raised, followed by the owner window's <see cref="Window.Closing"/> events. A child
  /// canceling the close will result in the owner Window's close being cancelled.
  /// </summary>
  OwnerAndChildWindows,

  /// <summary>
  /// When the owner window is closed, only the owner window's <see cref="Window.Closing"/> event
  /// will be raised. This is the same as WPF's.
  /// </summary>
  OwnerWindowOnly,
}

What is the current behavior?

Avalonia's Window closing order when child windows are present differs from WPF's and can cause some side effects when there are many child windows to managed. All child windows are checked for closing before the parent window gets checked. If a child window performs disposal logic or any logic that may change the state of the window, assuming that it's going to get closed, but the Closing event is cancelled by the next child or the parent window, that window may be left in an invalid state.

What is the updated/expected behavior with this PR?

This pr add api to decide how closing multiple related windows should be handled. if ClosingBehavior is set to OwnerWindowOnly, only the owner window, i.e the window where the Closing event was first raised either by the Close button or programmatically, checks for cancellation, and if not cancelled, all child windows are Closed, skipping raising the Closing event for child windows. Setting ClosingBehavior to OwnerAndChildWindows, which is the default, maintains the previous behavior.

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

@emmauss emmauss force-pushed the override_owned_closing branch from fbc8ecb to 603ac32 Compare February 16, 2024 11:15
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0045018-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Comment on lines +70 to +81
/// <summary>
/// When the owner window is closed, the child windows' <see cref="Window.Closing"/> event
/// will be raised, followed by the owner window's <see cref="Window.Closing"/> events. A child
/// canceling the close will result in the owner Window's close being cancelled.
/// </summary>
OwnerAndChildWindows,

/// <summary>
/// When the owner window is closed, only the owner window's <see cref="Window.Closing"/> event
/// will be raised. This behavior is the same as WPF's.
/// </summary>
OwnerWindowOnly,
Copy link
Contributor

@jp2masa jp2masa Feb 16, 2024

Choose a reason for hiding this comment

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

I think the Closing and Closed events are a bit mixed in the XML comments, at least for OwnerAndChildWindows. According to the PR description, it doesn't raise the Closing event for child windows. Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When set to OwnerAndChildWindows which is the default and is the current behavior as of master, Closing will be raised on all child windows before being raised on the parent. Closed event is raised at the point that it's decided the window will be closed, i.e. all Closing handlers that were raised didn't cancel Closing event.

@thevortexcloud
Copy link
Contributor

Out of curiosity, when does Closed fire in the situation described in the PR description?

@emmauss
Copy link
Contributor Author

emmauss commented Feb 16, 2024

Out of curiosity, when does Closed fire in the situation described in the PR description?

Closed event is still fired as normal, for any window that has been closed. This event is raised when no window cancels the Closing event. At the point Closed is raised, the window is no longer alive. This PR only changes the way Closing is raised, giving the option to match WPF's behavior while still keeping the current behavior.

@jp2masa
Copy link
Contributor

jp2masa commented Feb 16, 2024

I think this is the confusing part of the PR description:

(...) only the owner window (...) checks for cancellation, and if not cancelled, all child windows are Closed, skipping the Closing event for child windows.

This is supposed to mean that the Closing event is still called but ignored for cancellation purposes? In that case, the Closing event is not skipped.

@emmauss
Copy link
Contributor Author

emmauss commented Feb 16, 2024

I think this is the confusing part of the PR description:

(...) only the owner window (...) checks for cancellation, and if not cancelled, all child windows are Closed, skipping the Closing event for child windows.

This is supposed to mean that the Closing event is still called but ignored for cancellation purposes? In that case, the Closing event is not skipped.

I've updated to "skipping raising the Closing event". The Closing event is never called for the children window of the window that is being closed, i.e. the window the user has pressed the Close button, for an example.

@jp2masa
Copy link
Contributor

jp2masa commented Feb 16, 2024

In that case, I think the XML documentation is wrong, because the Closing event won't be raised for child windows:

        /// When the owner window is closed, the child windows' <see cref="Window.Closing"/> event
        /// will be raised, followed by the owner window's <see cref="Window.Closing"/> events. A child
        /// canceling the close will result in the owner Window's close being cancelled.

@emmauss
Copy link
Contributor Author

emmauss commented Feb 16, 2024

In that case, I think the XML documentation is wrong, because the Closing event won't be raised for child windows:

        /// When the owner window is closed, the child windows' <see cref="Window.Closing"/> event
        /// will be raised, followed by the owner window's <see cref="Window.Closing"/> events. A child
        /// canceling the close will result in the owner Window's close being cancelled.

The description is accurate. OwnerAndChildWindows does not skip child windows. Child windows are actually checked first, before parent windows, and all event as raised. If a child cancels the Closing event, there is no need to check the parent for closing. If no window cancels the event, all related windows would have gotten the Closing event.
Its OwnerWindowOnly that will never raise Closing event on the child window, unless the child window is the source of the Closing event.

@jp2masa
Copy link
Contributor

jp2masa commented Feb 16, 2024

You're right. I was confused... Sorry for wasting your time, and thanks for clarifying.

@kekekeks kekekeks added this pull request to the merge queue Feb 18, 2024
Merged via the queue into master with commit 073a530 Feb 18, 2024
7 checks passed
@kekekeks kekekeks deleted the override_owned_closing branch February 18, 2024 09:27
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.

5 participants