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

[ios] fix memory leak in SearchBar #16383

Merged
merged 5 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 2 additions & 0 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ void SetupBuilder()
handlers.AddHandler<IndicatorView, IndicatorViewHandler>();
handlers.AddHandler<RefreshView, RefreshViewHandler>();
handlers.AddHandler<IScrollView, ScrollViewHandler>();
handlers.AddHandler<SearchBar, SearchBarHandler>();
handlers.AddHandler<Slider, SliderHandler>();
handlers.AddHandler<Stepper, StepperHandler>();
handlers.AddHandler<SwipeView, SwipeViewHandler>();
Expand Down Expand Up @@ -74,6 +75,7 @@ void SetupBuilder()
[InlineData(typeof(Polyline))]
[InlineData(typeof(RefreshView))]
[InlineData(typeof(ScrollView))]
[InlineData(typeof(SearchBar))]
[InlineData(typeof(Slider))]
[InlineData(typeof(Stepper))]
[InlineData(typeof(SwipeView))]
Expand Down
158 changes: 98 additions & 60 deletions src/Core/src/Handlers/SearchBar/SearchBarHandler.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace Microsoft.Maui.Handlers
public partial class SearchBarHandler : ViewHandler<ISearchBar, MauiSearchBar>
{
UITextField? _editor;
readonly MauiSearchBarProxy _proxy = new();

public UITextField? QueryEditor => _editor;

Expand All @@ -23,35 +24,14 @@ protected override MauiSearchBar CreatePlatformView()

protected override void ConnectHandler(MauiSearchBar platformView)
{
platformView.CancelButtonClicked += OnCancelClicked;
platformView.SearchButtonClicked += OnSearchButtonClicked;
platformView.TextSetOrChanged += OnTextPropertySet;
platformView.OnMovedToWindow += OnMovedToWindow;
platformView.ShouldChangeTextInRange += ShouldChangeText;
platformView.OnEditingStarted += OnEditingStarted;
platformView.EditingChanged += OnEditingChanged;
platformView.OnEditingStopped += OnEditingStopped;
_proxy.Connect(this, VirtualView, platformView);
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR completely breaks all the UI tests, which accesses a SearchBar to find & run each test?

I did not figure out the cause, it seemed like SearchBar generally worked when I tried in other apps like the DeviceTests.


base.ConnectHandler(platformView);
}

void OnMovedToWindow(object? sender, EventArgs e)
{
// The cancel button doesn't exist until the control has moved to the window
// so we fire this off again so it can set the color
UpdateValue(nameof(ISearchBar.CancelButtonColor));
}

protected override void DisconnectHandler(MauiSearchBar platformView)
{
platformView.CancelButtonClicked -= OnCancelClicked;
platformView.SearchButtonClicked -= OnSearchButtonClicked;
platformView.TextSetOrChanged -= OnTextPropertySet;
platformView.ShouldChangeTextInRange -= ShouldChangeText;
platformView.OnMovedToWindow -= OnMovedToWindow;
platformView.OnEditingStarted -= OnEditingStarted;
platformView.EditingChanged -= OnEditingChanged;
platformView.OnEditingStopped -= OnEditingStopped;
_proxy.Disconnect(platformView, _editor);

base.DisconnectHandler(platformView);
}
Expand Down Expand Up @@ -166,56 +146,114 @@ public static void MapKeyboard(ISearchBarHandler handler, ISearchBar searchBar)
handler.PlatformView?.UpdateKeyboard(searchBar);
}

void OnCancelClicked(object? sender, EventArgs args)
void UpdateCancelButtonVisibility()
{
if (VirtualView != null)
VirtualView.Text = string.Empty;
if (PlatformView.ShowsCancelButton != VirtualView.ShouldShowCancelButton())
UpdateValue(nameof(ISearchBar.CancelButtonColor));
}

void OnSearchButtonClicked(object? sender, EventArgs e)
class MauiSearchBarProxy
{
VirtualView?.SearchButtonPressed();
}
WeakReference<SearchBarHandler>? _handler;
WeakReference<ISearchBar>? _virtualView;

void OnTextPropertySet(object? sender, UISearchBarTextChangedEventArgs a)
{
VirtualView.UpdateText(a.SearchText);
ISearchBar? VirtualView => _virtualView is not null && _virtualView.TryGetTarget(out var v) ? v : null;

UpdateCancelButtonVisibility();
}
SearchBarHandler? Handler => _handler is not null && _handler.TryGetTarget(out var h) ? h : null;

bool ShouldChangeText(UISearchBar searchBar, NSRange range, string text)
{
var newLength = searchBar?.Text?.Length + text.Length - range.Length;
return newLength <= VirtualView?.MaxLength;
}
public void Connect(SearchBarHandler handler, ISearchBar virtualView, MauiSearchBar platformView)
{
_handler = new(handler);
_virtualView = new(virtualView);

platformView.CancelButtonClicked += OnCancelClicked;
platformView.SearchButtonClicked += OnSearchButtonClicked;
platformView.TextSetOrChanged += OnTextPropertySet;
platformView.OnMovedToWindow += OnMovedToWindow;
platformView.ShouldChangeTextInRange += ShouldChangeText;
platformView.OnEditingStarted += OnEditingStarted;
platformView.OnEditingStopped += OnEditingStopped;

if (handler.QueryEditor is UITextField editor)
editor.EditingChanged += OnEditingChanged;
}

void OnEditingStarted(object? sender, EventArgs e)
{
if (VirtualView is not null)
VirtualView.IsFocused = true;
}
public void Disconnect(MauiSearchBar platformView, UITextField? editor)
{
_virtualView = null;
_handler = null;

platformView.CancelButtonClicked -= OnCancelClicked;
platformView.SearchButtonClicked -= OnSearchButtonClicked;
platformView.TextSetOrChanged -= OnTextPropertySet;
platformView.ShouldChangeTextInRange -= ShouldChangeText;
platformView.OnMovedToWindow -= OnMovedToWindow;
platformView.OnEditingStarted -= OnEditingStarted;
platformView.OnEditingStopped -= OnEditingStopped;

if (editor is not null)
editor.EditingChanged -= OnEditingChanged;
}

void OnEditingChanged(object? sender, EventArgs e)
{
if (VirtualView == null || _editor == null)
return;
void OnMovedToWindow(object? sender, EventArgs e)
{
// The cancel button doesn't exist until the control has moved to the window
// so we fire this off again so it can set the color
if (Handler is SearchBarHandler handler)
{
handler.UpdateValue(nameof(ISearchBar.CancelButtonColor));
}
}

void OnCancelClicked(object? sender, EventArgs args)
{
if (VirtualView is ISearchBar virtualView)
virtualView.Text = string.Empty;
}

VirtualView.UpdateText(_editor.Text);
void OnSearchButtonClicked(object? sender, EventArgs e)
{
VirtualView?.SearchButtonPressed();
}

UpdateCancelButtonVisibility();
}
void OnTextPropertySet(object? sender, UISearchBarTextChangedEventArgs a)
{
if (VirtualView is ISearchBar virtualView)
{
virtualView.UpdateText(a.SearchText);

if (Handler is SearchBarHandler handler)
{
handler.UpdateCancelButtonVisibility();
}
}
}

void OnEditingStopped(object? sender, EventArgs e)
{
if (VirtualView is not null)
VirtualView.IsFocused = false;
}
bool ShouldChangeText(UISearchBar searchBar, NSRange range, string text)
{
var newLength = searchBar?.Text?.Length + text.Length - range.Length;
return newLength <= VirtualView?.MaxLength;
}

void UpdateCancelButtonVisibility()
{
if (PlatformView.ShowsCancelButton != VirtualView.ShouldShowCancelButton())
UpdateValue(nameof(ISearchBar.CancelButtonColor));
void OnEditingStarted(object? sender, EventArgs e)
{
if (VirtualView is ISearchBar virtualView)
virtualView.IsFocused = true;
}

void OnEditingChanged(object? sender, EventArgs e)
{
if (Handler is SearchBarHandler handler)
{
handler.UpdateCancelButtonVisibility();
}
}

void OnEditingStopped(object? sender, EventArgs e)
{
if (VirtualView is ISearchBar virtualView)
virtualView.IsFocused = false;
}
}
}
}
8 changes: 4 additions & 4 deletions src/Core/src/Platform/iOS/MauiSearchBar.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ protected internal MauiSearchBar(NativeHandle handle) : base(handle)
// Native Changed doesn't fire when the Text Property is set in code
// We use this event as a way to fire changes whenever the Text changes
// via code or user interaction.
[UnconditionalSuppressMessage("Memory", "MEM0001", Justification = "FIXME: https://github.com/dotnet/maui/pull/16383")]
[UnconditionalSuppressMessage("Memory", "MEM0001", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")]
public event EventHandler<UISearchBarTextChangedEventArgs>? TextSetOrChanged;

public override string? Text
Expand All @@ -52,9 +52,9 @@ public override string? Text
}
}

[UnconditionalSuppressMessage("Memory", "MEM0001", Justification = "FIXME: https://github.com/dotnet/maui/pull/16383")]
[UnconditionalSuppressMessage("Memory", "MEM0001", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")]
internal event EventHandler? OnMovedToWindow;
[UnconditionalSuppressMessage("Memory", "MEM0001", Justification = "FIXME: https://github.com/dotnet/maui/pull/16383")]
[UnconditionalSuppressMessage("Memory", "MEM0001", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")]
internal event EventHandler? EditingChanged;

public override void WillMoveToWindow(UIWindow? window)
Expand All @@ -74,7 +74,7 @@ public override void WillMoveToWindow(UIWindow? window)
OnMovedToWindow?.Invoke(this, EventArgs.Empty);
}

[UnconditionalSuppressMessage("Memory", "MEM0003", Justification = "FIXME: https://github.com/dotnet/maui/pull/16383")]
[UnconditionalSuppressMessage("Memory", "MEM0003", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")]
void OnEditingChanged(object? sender, EventArgs e)
{
EditingChanged?.Invoke(this, EventArgs.Empty);
Expand Down
20 changes: 18 additions & 2 deletions src/Core/src/Platform/iOS/SearchBarExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,25 @@ public static class SearchBarExtensions
internal static UITextField? GetSearchTextField(this UISearchBar searchBar)
{
if (OperatingSystem.IsIOSVersionAtLeast(13))
{
return searchBar.SearchTextField;
else
return searchBar.GetSearchTextField();
}
Comment on lines 12 to +15
Copy link
Member Author

Choose a reason for hiding this comment

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

If iOS < 13 before, this just threw StackOverflowException? Let me know if I'm missing something, I ported a Swift example to C# instead.


// Search Subviews up to two levels deep
// https://stackoverflow.com/a/58056700
foreach (var child in searchBar.Subviews)
{
if (child is UITextField childTextField)
return childTextField;

foreach (var grandChild in child.Subviews)
{
if (grandChild is UITextField grandChildTextField)
return grandChildTextField;
}
}

return null;
}

internal static void UpdateBackground(this UISearchBar uiSearchBar, ISearchBar searchBar)
Expand Down
Loading