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

MenuItem gets updated when IconImageSource changes - Fix 17210 #18119

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jm-goldacker
Copy link

Description of Change

I added an event handler method that subscribes to the SourceChanged event of the MenuItem's IconImageSource.

When this event occurs, subscribers will be notified that the IconImageSourceProperty of the MenuItem changed - so the UI will update.

Issues Fixed

Fixes #17210

examples from public reproduction project repository, before swapping icons:
image

after swapping icons:
image

The examples can be found in the Maui.Controls.Sample project.

@jm-goldacker jm-goldacker requested a review from a team as a code owner October 18, 2023 20:19
@ghost ghost added the community ✨ Community Contribution label Oct 18, 2023
@ghost
Copy link

ghost commented Oct 18, 2023

Hey there @jm-goldacker! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@jm-goldacker
Copy link
Author

The issue mentions "Binding does not work with glyph."

This is the corresponding code in the reproduction project:
<MenuFlyoutItem.IconImageSource> <FontImageSource Glyph="{Binding Icon1}" FontFamily="IconsFont" Color="Blue"/> </MenuFlyoutItem.IconImageSource>

That code will not render the glyph. The problem is the binding.

I solved it by referencing the binding source:
<MenuFlyoutItem.IconImageSource> <FontImageSource Glyph="{Binding BindingContext.Icon1, Source={x:Reference menuBarSamplePage}}" FontFamily="IconsFont"/> </MenuFlyoutItem.IconImageSource>

Is it intended to work like that?
If not, I will still have to put some work into this issue 😅

@jm-goldacker jm-goldacker changed the title Fix 17210 MenuItem gets updated when IconImageSource changes - Fix 17210 Oct 19, 2023
@samhouts samhouts added this to the Under Consideration milestone Oct 20, 2023
@Eilon Eilon added the legacy-area-desktop Windows / WinUI / Project Reunion & Mac Catalyst / macOS specifics (Menus & other Controls)) label Nov 8, 2023
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Dec 7, 2023
@Eilon Eilon added t/desktop The issue relates to desktop scenarios (MacOS/MacCatalyst/Windows/WinUI/WinAppSDK) area-controls-menubar Desktop MenuBarItems and removed legacy-area-desktop Windows / WinUI / Project Reunion & Mac Catalyst / macOS specifics (Menus & other Controls)) labels May 10, 2024
@PureWeen PureWeen removed this from the Under Consideration milestone Aug 2, 2024
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! The fix looks right, but I think the issue you pointed out with the weird binding context may be because we are not properly propagating the context to menu items.

Are you still able to look at this PR? I know it has been a day or year... Let me know. I will try and be more responsive. Back in 2023, things were more crazy - now we got time to look at PRs :P

Some questions for the binding issue:

Let me know if you still feel like working on this issue.

@mattleibow
Copy link
Member

I had a quick look at a rebase, and saw this conflict:

		public static readonly BindableProperty IconImageSourceProperty = BindableProperty.Create(nameof(IconImageSource), typeof(ImageSource), typeof(MenuItem), default(ImageSource),
<<<<<<< fix-17210
			propertyChanging: OnImageSourceChanging, propertyChanged: OnImageSourceChanged);
=======
			propertyChanged: (bindable, oldValue, newValue) =>
			{
				((MenuItem)bindable).AddRemoveLogicalChildren(oldValue, newValue);
			}
		);
>>>>>>> main

Basically, the code in main now propagates the binding context. I think if you were to rebase, the old bindings would work and the issue would be to just add a UI test.

@jm-goldacker
Copy link
Author

Hi,
thanks for looking into the PR.

I would indeed like to continue working on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-menubar Desktop MenuBarItems community ✨ Community Contribution stale Indicates a stale issue/pr and will be closed soon t/desktop The issue relates to desktop scenarios (MacOS/MacCatalyst/Windows/WinUI/WinAppSDK)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Binding IconImageSource on MenuFlyoutItem does not update on runtime.
5 participants