dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.15k stars 1.74k forks source link

[Windows] BoolToVisibilityConverter returns the incorrect type #6868

Open ivan-todorov-progress opened 2 years ago

ivan-todorov-progress commented 2 years ago

Description

I am using the BoolToVisibilityConverter converter in UWP XAML: https://github.com/dotnet/maui/blob/main/src/Compatibility/Core/src/Windows/BoolToVisibilityConverter.cs.

It took me a few hours debugging and banging my head against the wall to figure out why it is not working. The converter implements the Microsoft.UI.Xaml.Data.IValueConverter interface and is supposed to convert a bool value to a Microsoft.UI.Xaml.Visibility value. The converter actually returns Microsoft.Maui.Visibility however. This makes it useless in UWP XAML, because it returns the wrong type and not applicable in MAUI XAML, as it implements an incompatible interface.

Steps to Reproduce

Run the following code in a MAUI application:

var converter = new Microsoft.Maui.Controls.Compatibility.Platform.UWP.BoolToVisibilityConverter();
var visibility = converter.Convert(true, typeof(bool), null, null);
var type = visibility.GetType();

System.Diagnostics.Debug.WriteLine($"Converted value type: {type}");
System.Diagnostics.Debug.Assert(visibility is Microsoft.UI.Xaml.Visibility);

Examine the type of the value returned from the converter.

Version with bug

Release Candidate 2 (current)

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

I have not tested on various versions of the different platforms.

Did you find any workaround?

The only workaround I have found is to implement my own converter which returns the correct type.

Relevant log output

No response

drasticactions commented 2 years ago

That was originally there for Xamarin.Forms, where it did a similar thing. AFAIK, this is intended for use in MAUI Xaml, so it's correct. It's meant to be cross-platform, and Microsoft.Maui.Visibility is the cross-platform implementation. This isn't an "Essential" API where it's meant for use with the core platform.

So I'm not sure how it's wrong here. If you want to make your BooleanToVisibilityConverter for WinUI, you should be able to do that on your own.

ghost commented 2 years ago

Hi @ivan-todorov-progress. We have added the "s/needs-info" label to this issue, which indicates that we have an open question for you before we can take further action. 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.

ivan-todorov-progress commented 2 years ago

@drasticactions The converter cannot be used in MAUI XAML, because it implements the wrong interface: Microsoft.UI.Xaml.Data.IValueConverter. For the converter to work in MAUI XAML it should implement Microsoft.Maui.Controls.IValueConverter instead. In its current state, the converter cannot be used in either MAUI XAML or Win UI XAML - it is a rather useless converter. ;-) This is what I have tried to explain in the bug's description.

I do not mind this TBH, as I have already implemented the correct converter myself. I was porting an existing Xamarin.Forms control to MAUI and realized the converter is no longer working. Please, do not kill the messenger! ;-) I am just pointing out a rather peculiar bug that was introduced after moving types around to different namespaces. It is up to you whether you want to fix it or delete the useless converter. Leaving the converter in its current state would confuse developers, as they would most likely try to use it and waste valuable time trying to figure out why it is not working.

mattleibow commented 2 years ago

@PureWeen I am wondering if we need this converter anymore. I believe it existed for the maui implementation, but I see it getting registered in the resources, but never actually accessed.

In fact, there are a few other ones in there that can probably go away now.

PureWeen commented 2 years ago

@mattleibow @ivan-todorov-progress

yea, this was mostly likely just a mistake when moving code around.

We're not using it internally and AFAICT we weren't using it in FORMS either

Right now it's sitting in our compatibility project which probably won't be auto included with NET7 so we might want to just move that converter into Microsoft.Maui.Controls.Platform and return the correct Visibility property.

It doesn't seem worth breaking users to not include this

VincentBu commented 2 years ago

repro with vs main (32623.270)

ghost commented 2 years ago

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.