AvaloniaUI / Avalonia

Develop Desktop, Embedded, Mobile and WebAssembly apps with C# and XAML. The most popular .NET UI client technology
https://avaloniaui.net
MIT License
25.59k stars 2.21k forks source link

Custom Converter stop working at nightly versions #9926

Open DmitriyYukhanov opened 1 year ago

DmitriyYukhanov commented 1 year ago

Description:

I have a IValueConverter which works just fine at Avalonia 11 preview.4 but it stops working somewhere in between 11.0.999-cibuild0026023-beta and 11.0.999-cibuild0026698-beta and still not working at latest nightly. It feels like a regression or some non-obvious change in styling / binding / APIs, requiring me to somehow update my code in a non-obvious way.

Steps to reproduce:

I can't share the converter code on public, thus can provide only abstracted steps to follow:

  1. Add MyConverter : IValueConverter to the project.
  2. Declare it as StaticResource in Application.Resources, i.e. <ns:MyConverter x:Key="MyConverter"/>.
  3. Add this converter to Opacity property of any Visual with class myClass, i.e.:

    <Style Selector=":is(Visual).myClass">
    <Setter Property="Transitions">
        <Transitions>
            <DoubleTransition Property="Opacity" Duration="0:0:1"/>
        </Transitions>
    </Setter>
    
    <Setter Property="Opacity">
        <Binding Converter="{StaticResource MyConverter}"/>
    </Setter>
    </Style>
  4. Put this Style Selector into the Application.Styles (I did this using <StyleInclude Source="/Styles/MyStyles.axaml" />).
  5. Add class myClass to any Visual and change its Opacity using Binding: <Image Classes="spinner myClass" Opacity="{Binding IsBusy}"/>

MyConverter should convert bool to double in order to test the behavior, and bool IsBusy should be set to true first and change to false after a few seconds.

See it doesn't change Opacity in nightly builds since 11.0.999-cibuild0026698-beta and at least till the 11.0.999-cibuild0028309-beta (currently latest available), while it working fine and change the Opacity in 11 preview.4 and 11.0.999-cibuild0026023-beta nightly.

Expected behavior Opacity changes using declared Transition according to the converted IsBusy -> 1 value.

Environment:

maxkatz6 commented 1 year ago

I am not sure if this code ever supposed to work. Seems like a misuse of some features in an unexpected way.

    <Setter Property="Opacity">
        <Binding Converter="{StaticResource MyConverter}"/>
    </Setter>

Here binding will use DataContext from the Visual control. An object that has IsBusy property in it, your view model I suppose. Converter won't convert anything this way, as it expects boolean.

<Image Classes="spinner myClass" Opacity="{Binding IsBusy}"/>

While here you use "{Binding IsBusy}" to set the binding on the property itself and not DataContext. Not to mention, local value binding will have higher priority than any style (except animations). So, style setter should never be applied, nor converter be executed.

Not to mention it was type unsafe situation, when you set boolean binding to the double property. Relying on existence of style, which might not be applied all the time. With compiled bindings enabled it would show compile time errors as well.

I have no idea how it worked before if it did.

DmitriyYukhanov commented 1 year ago

Greetings @maxkatz6

Thank you for looking into this issue.

Here binding will use DataContext from the Visual control.

Ah, so it doesn't simply set a "default" converter to any Opacity bindings with this line? That's what I thought it does, and that's how it looked to me when it worked like this (all Visuals with myClass triggered MyConverter when Opacity was set using boolean binding).

Converter won't convert anything this way, as it expects boolean.

It actually works and returns decimal value based on boolean input, simply like this:

if (value is bool input)
    return (double)(input ? 1 : 0);

I mean, when I set bool value, my converter kicks in and converts it to the decimal before it gets set to the Opacity property.

Not to mention, local value binding will have higher priority than any style (except animations). So, style setter should never be applied, nor converter be executed.

I had a feeling that Style just sets binding's Converter property and doesn't affect Path if I don't set Path from the Style setter, and that's how it actually works, thus I had a feeling it's true.

Not to mention it was type unsafe situation, when you set boolean binding to the double property. Relying on existence of style, which might not be applied all the time. With compiled bindings enabled it would show compile time errors as well.

Isn't it supposed to be a different type when you're using a Converter? I have compiler bindings enabled and have no errors.

I have no idea how it worked before if it did.

Please take a look at this simplified repro case to see how it works in current 11.0.0 Preview 4 and stop working in nightly builds:

https://user-images.githubusercontent.com/883587/211161712-1695ca17-6c6c-4ad3-978f-f810d0c88def.mp4

MyApp.zip

It would be great to know if it has really something not intended and thus not supported in new release.

maxkatz6 commented 1 year ago

Ah, so it doesn't simply set a "default" converter to any Opacity bindings with this line? I had a feeling that Style just sets binding's Converter property and doesn't affect Path

No, by setting property binding as a value, you actually change value of a property. There is no "default" converter that can be changed with styles.

Please take a look at this simplified repro case to see how it works in current 11.0.0 Preview 4 and stop working in nightly builds:

Same problems.

Opacity="{Binding IsBig}"

Won't work, because "IsBig" is not a double.

<Binding Converter="{StaticResource OpacityConverter}"/>

Will be ignored, because Opacity already has a value with higher priority ({Binding IsBig}). If you remove Opacity="{Binding isBig}", then this value in the style will be applied, but converter will return a binding error, because input is not a bool.

In 11.0-preview4 it works even if you remove style with converter value completely. So, I assume there was some kind of implicit conversion from bool to 1/0 double value.

maxkatz6 commented 1 year ago

Although, changing version number to "11.0.999-cibuild0028420-beta" or "11.0.999-cibuild0026698-beta" doesn't break it for me. Even in release, it all works.

maxkatz6 commented 1 year ago

For the context, I found that bool to double implicit conversion is done here: https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Base/Utilities/TypeUtilities.cs#L202

rabbitism commented 1 year ago

For the context, I found that bool to double implicit conversion is done here: https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Base/Utilities/TypeUtilities.cs#L202

Can you also help to review my geometry converter PR? I think there is implicit conversion in compile phase but I'm not sure.

DmitriyYukhanov commented 1 year ago

Thanks a lot for coming back with updates @maxkatz6 ! The implicit bool > decimal conversion made my day, really glad Avalonia has it, thanks for pointing me out.

I'm glad I was wrong the problem was caused by converter, but I finally found what broke the behavior for me in new nightly versions. Turns out, it were the conditional selectors, like this:

Selector=":is(InputElement).fade[Opacity=0]

Basically, I'm using it to turn elements on and off depending on their opacity.

This portion of XAML working fine in latest preview and breaks in latest nightly:

<Window.Styles>
    <Style Selector=":is(Visual).fade">
        <Setter Property="Transitions">
            <Transitions>
                <DoubleTransition Property="Opacity" Duration="0:0:1"/>
            </Transitions>
        </Setter>
    </Style>

    <Style Selector=":is(InputElement).fade[Opacity=0]">
        <Setter Property="IsHitTestVisible" Value="False"/>
        <Setter Property="IsVisible" Value="False"/>
    </Style>

    <Style Selector=":is(InputElement).fade[Opacity=0.01]">
        <Setter Property="IsVisible" Value="True"/>
    </Style>

    <Style Selector=":is(InputElement).fade[Opacity=1]">
        <Setter Property="IsHitTestVisible" Value="True"/>
        <Setter Property="IsVisible" Value="True"/>
    </Style>
</Window.Styles>

<TextBlock Classes="fade" Text="Hello there" Opacity="{Binding IsBig}"/>

11 Preview 4:

https://user-images.githubusercontent.com/883587/211304405-54bfd67b-a81c-4da5-86f5-7368b2684e63.mp4

11.0.999-cibuild0028449-beta:

https://user-images.githubusercontent.com/883587/211304423-1c87c1c7-7b78-4aa4-9555-c8d0d019de65.mp4

Looking at the IsVisible / IsHitTestVisible values, they were (unset) in 11 preview 4 and now are True in nightly. Looks like something was fixed actually, and perhaps I'm just misusing conditional styles leading to broken behavior. Thus, I'm not sure if I need to open a new issue for this instead or just look for another way to achieve what I'm trying to do.