-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
NavigationBarColors from NavigationPage not changing on AppTheme changing - fix #24766
base: main
Are you sure you want to change the base?
Conversation
Hey there @kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
@@ -587,6 +587,8 @@ protected virtual void OnChildAdded(Element child) | |||
{ | |||
child.SetParent(this); | |||
|
|||
child.ApplyBindings(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was needed after this PR: #24553
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I removed on purpose in my PR, may you explain why this is needed?
SetParent
propagates the BindingContext
which will automatically call ApplyBindings
, while if a Binding
is created with new Binding("Parent", source: self)
it will be triggered by the change of Parent
property.
Also note ApplyBindings
now will apply all bindings except BindingContext
property.
This change you put here will break a unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kubaflo I tested your (reverted) UI test page and added this:
<ContentPage.ToolbarItems>
<ToolbarItem>
<ToolbarItem.IconImageSource>
<FontImageSource Glyph=""
x:Name="TheIcon"
Color="{AppThemeBinding Light=Black, Dark=White}"
FontFamily="FA"/>
</ToolbarItem.IconImageSource>
</ToolbarItem>
</ContentPage.ToolbarItems>
<VerticalStackLayout>
<Label Text="{Binding Color, Source={x:Reference TheIcon}}" />
You can clearly see that the color changes even without this child.ApplyBindings()
, which means that the binding is working correctly and probably the real issue is somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the best way to achieve what you want is to act on (Primary|Secondary)ToolbarItem
and where item.PropertyChanged += OnPropertyChanged;
is subscribed, also subscribe to item.IconImageSource.PropertyChanged += OnIconPropertyChanged;
.. something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@albyrock87 This PR worked before this #24553 and your changes broke it😅 I'm not saying it is bad, but maybe something worth looking into... It's because if it broke a fix in this pr then maybe it breaks something more important that already exists in the codebase🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly why I've investigated your report in depth, and now I'm completely sure that the issue is in the navigation renderer and not in Element.
That line on Element was basically applying the Bindings twice, which had a side effect of retriggering the background property changed, which (thanks to your PR) causes recreation of the toolbar item.
I think re-creating the item is simply wrong considering that only the color of an icon changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got you but... I've just checked and it has broken Android, which I didn't touch.
Before
Screen.Recording.2024-09-15.at.20.13.25.mov
After:
Screen.Recording.2024-09-15.at.20.32.37.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably fix them all from inside ToolbarItem by triggering property changed of IconImageSource when IconImageSource.SourceChanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm this fixes everything (MenuItem.cs
).
/// <summary>Bindable property for <see cref="IconImageSource"/>.</summary>
public static readonly BindableProperty IconImageSourceProperty = BindableProperty.Create(nameof(IconImageSource), typeof(ImageSource), typeof(MenuItem), default(ImageSource),
propertyChanged: (bindable, oldValue, newValue) => {
if (oldValue is ImageSource oldSource)
{
oldSource.SourceChanged -= ((MenuItem)bindable).OnImageSourceSourceChanged;
}
((MenuItem)bindable).AddRemoveLogicalChildren(oldValue, newValue);
if (newValue is ImageSource newSource)
{
newSource.SourceChanged += ((MenuItem)bindable).OnImageSourceSourceChanged;
}
}
);
private void OnImageSourceSourceChanged(object sender, EventArgs e)
{
OnPropertyChanged(IconImageSourceProperty.PropertyName);
}
Now this is not a "clean" solution, but it avoids subscribing to SourceChanged
on each platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, it works and this solution is the same as in imageCell.cs
which leads me to believe that it is the right one!
Co-Authored-By: Alberto Aldegheri <1423005+albyrock87@users.noreply.github.com>
This approach is just like the one used in
ImageCell.cs
https://github.com/kubaflo/maui/blob/0a6df3601695f0ca84e63dd70a06877a14197b6f/src/Controls/src/Core/Cells/ImageCell.cs#L9-L13
Issues Fixed
@StephaneDelcroix I've opened this PR for you to have a look after your PR: #24688. However, feel free to close it if you want :)
Fixes #23195
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-14.at.00.37.55.mp4
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-14.at.02.17.26.mp4