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.54k stars 2.21k forks source link

Unable to create custom binding in 11.1.0-beta1, after binding system refactor #15270

Open SKProCH opened 6 months ago

SKProCH commented 6 months ago

Describe the bug

Hello, I've noticed a "rework" of binding system internals in a 11.1 version, #13970

In my project, DialogHost.Avalonia I developed a custom Binding to pick up multiple brush values from different themes. In 10.0 it has been a quite simple - just implement an IBinding and you ready to go. With Avalonia 11 IBinding was marked with NotClientImplementable attribute, so it can't be implemented in user code. But, after a discussions in telegram the way out was been found - i can inherit from Binding class add explicit IBinding.Initiate implementation to my class and create an InstancedBinding instance there.

https://github.com/AvaloniaUtils/DialogHost.Avalonia/blob/d303596bc4880cfcd07d78d381f68054851fc423/DialogHost.Avalonia/Utilities/MultiDynamicResourceExtension.cs#L27-L46

But seems like this got a rework in #13970. So, actually IBinding.Initiate wasn't used and left for backward compatibility. After looking through the code seems like IBinding2.Initiate used for now. I've tried to find a way out of this situation, but I can't. So I've open this issue as stated in warning If you depend on this API, please open an issue with details of your use-case.

I've tried some ways to workaround this problem, but still doesn't find a proper one. Here is what I tried:

So, my question is: how to implement my custom binding from user code?

For my use case I want to pass some set of resource keys (from different themes like Fluent, Material, etc) and want to control to pick any of them depends on what key is present in user app.
I know what it can be achieved inside of theme package, but it requires either the package will have to have a strict dependency on DialogHost, or it will be necessary to produce countless packages for almost every existing theme and force users to download them (and include them in styles) to support just a few brushes from resources.

Also, this is not just an issue about "oh my god, i can't hacky support multiple resource keys in my library", I think that's the issue about the openness and extensibility of Avalonia as this is one of the key features of this framework, and I believe that it is necessary to leave users the opportunity to create their own complex bindings, or, at least, allow some point of extensibility here.

Probably I just miss something and the solution lies on the surface? Also, pinging @grokys as the author of new binding system

To Reproduce

Try to create an binding implementation from user code.

Expected behavior

No response

Avalonia version

11.1.0-beta1

OS

No response

Additional context

No response

timunie commented 6 months ago

In your use case I think the Key shouldn't change during runtime but the resource can. So I'd say returning a ResourceObservable should be enough for your case.

If not: there are some discussions how to create custom Localization extensions. Most of them use reflection bindings however there's at least one triyng to make it a compiled one as well.

Last but not least: I also think it would be great to have a convenient solution for custom bindings or at least a documented way without using private API.

MrJul commented 6 months ago

Related: #14791

SKProCH commented 6 months ago

In your use case I think the Key shouldn't change during runtime but the resource can. So I'd say returning a ResourceObservable should be enough for your case.

What did you mean by ResourceObservable? Can't find such class

timunie commented 6 months ago

@SKProCH see https://docs.avaloniaui.net/docs/guides/styles-and-resources/resources#consuming-resources-from-code

SKProCH commented 6 months ago

@timunie I've looked through my old implementation and I actually use GetResourceObservable already.

Also, only opportunity what I have is the ProvideValue from markup extension logic. But, i can't return InstancedBinding or IObservable from it, as @maxkatz6 suggested in telegram. Seems like only IBinding2 is supported for use case, but I can't implement it (due to internal access modifier):

https://github.com/AvaloniaUI/Avalonia/blob/544d5372e31fc472bcb30ae926aaff5391ce9e59/src/Avalonia.Base/Styling/Setter.cs#L79-L90

Old approach with passing IBinding here doesn't work seems like due to throw new AvaloniaInternalException("TODO: Make all IBindings implement IBinding2."); in code above.

So, currently there is absolutely no way to do something with that and this is sad.

timunie commented 6 months ago

this works for me using latest nightly (should also work in 11.1):

public class MyExtenstion : MarkupExtension{
    public override object ProvideValue(IServiceProvider serviceProvider)
    {
        var res = App.Current.GetResourceObservable("MyKey");
        return res.ToBinding();
    }
}

it also updates the message:

  <Application.Resources>
    <x:String x:Key="MyKey">Hello</x:String>
  </Application.Resources>
App.Current.Resources["MyKey"] = "You pressed me";
SKProCH commented 6 months ago

Oh, thanks actually. I've missed a ToBinding extension method. This works, but it relies on app theme, nor the actual control resources. Anyway, better then nothing.

maxkatz6 commented 6 months ago

@SKProCH you can get styled element (or resource node) to startup a lookup from using service provider. Something like dynamic resource does https://github.com/AvaloniaUI/Avalonia/blob/master/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/DynamicResourceExtension.cs#L33C17-L38

timunie commented 6 months ago

@SKProCH it would be easier to understand your usecase if you could file a minimal sample. Then we could probable give you an alternative or extend our API if we find it's a useful feature.

SKProCH commented 6 months ago

@SKProCH you can get styled element (or resource node) to startup a lookup from using service provider. Something like dynamic resource does https://github.com/AvaloniaUI/Avalonia/blob/master/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/DynamicResourceExtension.cs#L33C17-L38

This allows me to get style instance. For example DialogHostStyles instance, not the actual control itself. Here is the example: image In the other hand DynamicResourceExtension will instanciate an DynamicResourceExpression, which will operate actual control instance, not the styles (ControlTheme) instance, which will be passed to AttachCore:

https://github.com/AvaloniaUI/Avalonia/blob/0bf3ffc05b4ba6ad59de501099f9d9ba39cbd944/src/Avalonia.Base/Data/Core/UntypedBindingExpressionBase.cs#L256-L268


Since now I can't create my own BindingExpression, I can't make my binding (or markup extension) work with actual control instance. For example, I can't create my analogue of DynamicResourceExtension due to that.

I've make a workaround using some App theme resources for my case (i've already implemented this workaround) So, now I can use some static values, as App.Current, as @timunie suggested and take resources from there, but it doesn't work with some resources, overridden near an actual control instance, e.g. if I override a resource in Window.Resources DynamicResource will properly use overridden resources, since it takes it from the actual control instance. While my solution wasn't since it can only operate resources, defined at App level.

I've created a sample app to demonstrate this: image

My old implementation (with actual binding to actual control) handles this properly.


This fact (what now we don't have access to actual control instance) will be also breaks other scenarios. For example, this is the quick proof of concept of binding to untyped JsonObject for DataGrid. Probably it isn't possible now.

As I sad earlier its not just a "i can't hacky bind colors now", its block the possibility of creating any custom binding with user driven logic, and key point - we can't get access to actual control anymore.

This 2 examples this is just what I encountered, but I'm sure what here is many others usages for custom bindings.


I've looked at current bindings implementation and probably opening user access to BindingExpressionBase (and probably UntypedBindingExpressionBase) will allow creating custom binding and shouldn't(?) break anything.

grokys commented 5 months ago

I've looked at current bindings implementation and probably opening user access to BindingExpressionBase (and probably UntypedBindingExpressionBase) will allow creating custom binding and shouldn't(?) break anything.

Just a bit of background on the "closing" of creating new binding types: this was done because I'm not confident enough in the current shape of the API yet to "freeze" them in their current shape. The previous API was way too open which prevented us from making improvements to the binding system throughout the lifetime of 0.10.x and I'd like to not have this problem moving forward.

I've taken a bit of a look at your use-case and I think I see the problem you're trying to solve. Allowing a control library to use different resource keys based on which theme is currently in use is definitely a problem we've not solved. We need a long term-solution for issues like this, so I understand your desire for a workaround in the short-term.

Having said that, I do think that the way you're doing this with a MultiDynamicResource extension is less than optimal. It's pushing the evaluation of the available resource keys to the initialization of the control, when really this could be done a single time (assuming a single application-wide theme),

One (untested) alternative idea: could you create a custom implementation of IResourceProvider which does this lookup? Taking this as an example you'd simply do something like:

<Setter Property="OverlayBackground" Value="{DynamicResource DialogHostOverlayBackground}" />

You would then have an e.g. DialogHostResources class (which implements IResourceProvider) from somewhere in your resources which does the lookup of DialogHostOverlayBackground to the appropriate theme resource. This would have the downside that it wouldn't react to changes in the value of the resource, but I'm not sure how necessary that is?

Regarding your comment:

I think that's the issue about the openness and extensibility of Avalonia as this is one of the key features of this framework

Yep, I understand. Unfortunately the openness of the API has to be balanced with API stability and the ability to make improvements without API breakages... It's a tricky balancing act.

SKProCH commented 5 months ago

Thanks for extended response, @grokys. Actually, interesting suggestion about the IResourceProvider, I'll try to implement it.

I'm not confident enough in the current shape of the API yet to "freeze" them in their current shape

So, is there is chance what some kind of extensibility at this point will be available when 11.1 will be out of beta stage? If yes, so, apparently this is not a big problem.

Unfortunately the openness of the API has to be balanced with API stability and the ability to make improvements without API breakages

Perhaps, if you are not confident in the stability of the API, then you should allow it to be extended, for example through a separate nuget package, which will have a non-release version (e.g. Avalonia.Unstable.Bindings with note that anything can be changed at any time with breaking changes). This allows advanced users make required extensions, if they are knows what they doing. Or, probably you can use the 'Experimental' attribute for those APIs, and you can break things when you want. image

grokys commented 5 months ago

We already have a way of allowing access to private APIs ;) It's a little bit hidden because we don't want loads of people using it and then blaming us when we break them, but see here: https://github.com/AvaloniaUI/Avalonia/wiki/Using-private-apis-in-nuget-packages.

SKProCH commented 5 months ago

Oh, interesting solution. It might not be worth using this in public packages, but it is a great feature. Thanks for feedback!

In any case, the fact that blocking access to creating bindings is a temporary (in some way) problem related more to the stability of the API, and not to your reluctance, is good to hear.