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

ViewHandler mapper extension methods won't reliably fire if mapper methods are replaced #6429

Closed
PureWeen opened this issue Apr 22, 2022 · 4 comments
Labels
s/triaged Issue has been reviewed s/try-latest-version Please try to reproduce the potential issue on the latest public version t/bug Something isn't working
Milestone

Comments

@PureWeen
Copy link
Member

PureWeen commented Apr 22, 2022

#4714 (comment)

If you write the following code

global::Microsoft.Maui.Handlers.ViewHandler.ViewMapper.AppendToMapping("BackgroundColor",
	(handler, view) =>
	{

	});

global::Microsoft.Maui.Handlers.ViewHandler.ViewMapper.AppendToMapping("Background",
	(handler, view) =>
	{

	});

These will not fire for Entry because the EntryHandler replaces the MapBackground mapping on its mapper which means the call never reaches ViewHandler. This also means if we end up adding mappings to for inherited views later on it will cause the users code to stop firing if they had attached to a base class previously

@PureWeen PureWeen added the s/verified Verified / Reproducible Issue ready for Engineering Triage label Apr 22, 2022
@PureWeen PureWeen added this to the 6.0.300 milestone Apr 22, 2022
@bakerhillpins
Copy link

bakerhillpins commented May 11, 2022

@PureWeen
Rather than write up a separate issue and link back to this I though it might be prudent to add to this issue since the end result is variability in Mapper function resolution. I've been trying to author a control that inherits and overrides the Microsoft.Maui.Controls.Label and found that it's not behaving as I'd expect with regard to the mapper and overriding.

For example: If I use the PropertyMapper constructor that takes a collection of IPropertyMappers:

	public PropertyMapper(params IPropertyMapper[] chained)

the order of the chained Mappers (arguments in the constructor) can effect which handler gets called when both mappers override the same key. This is due to the fact that the first handler short circuits the algo.

		else if (Chained is not null)
		{
			foreach (var ch in Chained)
			{
				var returnValue = ch.GetProperty(key);
				if (returnValue != null)
					return returnValue;
			}
		}

The Chained search algo above is duplicated in both the PropertyMapper.GetProperty(string key) and the PropertyMapper<TVirtualView, TViewHandler>.Add(string key, Action<TViewHandler, TVirtualView> action) methods. The Add method actually wraps up the inserted handler with this logic. It would seem that this duplication is unnecessary?

The indexer on the PropertyMapper<TVirtualView, TViewHandler> class also wraps up the stored handler in an empty delegate. If this extra delegate is necessary then the action returned from the GetProperty(key) call on the line above should be null checked as GetProperty can return null.

I.e. => action.Invoke(h, v)); should be: => action?.Invoke(h, v));

@mattleibow mattleibow modified the milestones: 6.0.300, 6.0-servicing Aug 29, 2022
@Redth Redth modified the milestones: 6.0-servicing, Backlog Aug 30, 2022
@ghost
Copy link

ghost commented Aug 30, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@samhouts samhouts removed the s/verified Verified / Reproducible Issue ready for Engineering Triage label Apr 5, 2023
@samhouts samhouts added the t/bug Something isn't working label Jul 31, 2023
@Zhanglirong-Winnie Zhanglirong-Winnie added s/triaged Issue has been reviewed s/try-latest-version Please try to reproduce the potential issue on the latest public version labels Dec 25, 2023
@ghost
Copy link

ghost commented Dec 25, 2023

Hi @PureWeen. We have added the "s/try-latest-version" label to this issue, which indicates that we'd like you to try and reproduce this issue on the latest available public version. This can happen because we think that this issue was fixed in a version that has just been released, or the information provided by you indicates that you might be working with an older version.

You can install the latest version by installing the latest Visual Studio (Preview) with the .NET MAUI workload installed. If the issue still persists, please let us know with any additional details and ideally a reproduction project provided through a GitHub repository.

This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@Zhanglirong-Winnie
Copy link

Verified this issue with Visual Studio Enterprise 17.9.0 Preview 2. Not repro this issue.

@ghost ghost closed this as completed Jan 1, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 1, 2024
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
s/triaged Issue has been reviewed s/try-latest-version Please try to reproduce the potential issue on the latest public version t/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants