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 (take 2) #24695

Merged
merged 4 commits into from
Sep 27, 2024
Merged

[Windows] Handle focus events using FocusManager (take 2) #24695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85fe5ee
WIPWIP
MartyIX Aug 21, 2024
5291005
Feedback
MartyIX Aug 21, 2024
fe46d42
Refactor usage of `FocusManager` to avoid weak table usage
albyrock87 Sep 10, 2024
4e65db7
Use ConditionalWeakTable
MartyIX Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 32 additions & 16 deletions src/Core/src/Handlers/View/ViewHandler.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,35 @@
#nullable enable
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Runtime.CompilerServices;
using Microsoft.UI.Xaml;
using Microsoft.UI.Xaml.Controls;
using Microsoft.UI.Xaml.Controls.Primitives;
using Microsoft.UI.Xaml.Input;
using PlatformView = Microsoft.UI.Xaml.FrameworkElement;

namespace Microsoft.Maui.Handlers
{
public partial class ViewHandler
{
readonly static ConditionalWeakTable<PlatformView, ViewHandler> FocusManagerMapping = new();

static ViewHandler()
{
FocusManager.GotFocus += FocusManager_GotFocus;
FocusManager.LostFocus += FocusManager_LostFocus;
}

partial void ConnectingHandler(PlatformView? platformView)
{
if (platformView != null)
if (platformView is not null)
{
platformView.GotFocus += OnPlatformViewGotFocus;
platformView.LostFocus += OnPlatformViewLostFocus;
FocusManagerMapping.Add(platformView, this);
Copy link
Member

Choose a reason for hiding this comment

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

Is there no perf issues when there is a super complex UI? If we have maybe 1k elements on screen, will not the add and lookup be slow? It may still be faster than what we had before, but I am getting flashbacks when millions of objects would sub and unsub from the theme changed event.

Copy link
Contributor

Choose a reason for hiding this comment

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

when millions of objects would sub and unsub from the theme changed event

Removing an entry from a CWT seems very fast compared to event unsubscription.

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs#L614

I would rather set an initial capacity of 128 to the CWT to avoid a lot of initial resize operation (which is expensive).

If you're still concerned we have an alternative but much slower implementation:
#24355

Copy link
Contributor Author

@MartyIX MartyIX Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we have maybe 1k elements on screen, will not the add and lookup be slow?

Adding to CWT is definitely faster because it does not involve interops (perf comparison of ConnectingHandler should be persuasive here). CTW should behave like a dictionary and "insert" operation for dictionaries is O(1) (i.e. so in practice it should be ~<10 nanoseconds1; whereas interops are more like milliseconds). So CWT lookups and inserts should be orders of magnitude faster.

I think that the CWT approach is strictly better for performance right now. What can be done is that I can try to create a huge grid with 1000 elements and measure performance for that scenario.

I would rather set an initial capacity of 128 to the CWT to avoid a lot of initial resize operation (which is expensive).

That's a good idea.

Footnotes

  1. https://startdebugging.net/2023/08/net-8-performance-dictionary-vs-frozendictionary/ just to get a feel what one might expect. ↩

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps @jonathanpeppers can double check my post as he's a performance guru :)

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 would rather set an initial capacity of 128 to the CWT to avoid a lot of initial resize operation (which is expensive).

That's a good idea.

@albyrock87 Interestingly, the CTW does not support passing capacity as far as I can tell: https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2.-ctor?view=net-8.0 👀

}
}

partial void DisconnectingHandler(PlatformView platformView)
{
FocusManagerMapping.Remove(platformView);
UpdateIsFocused(false);

platformView.GotFocus -= OnPlatformViewGotFocus;
platformView.LostFocus -= OnPlatformViewLostFocus;
}

static partial void MappingFrame(IViewHandler handler, IView view)
Expand Down Expand Up @@ -88,7 +92,9 @@ public static void MapAnchorY(IViewHandler handler, IView view)
public static void MapToolbar(IViewHandler handler, IView view)
{
if (view is IToolbarElement tb)
{
MapToolbar(handler, tb);
}
}

internal static void MapToolbar(IElementHandler handler, IToolbarElement toolbarElement)
Expand Down Expand Up @@ -133,25 +139,35 @@ internal static void MapContextFlyout(IElementHandler handler, IContextFlyoutEle
}
}

void OnPlatformViewGotFocus(object sender, RoutedEventArgs args)
static void FocusManager_GotFocus(object? sender, FocusManagerGotFocusEventArgs e)
{
UpdateIsFocused(true);
if (e.NewFocusedElement is PlatformView platformView && FocusManagerMapping.TryGetValue(platformView, out ViewHandler? handler))
{
handler.UpdateIsFocused(true);
}
}

void OnPlatformViewLostFocus(object sender, RoutedEventArgs args)
static void FocusManager_LostFocus(object? sender, FocusManagerLostFocusEventArgs e)
{
UpdateIsFocused(false);
if (e.OldFocusedElement is PlatformView platformView && FocusManagerMapping.TryGetValue(platformView, out ViewHandler? handler))
{
handler.UpdateIsFocused(false);
}
}

void UpdateIsFocused(bool isFocused)
{
if (VirtualView == null)
if (VirtualView is not { } virtualView)
{
return;
}

bool updateIsFocused = (isFocused && !VirtualView.IsFocused) || (!isFocused && VirtualView.IsFocused);
bool updateIsFocused = (isFocused && !virtualView.IsFocused) || (!isFocused && virtualView.IsFocused);

if (updateIsFocused)
VirtualView.IsFocused = isFocused;
{
virtualView.IsFocused = isFocused;
}
}
}
}
6 changes: 5 additions & 1 deletion src/Core/src/Platform/Windows/ViewExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ public static void Focus(this FrameworkElement platformView, FocusRequest reques
public static void Unfocus(this FrameworkElement platformView, IView view)
{
if (platformView is Control control)
{
UnfocusControl(control);
}
}

public static void UpdateVisibility(this FrameworkElement platformView, IView view)
Expand Down Expand Up @@ -378,8 +380,10 @@ internal static Graphics.Rect GetBoundingBox(this FrameworkElement? platformView

internal static void UnfocusControl(Control control)
{
if (control == null || !control.IsEnabled)
if (!control.IsEnabled)
{
return;
}

var isTabStop = control.IsTabStop;
control.IsTabStop = false;
Expand Down
2 changes: 1 addition & 1 deletion src/Core/tests/DeviceTests.Shared/HandlerTests/Focus/FocusHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ await AttachAndRun<LayoutHandler>(layout, async (contentViewHandler) =>
Assert.True(inputControl1.IsFocused);
Assert.False(inputControl2.IsFocused);
// UNfocus the first control (revert the focus)
// Unfocus the first control (revert the focus)
inputControl1.Handler.Invoke(nameof(IView.Unfocus));
// assert
Expand Down
29 changes: 19 additions & 10 deletions src/TestUtils/src/DeviceTests/AssertionExtensions.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Microsoft.UI.Xaml.Media.Imaging;
using Windows.Graphics.DirectX;
using Windows.Storage.Streams;
using Microsoft.UI.Xaml.Input;
using Xunit;
using Xunit.Sdk;
using WColor = Windows.UI.Color;
Expand Down Expand Up @@ -49,42 +50,50 @@ public static Task SendKeyboardReturnType(this FrameworkElement view, ReturnType
public static async Task WaitForFocused(this FrameworkElement view, int timeout = 1000)
{
TaskCompletionSource focusSource = new TaskCompletionSource();
view.GotFocus += OnFocused;

FocusManager.GotFocus += OnFocused;

try
{
await focusSource.Task.WaitAsync(TimeSpan.FromMilliseconds(timeout));
}
finally
{
view.GotFocus -= OnFocused;
FocusManager.GotFocus -= OnFocused;
}

void OnFocused(object? sender, RoutedEventArgs e)
void OnFocused(object? sender, FocusManagerGotFocusEventArgs e)
{
view.GotFocus -= OnFocused;
focusSource.SetResult();
if (e.NewFocusedElement == view)
{
FocusManager.GotFocus -= OnFocused;
focusSource.SetResult();

This comment was marked as resolved.

Sign in to view

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that could happen considering the event is being unsubscribed in the line above.

If there was a race condition, raising an exception would be exactly what we want in this device test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh my bad, I didn't see it's a unit test. Reviewing code from mobile is hard 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw: are you interested in this PR for performance reasons?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MartyIX yes, and it looks like to have a good gotcha on here, so I'm interested in learning new tricks (;

}
}
}

public static async Task WaitForUnFocused(this FrameworkElement view, int timeout = 1000)
{
TaskCompletionSource focusSource = new TaskCompletionSource();
view.LostFocus += OnUnFocused;

FocusManager.LostFocus += OnUnFocused;

try
{
await focusSource.Task.WaitAsync(TimeSpan.FromMilliseconds(timeout));
}
finally
{
view.LostFocus -= OnUnFocused;
FocusManager.LostFocus -= OnUnFocused;
}

void OnUnFocused(object? sender, RoutedEventArgs e)
void OnUnFocused(object? sender, FocusManagerLostFocusEventArgs e)
{
view.LostFocus -= OnUnFocused;
focusSource.SetResult();
if (e.OldFocusedElement == view)
{
FocusManager.LostFocus -= OnUnFocused;
focusSource.SetResult();
}
}
}

Expand Down
Loading