Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

[Feature] Add static functions to converters in provided IValueConverters #4558

Closed
Avid29 opened this issue May 16, 2022 · 15 comments
Closed
Labels
feature request 📬 A request for new changes to improve functionality labs 🧪 Marks an issue/PR involved with Toolkit Labs

Comments

@Avid29
Copy link
Contributor

Avid29 commented May 16, 2022

Describe the problem

Adding static functions to the converters would allow static function binding instead of using actual Converters. Actual converters could still be used where appropriate though.

Describe the solution

Add a static functions that can beused instead of the IValueConverter

Alternatives

Use the converter as a StaticResource value converter

Additional info

No response

Help us help you

Yes, I'd like to be assigned to work on this item.

@Avid29 Avid29 added the feature request 📬 A request for new changes to improve functionality label May 16, 2022
@ghost
Copy link

ghost commented May 16, 2022

Hello, 'Avid29! Thanks for submitting a new feature request. I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future!

@michael-hawker michael-hawker added the labs 🧪 Marks an issue/PR involved with Toolkit Labs label May 17, 2022
@michael-hawker
Copy link
Member

michael-hawker commented May 17, 2022

Huh, thought we had something tracking this already. I know it's been on the backlog for a while to do something like this.

I know it's trickier too as x:Bind functions need to match source/destination types exactly to work, so you end up needing more combinations of functions to accept different types and be used in different places.

I would imagine we would want one general helper class vs. providing static methods on each value converter. So you can just get intellisense and do something like {x:Bind converters:Functions.BoolToOrientation(MyPropName)}...

Could be a good non-layout control candidate for Labs. Would be nice to think of a list of the types of converters we'd want to have and their signatures and such as an API proposal.

We're still working on some of the technical aspects of testing in Labs (trying to get more UI oriented tests setup). Once that's done and we're ready to start adding more experiences, I'll reach out to you in a week or two-ish.

FYI @Arlodotexe

@Avid29
Copy link
Contributor Author

Avid29 commented May 18, 2022

What I had in mind was simply a refined set of converters that included static methods. Here's an example

public class IsNullConverter : IValueConverter
{
    public static bool Convert(object? value) => value is null;
    
    public object Convert(object? value, Type targetType, object parameter, string language)
        => Convert(value);
    
    public object ConvertBack(object? value, Type targetType, object parameter, string language)
        => throw new NotImplementedException();
}

(A static ConvertBack could be provided where appropriate as well)

This both provides consistency between the IValueConverter based converters and static functions, it also makes convert back methods easier to implement. This would also help keep track of them together and ensure both converter methods are supported.

I do however see how the single class is compelling from a convenience basis.

@Avid29
Copy link
Contributor Author

Avid29 commented May 18, 2022

Here is also a more complicated example with a convert back, the static method takes to arguments and the IValueConverter has a property.

public class DateTimeFormatConverter : IValueConverter
{
    public static string Convert(DateTime dateTime, string format)
        => dateTime.ToString(format);

    public static DateTime ConvertBack(string dateTime)
        => DateTime.Parse(dateTime);

    public string Format { get; set; }

    public object Convert(object value, Type targetType, object parameter, string language)
    {
        if (value is DateTime dateTime)
        {
            return Convert(dateTime, Format);
        }

        return null;
    }

    public object ConvertBack(object value, Type targetType, object parameter, string language)
    {
        if (value is string dateTime)
        {
            return ConvertBack(dateTime);
        }

        return null;
    }
}

@Arlodotexe
Copy link
Member

Arlodotexe commented May 23, 2022

I would imagine we would want one general helper class vs. providing static methods on each value converter. So you can just get intellisense and do something like {x:Bind converters:Functions.BoolToOrientation(MyPropName)}...

I think you'd already get intellisense for this, since you have to use the converters: namespace? If that's the case, I'd want to keep the static method in each relevant converter to help keep things clean and maintainable.

@Arlodotexe
Copy link
Member

Arlodotexe commented May 23, 2022

Could be a good non-layout control candidate for Labs. Would be nice to think of a list of the types of converters we'd want to have and their signatures and such as an API proposal.

If we're bringing this to Labs vs improving what's currently in the toolkit, I think we should look at creating some kind of ChainConverter as well. That way we could avoid creating converters that are combinations of existing simpler converters.

We can take inspiration from this existing ChainConverter project for Xaml, but getting that to work with x:Bind is a different story and would require source generators.

For example, an InverseNullToVisibilityConverter could easily be NullToBoolConverter + InverseBoolConverter + BoolToVisibilityConverter.

It would add significant flexibility to what you can do with converters, while reducing maintenance overhead and namespace clutter.

@michael-hawker
Copy link
Member

In general probably solving the converter/x:Bind debacle is best done with source generators somehow. We're never going to be able to account for all the possible conversions folks are going to want to do, see microsoft/microsoft-ui-xaml#2407.

What if we have a single static class which defines the methods for all the translations we want to support that can be used by x:Bind directly? Then have a source generator which spits out equivalent converters for them?

public static class Converters {

    [MakeConverter]
    public Visibility OrientationToVisibility(Orientation value, bool negate) => negate ?
        switch value {
           Horizontal => Visible,
           Vertical => Collapsed,
        } :
        switch value {
          Horizontal => Collaspsed,
          Vertical => Visible, 
        };
}

Or we might want to do a T4 thing where we have a pattern for types to and from or something? Like a lot of the toolkit converters are generic with our use of dependency properties to be set in XAML ahead of time, which isn't something as supportable with x:Bind.

@Arlodotexe
Copy link
Member

Arlodotexe commented May 24, 2022

That's quite a bit different than what I had in mind. If we create chain converters, we'd want it to be something the user can declare in XAML.

Taking the ChainConverter mentioned earlier as an example:

<conv:ChainConverter x:Key="OppositeBoolToVisibilityConverter">
    <conv:ChainConverter.Chain>
        <conv:BoolNegationConverter />
        <conv:BoolToVisibilityConverter />
    </conv:ChainConverter.Chain>
</conv:ChainConverter>

<TextBlock Visibility="{Binding IsDisabled, Converter={StaticResource OppositeBoolToVisibilityConverter}}" />

As you can see, this is pretty easy to do with classic IValueConverters since the input/output is always object.

However, we want to also be able to declare these in Xaml and use them with x:Bind, which is tricky because it means the type must match.

Luckily, source generators allow you to see the generated XAML code, so we can look for any ChainConverter, check if they have a static method we can use for x:Bind, and just generate a method in code-behind that (with type safety), chains them together and outputs the new method somewhere we can x:Bind to.

Putting it all together, it would look like this:

<!-- Keep the non-xbind syntax for convenience -->
<conv:ChainConverter x:Key="OppositeBoolToVisibilityConverter">
    <conv:ChainConverter.Chain>
        <conv:BoolNegationConverter />
        <conv:BoolToVisibilityConverter />
    </conv:ChainConverter.Chain>
</conv:ChainConverter>

<TextBlock Visibility="{x:Bind generated:OppositeBoolToVisibilityConverter.Execute(IsDisabled), Mode=OneWay}" />

@Avid29
Copy link
Contributor Author

Avid29 commented May 24, 2022

@Arlodotexe that's not necessary.

{x:Bind foo:Foo.For(bar:Bar.Bar(target))} is acceptable binding.

Besides that, any generated static method seems far less sensible than merely expecting someone to write their own static method.

@michael-hawker
Copy link
Member

@Arlodotexe that's not necessary.

{x:Bind foo:Foo.For(bar:Bar.Bar(target))} is acceptable binding.

Besides that, any generated static method seems far less sensible than merely expecting someone to write their own static method.

@Avid29 that shouldn't be a valid x:Bind statement, unless they fixed microsoft/microsoft-ui-xaml#2407 in WinUI 3 and didn't tell anyone?

You can't nest x:Bind statements in UWP, that's the big limitation that's made this so hard to get done practically in the toolkit for so long.

@Arlodotexe
Copy link
Member

{x:Bind foo:Foo.For(bar:Bar.Bar(target))} is acceptable binding.

That's definitely new to me! A bit of a wacky syntax, but if it works already then no need to set up a source generator 😄

I want to reiterate that keeping just the simple converters in the Toolkit while allowing chaining them together would add significant flexibility, while reducing maintenance overhead and namespace clutter.

If we're revamping our converters, this is something we should keep in mind

@Avid29
Copy link
Contributor Author

Avid29 commented May 26, 2022

I was not aware nested x:Bind functions were broken. That really sucks.

I think the source generator approach is really nice, though I think ValueConverterAttribute is a better name than MakeConverterAttribute.

Here's my draft list of converters

  • Object
    • EqualityConverter (One-Way)
    • IsNullConverter (One-Way)
  • Numeric
    • IsLessThanConverter (One-Way)
    • IsLessThanOrEqualConverter (One-Way)
    • IsGreaterThanConverter (One-Way)
    • IsGreaterThanOrEqualConverter (One-Way)
    • DoubleToThickness (One-Way)
  • Visibility
    • BoolToVisibilityConverter (Two-Way)
  • String/Collection
    • IsNullOrEmptyConverter (One-Way)
  • String
    • IsWhiteSpaceConverter (One-Way)
    • CharacterCasingConverter (One-Way)
  • DateTime/TimeSpan
    • DateTimeFormatConverter (Two-Way)
    • TimeSpanFormatConverter (Two-Way)

@michael-hawker
Copy link
Member

To be clear, nested x;Bind functions were never working; it's a feature request for the future. 😉

@michael-hawker
Copy link
Member

@Avid29 also note this issue which is a thorn for these things as well: microsoft/microsoft-ui-xaml#5514

@Avid29
Copy link
Contributor Author

Avid29 commented May 31, 2022

Yeah. I'm aware and it's pretty frustrating. We could have source generation for having methods that return bool also have a version that returns visibility I guess.

@CommunityToolkit CommunityToolkit locked and limited conversation to collaborators Jul 28, 2022
@LalithaNadimpalli LalithaNadimpalli converted this issue into discussion #4617 Jul 28, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
feature request 📬 A request for new changes to improve functionality labs 🧪 Marks an issue/PR involved with Toolkit Labs
Projects
None yet
Development

No branches or pull requests

3 participants