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

Event Renaming (Removing Type Prefix) #11549

Merged
merged 2 commits into from
May 29, 2023
Merged

Conversation

robloo
Copy link
Contributor

@robloo robloo commented May 28, 2023

What does the pull request do?

Standardizes event naming by removing the type prefix. See the full analysis in the issue #10663 here.

What is the current behavior?

Events are improperly named and don't follow WPF/WinUI.

What is the updated/expected behavior with this PR?

Events are properly named removing the type prefix and follow WPF/WinUI where possible.

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

Obvious

Checklist

Breaking changes

Yes, event names have changed which will break compilation.

  • ContextMenu
    • ContextMenuClosing -> Closing
    • ContextMenuOpening -> Opening
  • MenuBase
    • MenuClosed -> Closed
    • MenuOpened -> Opened

Obsoletions / Deprecations

None

Fixed issues

Closes #10663

@robloo
Copy link
Contributor Author

robloo commented May 28, 2023

Window is the only other type I can find (searched routed events) that has this issue

/// <summary>
/// Routed event that can be used for global tracking of window destruction
/// </summary>
public static readonly RoutedEvent<RoutedEventArgs> WindowClosedEvent =
RoutedEvent.Register<Window, RoutedEventArgs>("WindowClosed", RoutingStrategies.Direct);
/// <summary>
/// Routed event that can be used for global tracking of opening windows
/// </summary>
public static readonly RoutedEvent<RoutedEventArgs> WindowOpenedEvent =
RoutedEvent.Register<Window, RoutedEventArgs>("WindowOpened", RoutingStrategies.Direct);

This appears to be a special-case so let me know if it needs to change as well. I would like to keep things consistent.

@robloo robloo marked this pull request as ready for review May 28, 2023 17:29
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0035527-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 requested a review from grokys May 28, 2023 17:44
@danwalmsley danwalmsley added this to the 11.0 milestone May 29, 2023
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.

LGTM, @maxkatz6 says we should consider ContextMenu obsolete and not change it but I think that given that we're changing MenuBase here we might as well do it.

I was going to suggest adding compatibility aliases for the old event naming but given the recent amount of breaking changes made by us I fear I'll be laughed out of town, so I won't do that :D

@grokys grokys added this pull request to the merge queue May 29, 2023
Merged via the queue into AvaloniaUI:master with commit 44454f5 May 29, 2023
@robloo robloo deleted the event-renaming branch May 29, 2023 22:24
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.

Menu Events are Improperly Named
4 participants