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.

Sign up for GitHub

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

Jump to bottom

[Windows] Handle focus events using FocusManager #24355

Closed
wants to merge 3 commits into from
Closed

[Windows] Handle focus events using FocusManager #24355

wants to merge 3 commits into from

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Aug 21, 2024 •
edited
Loading

Alternative to #23885

Description of Change

Subscribing and unsubscribing focus events for each MAUI view is costly.

This PR attempts to use FocusManager events to achieve the same result.

Performance impact

image

-> 64% improvement

Issues Fixed

Contributes to #21787

@MartyIX MartyIX requested a review from a team as a code owner August 21, 2024 14:25
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Aug 21, 2024
@MartyIX MartyIX mentioned this pull request Aug 21, 2024
Closed
@mattleibow
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

jonathanpeppers
jonathanpeppers reviewed Aug 21, 2024
View reviewed changes
src/Core/src/Handlers/View/ViewHandler.Windows.cs Outdated
using PlatformView = Microsoft.UI.Xaml.FrameworkElement;

namespace Microsoft.Maui.Handlers
{
public partial class ViewHandler
{
static Dictionary<PlatformView, ViewHandler>? FocusManagerMapping;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a ConditionalWeakTable? If DisconnectingHandler() were never called (which can be the case), would this persist both the PlatformView and ViewHandler in memory indefinitely?

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 a good point.

I think you are right that it's safer to use ConditionalWeakTable. I'll push a new commit once the tests finish to know if this PR passes all tests or not to save some time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed 68155a3. Please take a look. It's the first time I'm using this data structure, so I hope there are no gotchas.

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Aug 21, 2024 •
edited by jfversluis
Loading

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

pictos
pictos reviewed Aug 21, 2024
View reviewed changes
src/Core/src/Handlers/View/ViewHandler.Windows.cs Outdated
using PlatformView = Microsoft.UI.Xaml.FrameworkElement;

namespace Microsoft.Maui.Handlers
{
public partial class ViewHandler
{
static ConditionalWeakTable<PlatformView, ViewHandler>? FocusManagerMapping;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this's static, can't it be initialized inline?

Suggested change
static ConditionalWeakTable<PlatformView, ViewHandler>? FocusManagerMapping;
static ConditionalWeakTable<PlatformView, ViewHandler>? FocusManagerMapping = [];

This way we don't need to throw nothing

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 need to initialize https://github.com/dotnet/maui/pull/24355/files#diff-0d2394868c7630739afe7341642fb408d28ad86539bcdad79b91463a129881beR24-R25 so that's mostly the reason I have this nullable.

But I can add an initialization flag as well. I kind of don't think I can add a static ctor.

@MartyIX
Copy link
Contributor Author

MartyIX commented Aug 22, 2024

Unfortunately, UnfocusAndIsFocusedIsWorking test is failing and it's because of this PR.

The problem is that I store in

ConditionalWeakTable<PlatformView, ViewHandler>? FocusManagerMapping;

some platform view instances as keys but e.NewFocusedElement in

static void FocusManager_GotFocus(object? sender, FocusManagerGotFocusEventArgs e)

contains a seemingly different (!) object instance.

I added some debug logs and it reports:

ConnectingHandler: Platform view: Microsoft.Maui.Platform.ContentPanel (841190765)
ConnectingHandler: Platform view: Microsoft.UI.Xaml.Controls.ScrollViewer (822038485)
ConnectingHandler: Platform view: Microsoft.Maui.Platform.LayoutPanel (841193837)
ConnectingHandler: Platform view: Microsoft.UI.Xaml.Controls.TextBlock (853485565)
ConnectingHandler: Platform view: Microsoft.UI.Xaml.Controls.TextBlock (853477853)
ConnectingHandler: Platform view: Microsoft.Maui.Platform.LayoutPanel (841194605)
ConnectingHandler: Platform view: Microsoft.UI.Xaml.Controls.TextBlock (853482397)
ConnectingHandler: Platform view: Microsoft.UI.Xaml.Controls.TextBlock (853486269)
ConnectingHandler: Platform view: Microsoft.UI.Xaml.Controls.Image (853477917)
ConnectingHandler: Platform view: Microsoft.UI.Xaml.Controls.TextBlock (853477117)
ConnectingHandler: Platform view: Microsoft.UI.Xaml.Controls.TextBlock (853481725)
ConnectingHandler: Platform view: Microsoft.Maui.Platform.LayoutPanel (841191021)
ConnectingHandler: Platform view: Microsoft.Maui.Platform.MauiButton (731897293)
ConnectingHandler: Platform view: Microsoft.Maui.Platform.MauiButton (731898797)
  
FocusManager_GotFocus(Microsoft.UI.Xaml.Controls.Button)
  FocusManager_GotFocus: is platform view: 'Microsoft.UI.Xaml.Controls.Button'
  FocusManager_GotFocus[Button]: Not found platform view 685301861!
  FocusManager_GotFocus: Not found platform view 685301861!

-> So I put two MauiButtons to FocusManagerMapping with hash codes 731897293 and 731898797 but then FocusManager reports that a button was focused with hash code 685301861 (completely new).

Right now, I have no idea what to do about it. I spent a few hours on this to no avail. So either the PR idea is "wrong" or I'm missing something.

To debug the issue, one can just run Core.DeviceTests in Visual Studio, type "Unfocus" and run UnfocusAndIsFocusedIsWorking test by clicking it. The issue is 100 per cent reproducible.

@MartyIX
Copy link
Contributor Author

MartyIX commented Aug 22, 2024

Does anyone have an idea how to fix it?

@MartyIX
Copy link
Contributor Author

MartyIX commented Aug 23, 2024

I'm at loss what to do here, so I filed a question here: microsoft/microsoft-ui-xaml#9922

@MartyIX MartyIX force-pushed the feature/2024-08-21-Windows-Use-FocusManager branch from 68155a3 to ab5551f Compare September 10, 2024 13:40
@MartyIX
Copy link
Contributor Author

MartyIX commented Sep 10, 2024

@albyrock87 figured out how to fix the implementation and the failing test 🎉.

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX MartyIX mentioned this pull request Sep 10, 2024
Merged
@albyrock87
Copy link
Contributor

Given the huge performance bonus, I vote for #24695

@mattleibow mattleibow added the area-keyboard Keyboard, soft keyboard label Sep 12, 2024
@MartyIX MartyIX added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label Sep 24, 2024
@MartyIX
Copy link
Contributor Author

MartyIX commented Sep 27, 2024

Replaced by #24695

@MartyIX MartyIX closed this Sep 27, 2024
@MartyIX MartyIX deleted the feature/2024-08-21-Windows-Use-FocusManager branch September 27, 2024 13:15
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathanpeppers jonathanpeppers jonathanpeppers left review comments

@pictos pictos pictos left review comments

@jfversluis jfversluis Awaiting requested review from jfversluis jfversluis is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

@tj-devel709 tj-devel709 Awaiting requested review from tj-devel709 tj-devel709 is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

Assignees
No one assigned
Labels
area-keyboard Keyboard, soft keyboard community ✨ Community Contribution t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MartyIX @mattleibow @jonathanpeppers @albyrock87 @pictos