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

[Windows][GestureManager] Unsubscribe control tap events only if they were subscribed #23976

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Aug 2, 2024

Description of Change

The idea of the PR is that subscriptions of

  • _control.Tapped
  • _control.DoubleTapped

make sense to unsubscribe only if they were subscribed.

Performance impact

image

-> 86% for execution of GestureManager.SetupGestureManager

Issues Fixed

Contributes to #21787

Easier to review with whitespace off.

@MartyIX MartyIX requested a review from a team as a code owner August 2, 2024 10:37
Copy link
Contributor

Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Aug 2, 2024
@MartyIX MartyIX force-pushed the feature/2024-08-02-Win-GesturePlatformManager-subscriptions-FINAL branch from a487bef to 2547cfc Compare August 2, 2024 10:39

ClearContainerEventHandlers();

if (_element != null)
if (_element is View && ElementGestureRecognizers is not null)
Copy link
Contributor Author

@MartyIX MartyIX Aug 2, 2024

Choose a reason for hiding this comment

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

Just modernize this part a bit using pattern matching and make it less nested.

@MartyIX MartyIX added platform/windows 🪟 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) labels Aug 2, 2024
@jonathanpeppers
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).


ClearContainerEventHandlers();

if (_element != null)
if (_element is View && ElementGestureRecognizers is not null)
Copy link
Member

Choose a reason for hiding this comment

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

Could we change this line to be:

Suggested change
if (_element is View && ElementGestureRecognizers is not null)
if (_element is View && ElementGestureRecognizers is ObservableCollection<IGestureRecognizer> gestures)

And then use the local gestures field throughout.

Calling ElementGestureRecognizers multiple times, looks like it would do some work:

ObservableCollection<IGestureRecognizer>? ElementGestureRecognizers =>
(_handler.VirtualView as Element)?.GetCompositeGestureRecognizers() as ObservableCollection<IGestureRecognizer>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added bf2f835 and 0e14cb3. Moreover, I rebased the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used slightly different syntax to make the code more resilient to type changes (unlikely to occur I guess but anyway).

@MartyIX MartyIX force-pushed the feature/2024-08-02-Win-GesturePlatformManager-subscriptions-FINAL branch from 2547cfc to 0e14cb3 Compare August 8, 2024 19:01
@jonathanpeppers
Copy link
Member

jonathanpeppers commented Aug 8, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

jonathanpeppers
jonathanpeppers previously approved these changes Aug 8, 2024
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

Looks ok to me,👍I triggered CI

Copy link
Member

@jfversluis jfversluis left a comment

Choose a reason for hiding this comment

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

Some build errors here that need to be addressed

D:\a\1\s\src\Controls\src\Core\Platform\GestureManager\GesturePlatformManager.Windows.cs(294,61): error CS0128: A local variable or function named 'gestureRecognizers' is already defined in this scope [D:\a\1\s\src\Controls\src\Core\Controls.Core.csproj::TargetFramework=net8.0-windows10.0.20348.0]
D:\a\1\s\src\Controls\src\Core\Platform\GestureManager\GesturePlatformManager.Windows.cs(296,6): error CS0165: Use of unassigned local variable 'gestureRecognizers' [D:\a\1\s\src\Controls\src\Core\Controls.Core.csproj::TargetFramework=net8.0-windows10.0.20348.0]
C:\Users\VssAdministrator.nuget\packages\microsoft.windowsappsdk\1.5.240627000\buildTransitive\Microsoft.UI.Xaml.Markup.Compiler.interop.targets(841,9): error MSB3073: The command ""C:\Users\VssAdministrator.nuget\packages\microsoft.windowsappsdk\1.5.240627000\buildTransitive..\tools\net6.0..\net472\XamlCompiler.exe" "D:\a\1\s\artifacts\obj\Controls.Core\Release\net8.0-windows10.0.20348.0\input.json" "D:\a\1\s\artifacts\obj\Controls.Core\Release\net8.0-windows10.0.20348.0\output.json"" exited with code 1. [D:\a\1\s\src\Controls\src\Core\Controls.Core.csproj::TargetFramework=net8.0-windows10.0.20348.0]
D:\a\1\s\src\Controls\src\Core\Platform\GestureManager\GesturePlatformManager.Windows.cs(294,61): error CS0128: A local variable or function named 'gestureRecognizers' is already defined in this scope [D:\a\1\s\src\Controls\src\Core\Controls.Core.csproj::TargetFramework=net8.0-windows10.0.19041.0]
D:\a\1\s\src\Controls\src\Core\Platform\GestureManager\GesturePlatformManager.Windows.cs(296,6): error CS0165: Use of unassigned local variable 'gestureRecognizers' [D:\a\1\s\src\Controls\src\Core\Controls.Core.csproj::TargetFramework=net8.0-windows10.0.19041.0]

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jfversluis jfversluis merged commit 534ed02 into dotnet:main Aug 9, 2024
97 checks passed
@MartyIX MartyIX deleted the feature/2024-08-02-Win-GesturePlatformManager-subscriptions-FINAL branch August 9, 2024 13:07
@samhouts samhouts added the fixed-in-net8.0-nightly This may be available in a nightly release! label Aug 12, 2024
@samhouts samhouts added the fixed-in-net9.0-nightly This may be available in a nightly release! label Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community ✨ Community Contribution fixed-in-net8.0-nightly This may be available in a nightly release! fixed-in-net9.0-nightly This may be available in a nightly release! platform/windows 🪟 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants