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

[C] use ResourcesChanged to propagate Theme #21229

Merged
merged 3 commits into from
Mar 20, 2024
Merged

[C] use ResourcesChanged to propagate Theme #21229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2982b41
[C] use ResourcesChanged to propagate Theme
StephaneDelcroix Mar 15, 2024
4dd87d7
fix test
StephaneDelcroix Mar 19, 2024
88e554e
use a const string
StephaneDelcroix Mar 19, 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
2 changes: 1 addition & 1 deletion Microsoft.Maui-dev.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "UITest.Appium", "src\TestUt
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "UITest.NUnit", "src\TestUtils\src\UITest.NUnit\UITest.NUnit.csproj", "{A307B624-48D4-494E-A70D-5B3CDF6620CF}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Controls.SourceGen.UnitTests", "src\Controls\tests\SourceGen.UnitTests\Conrtrols.SourceGen.UnitTests\Controls.SourceGen.UnitTests.csproj", "{06747B55-91DB-47F5-B7A2-56526C28A0D3}"
Copy link
Contributor

Choose a reason for hiding this comment

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

😮

Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Controls.SourceGen.UnitTests", "src\Controls\tests\SourceGen.UnitTests\Controls.SourceGen.UnitTests\Controls.SourceGen.UnitTests.csproj", "{06747B55-91DB-47F5-B7A2-56526C28A0D3}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SourceGen.UnitTests", "src\Controls\tests\SourceGen.UnitTests\SourceGen.UnitTests.csproj", "{BC7F7C82-694F-4B97-86FC-273FB3FACA25}"
EndProject
Expand Down
58 changes: 34 additions & 24 deletions src/Controls/src/Core/AppThemeBinding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,38 @@ namespace Microsoft.Maui.Controls
{
class AppThemeBinding : BindingBase
{
public const string AppThemeResource = "__MAUI_ApplicationTheme__";
class AppThemeProxy : Element
{
public AppThemeProxy(Element parent, AppThemeBinding binding)
Copy link
Member

Choose a reason for hiding this comment

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

I see the AppThemeBinding uses a weak reference for the target: WeakReference<BindableObject> _weakTarget; Is this now creating a string reference that might cause issues? Maybe the AppThemeBinding does not need to be weak?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the proxy might prevent the AppThemeBinding to GC, but the AppThemeBinding doesn't hold a strong ref to the BO.
in this case, I trust the GC to do the job

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like I was wrong

{
_parent = parent;
Binding = binding;
this.SetDynamicResource(AppThemeProperty, AppThemeResource);
((IElementDefinition)parent).AddResourcesChangedListener(OnParentResourcesChanged);
}

public static BindableProperty AppThemeProperty = BindableProperty.Create("AppTheme", typeof(AppTheme), typeof(AppThemeBinding), AppTheme.Unspecified,
Copy link
Member

Choose a reason for hiding this comment

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

Will it be more expensive to have a separate proxy Element object with a single extra property vs just adding this to maybe the base Element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can't add that to the base Element, as an element might need have multiple AppThemebindings, and then multiples proxies. moving that to the element will require some sort of event subscription, and that's what we're avoiding here

propertyChanged: (bindable, oldValue, newValue) => ((AppThemeProxy)bindable).OnAppThemeChanged());
readonly Element _parent;

public void OnAppThemeChanged()
{
Binding.Apply(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about the performance impact, especially on CollectionView scrolling. Could we perhaps have an UITest with this example #18505 where we can display the minimum fps in a Label by scrolling and verifying that the value is greater than n (where n is value higher than before)?

Copy link
Member

@mattleibow mattleibow Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We may need @jonathanpeppers to run this PR in a release app with profiling. Our UI tests have a lot of overhead and appium things running.

}

public AppThemeBinding Binding { get; }

public void Unsubscribe()
{
((IElementDefinition)_parent).RemoveResourcesChangedListener(OnParentResourcesChanged);
}
}

WeakReference<BindableObject> _weakTarget;
BindableProperty _targetProperty;
bool _attached;
SetterSpecificity specificity;
AppThemeProxy _appThemeProxy;

internal override BindingBase Clone()
{
Expand All @@ -34,7 +62,6 @@ internal override void Apply(bool fromTarget)
{
base.Apply(fromTarget);
ApplyCore();
SetAttached(true);
}

internal override void Apply(object context, BindableObject bindObj, BindableProperty targetProperty, bool fromBindingContextChanged, SetterSpecificity specificity)
Expand All @@ -44,12 +71,15 @@ internal override void Apply(object context, BindableObject bindObj, BindablePro
base.Apply(context, bindObj, targetProperty, fromBindingContextChanged, specificity);
this.specificity = specificity;
ApplyCore(false);
SetAttached(true);
_appThemeProxy = new AppThemeProxy(bindObj as Element, this);

}

internal override void Unapply(bool fromBindingContextChanged = false)
{
SetAttached(false);
_appThemeProxy?.Unsubscribe();
_appThemeProxy = null;

base.Unapply(fromBindingContextChanged);
_weakTarget = null;
_targetProperty = null;
Expand All @@ -62,7 +92,6 @@ void ApplyCore(bool dispatch = false)
{
if (_weakTarget == null || !_weakTarget.TryGetTarget(out var target))
{
SetAttached(false);
return;
}

Expand Down Expand Up @@ -147,24 +176,5 @@ target is VisualElement ve &&
_ => _isLightSet ? Light : Default,
};
}

void SetAttached(bool value)
{
var app = Application.Current;
if (app != null && _attached != value)
{
if (value)
{
// Going from false -> true
app.RequestedThemeChanged += OnRequestedThemeChanged;
}
else
{
// Going from true -> false
app.RequestedThemeChanged -= OnRequestedThemeChanged;
}
_attached = value;
}
}
}
}
10 changes: 10 additions & 0 deletions src/Controls/src/Core/Application/Application.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,15 @@ public Page? MainPage

_singleWindowMainPage = value;

if (value is not null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fixes another bug. resources changed at application level weren't propagated to MainPage

Copy link
Member

Choose a reason for hiding this comment

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

I think if you are just using MainPage, then you get parented to a window during the creating of the UI - so once the app is ready, it calls CreateWindow and assigns the MainPage to the window.

The _singleWindowMainPage is just a placeholder while the app is still being constructed. Changing MainPage later literally just replaces the content of the first window:

Windows[0].Page = value;

What you have observed might just be because MainPage really is a delayed construction and assigned. Now this is to say it may be fine what you are doing here, but will it result in a double listener registration once the window is created and the MainPage parented to the window?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe, but that doesn't work in unit tests

{
OnParentResourcesChanged(this.GetMergedResources());
OnParentResourcesChanged([new KeyValuePair<string, object>(AppThemeBinding.AppThemeResource, _lastAppTheme)]);
((IElementDefinition)this).AddResourcesChangedListener(value.OnParentResourcesChanged);
} else if (MainPage is not null)
{
((IElementDefinition)this).RemoveResourcesChangedListener(MainPage.OnParentResourcesChanged);
}
if (Windows.Count == 1)
{
Windows[0].Page = value;
Expand Down Expand Up @@ -243,6 +252,7 @@ void TriggerThemeChangedActual()
_themeChangedFiring = true;
_lastAppTheme = newTheme;

OnParentResourcesChanged([new KeyValuePair<string, object>(AppThemeBinding.AppThemeResource, newTheme)]);
_weakEventManager.HandleEvent(this, new AppThemeChangedEventArgs(newTheme), nameof(RequestedThemeChanged));
}
finally
Expand Down
2 changes: 2 additions & 0 deletions src/Controls/src/Core/Shell/Shell.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,10 +827,12 @@ private protected override void OnHandlerChangingCore(HandlerChangingEventArgs a
return;

if (args.NewHandler == null)
#pragma warning disable CS0618 // Type or member is obsolete
Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, it's not anymore in this version of the PR, but maybe it should

Application.Current.RequestedThemeChanged -= OnRequestedThemeChanged;

if (args.NewHandler != null && args.OldHandler == null)
Application.Current.RequestedThemeChanged += OnRequestedThemeChanged;
#pragma warning restore CS0618 // Type or member is obsolete
}

private void OnRequestedThemeChanged(object sender, AppThemeChangedEventArgs e)
Expand Down
7 changes: 7 additions & 0 deletions src/Controls/tests/Core.UnitTests/AppThemeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public void ThemeChangeUsingSetAppThemeColor()
{
Text = "Green on Light, Red on Dark"
};
app.LoadPage(new ContentPage {Content = label});
Copy link
Contributor Author

@StephaneDelcroix StephaneDelcroix Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

apptheme propagation REQUIRES that the control is parented, which is fine, as otherwise it wouldn't be on screen


label.SetAppThemeColor(Label.TextColorProperty, Colors.Green, Colors.Red);
Assert.Equal(Colors.Green, label.TextColor);
Expand All @@ -46,11 +47,14 @@ public void ThemeChangeUsingSetAppThemeColor()
[Fact]
public void ThemeChangeUsingSetAppTheme()
{

var label = new Label
{
Text = "Green on Light, Red on Dark"
};

app.LoadPage(new ContentPage {Content = label});

label.SetAppTheme(Label.TextColorProperty, Colors.Green, Colors.Red);
Assert.Equal(Colors.Green, label.TextColor);

Expand All @@ -66,6 +70,7 @@ public void ThemeChangeUsingSetBinding()
{
Text = "Green on Light, Red on Dark"
};
app.LoadPage(new ContentPage {Content = label});

label.SetBinding(Label.TextColorProperty, new AppThemeBinding { Light = Colors.Green, Dark = Colors.Red });
Assert.Equal(Colors.Green, label.TextColor);
Expand All @@ -82,6 +87,7 @@ public void ThemeChangeUsingUserAppTheme()
{
Text = "Green on Light, Red on Dark"
};
app.LoadPage(new ContentPage {Content = label});

label.SetAppThemeColor(Label.TextColorProperty, Colors.Green, Colors.Red);
Assert.Equal(Colors.Green, label.TextColor);
Expand Down Expand Up @@ -225,6 +231,7 @@ public void NullApplicationCurrentFallsBackToEssentials()
{
Text = "Green on Light, Red on Dark"
};
app.LoadPage(new ContentPage {Content = label});

label.SetAppThemeColor(Label.TextColorProperty, Colors.Green, Colors.Red);

Expand Down
15 changes: 9 additions & 6 deletions src/Controls/tests/Core.UnitTests/DynamicResourceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,9 @@ public void SettingResourceTriggersValueChanged()
var label = new Label();
label.SetDynamicResource(Label.TextProperty, "foo");
Assert.Equal(Label.TextProperty.DefaultValue, label.Text);
label.Resources = new ResourceDictionary();
label.Resources.Add("foo", "FOO");
label.Resources = new ResourceDictionary {
{ "foo", "FOO" }
};
Assert.Equal("FOO", label.Text);
}

Expand All @@ -126,8 +127,9 @@ public void AddingAResourceDictionaryTriggersValueChangedForExistingValues()
[Fact]
public void ValueChangedTriggeredOnSubscribeIfKeyAlreadyExists()
{
var label = new Label();
label.Resources = new ResourceDictionary { { "foo", "FOO" } };
var label = new Label {
Resources = new ResourceDictionary { { "foo", "FOO" } }
};
Assert.Equal(Label.TextProperty.DefaultValue, label.Text);
label.SetDynamicResource(Label.TextProperty, "foo");
Assert.Equal("FOO", label.Text);
Expand All @@ -136,8 +138,9 @@ public void ValueChangedTriggeredOnSubscribeIfKeyAlreadyExists()
[Fact]
public void RemoveDynamicResourceStopsUpdating()
{
var label = new Label();
label.Resources = new ResourceDictionary { { "foo", "FOO" } };
var label = new Label {
Resources = new ResourceDictionary { { "foo", "FOO" } }
};
Assert.Equal(Label.TextProperty.DefaultValue, label.Text);
label.SetDynamicResource(Label.TextProperty, "foo");
Assert.Equal("FOO", label.Text);
Expand Down
1 change: 1 addition & 0 deletions src/Controls/tests/Xaml.UnitTests/Issues/Maui16538.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public void VSMandAppTheme([Values(false, true)] bool useCompiledXaml)

Application.Current.UserAppTheme = AppTheme.Dark;
var page = new Maui16538(useCompiledXaml);
Application.Current.MainPage = page;
Button button = page.button0;
Assert.That(button.BackgroundColor, Is.EqualTo(Color.FromHex("404040")));
button.IsEnabled = true;
Expand Down
Loading