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

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

Open
SKProCH opened this issue Apr 7, 2024 · 15 comments
Open

Comments

@SKProCH
Copy link
Contributor

SKProCH commented Apr 7, 2024

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:

  • Explicit implementation of IBinding2.Initiate as before with IBinding.Initiate. Can't be used since IBinding2 is internal interface.

  • Override Binding.Initiate does nothing and the method marked with obsolete attribute.

    [Obsolete(ObsoletionMessages.MayBeRemovedInAvalonia12)]
    public override InstancedBinding? Initiate(
    AvaloniaObject target,
    AvaloniaProperty? targetProperty,
    object? anchor = null,
    bool enableDataValidation = false)

  • ProvideValue method (since my binding is supposed to be used as markup extension anyway) seems like a perfect solution, but as I understand it evaluated during the styles initialization, nor the applying actual style (setter) to a actual control, so I can't get actual control from IServiceProvider (since actual control just doesn't exists at this point)
    image

  • I've looked through Avalonia's DynamicResourceExtension source code and this is perfect case for me, but currently its directly implementing IBinding2, and returning the DynamicResourceExpression.
    DynamicResourceExpression inherited from UntypedBindingExpressionBase which marked with PrivateApiAttrubite (and i can't invoke ctor), inherited from BindingExpressionBase which has an private protected ctor.

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

@SKProCH SKProCH added the bug label Apr 7, 2024
@timunie
Copy link
Contributor

timunie commented Apr 8, 2024

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
Copy link
Member

MrJul commented Apr 8, 2024

Related: #14791

@SKProCH
Copy link
Contributor Author

SKProCH commented Apr 9, 2024

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
Copy link
Contributor

timunie commented Apr 9, 2024

@SKProCH
Copy link
Contributor Author

SKProCH commented Apr 10, 2024

@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):

if (Value is IBinding2 binding)
return SetBinding((StyleInstance)instance, ao, binding);
else if (Value is IBinding)
throw new AvaloniaInternalException("TODO: Make all IBindings implement IBinding2.");
else if (Value is ITemplate template && !typeof(ITemplate).IsAssignableFrom(Property.PropertyType))
return new PropertySetterTemplateInstance(Property, template);
else if (!Property.IsValidValue(Value))
throw new InvalidCastException($"Setter value '{Value}' is not a valid value for property '{Property}'.");
else if (Property.IsDirect)
return SetDirectValue(target);
else
return this;

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
Copy link
Contributor

timunie commented Apr 11, 2024

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
Copy link
Contributor Author

SKProCH commented Apr 11, 2024

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
Copy link
Member

@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
Copy link
Contributor

timunie commented Apr 12, 2024

@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
Copy link
Contributor Author

SKProCH commented Apr 12, 2024

@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:

private void AttachCore(
IBindingExpressionSink sink,
ImmediateValueFrame? frame,
AvaloniaObject target,
AvaloniaProperty targetProperty,
BindingPriority priority)
{
if (_sink is not null)
throw new InvalidOperationException("BindingExpression was already attached.");
_sink = sink;
_frame = frame;
_target = new(target);


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
Copy link
Member

grokys commented May 9, 2024

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
Copy link
Contributor Author

SKProCH commented May 9, 2024

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
Copy link
Member

grokys commented May 10, 2024

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
Copy link
Contributor Author

SKProCH commented May 10, 2024

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.

@aguahombre
Copy link
Contributor

I also have created custom bindings that are now broken in Avalonia 11.
It looks like BindingBase is intended for creating custom bindings and would probably work if private protected abstract BindingExpressionBase Instance was changed to internal protected abstract BindingExpressionBase Instance.

I have two use cases that I don't think are supported by the built-in binding markup.

  1. A dynamic resource binding - looks up a resource based upon a property binding from the view model; e.g. an enum property value that changes the displayed string or brush etc.
  2. A binding for dynamic data collections. Used to bind directly to a SourceCache or SourceList without having to add boilerplate type code to the view model to generate intermediate observable collections.

I am sure there will be other custom binding use cases that will arise and will be prevented unless an API is created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants