taublast / DrawnUi.Maui

UI Rendering Engine for .NET MAUI powered by SkiaSharp
MIT License
206 stars 15 forks source link

Custom control's BindingContext gets overwritten by MainPage's BindingContext #92

Open ToolmakerSteve opened 5 days ago

ToolmakerSteve commented 5 days ago

How set separate BindingContext on a custom SkiaControl, when there is a different BindingContext on MainPage (in which that custom control is used)?

I haven't looked at Maui source code, but I assume Maui uses same BindingContext paradigm as Xamarin Forms: each control can set its own BindingContext.
If left null, then a search is done up the view hierarchy.

DrawnUI.DrawnView interferes with this paradigm.
its OnBindingContextChanged() recurses down through subviews, setting their BindingContext.

This wipes out any BindingContext set on those controls.

I could work around this, by making a subclass of SkiaLayout that overrides ApplyBindingContext. Put my custom control inside that.
Is there a cleaner fix available?


Thinking about how one could propagate BindingContext down, while respecting different values.
Idea: Add parameters:

public virtual void ApplyBindingContext(object oldValue, object newValue)
{
  foreach (var child in this.Views)
  {   // If child has a different BindingContext, leave it alone. But do replace null, for efficient access.
    if (child.BindingContext == null || child.BindingContext == oldValue)
      child.BindingContext = newValue;
  }

Similar change would be needed in other classes.

I see that SkiaViewSwitcher already avoids overriding null BindingContext of children. That also works, but ONLY if child sets its BindingContext AFTERWARDS. [Or there is some intermediate container with a null context.] Though might be less efficient, if Binding then has to search up the hierarchy. And is a "fragile" solution - a dynamic change later might not propagate the same. Either don't propagate at all, or propagate with something like what I show above.


UPDATE

My idea above doesn't "recurse" awareness of "oldValue". Won't work.

Looking at .Net source:

Not worth the time right now to trace through how this works.
Might be better to remove use of ApplyBindingContext. Let .Net do what it does.

For now, I'll work around what DrawnUI does. Stop the recursive BindingContext change in my subclass of SkiaLayout.

[I continue to be impressed by all the work that went into DrawnUI. There is much here that I will learn from.]

ToolmakerSteve commented 5 days ago

Found an acceptable hack to work around this:

In custom control:

// in constructor
  ...
  BindingContext = FrozenBindingContext = this;
  ...

// override
protected override void OnBindingContextChanged()
{   // Undo parent's misguided change of our (and our descendants) BindingContext.
    if (FrozenBindingContext != null && BindingContext != FrozenBindingContext)
        BindingContext = FrozenBindingContext;

    base.OnBindingContextChanged();
}

// data
private object FrozenBindingContext;

The result is when ApplyBindingContext recursion changes control's BindingContext,
it is forced back to correct value.


A better fix would be to change how BindingContext is set and propagated, in SkiaControl.Shared.cs.
But that seemed to require changing all DrawnUI overrides of ApplyBindingContext.
Possibly also overrides of OnBindingContextChanged.

Also SetParent:
[If my understanding of BindableObject is correct] if (BindingContext == null) BindingContext = control.BindingContext; is what starts the incompatibility with Microsoft.Maui.Controls.BindableObject's handling of null context. Once those nulls are gone, there is no easy way to track "old" ("directly set" as contrasted to "inherited") state of BindingContext in each descendant. To determine when to propagate a BindingContext change.


Feels like Microsoft.Maui.Controls.BindableObject's mechanism is overly complicated. Probably started out simple, but the failure to provide a way for subclasses to easily override propagation of BindingContext, I suspect is what led to complicated code inside BindableObject to propagate. [Brainstorming] maybe have public virtual SetBindingContext(object newValue), and make BindingContext setter call that to do its job. Subclass responsible for propagating in its override. Subclass would still know oldValue, because its BindingContext would not have changed yet. Or something like OnPropertyChanged, which has access to both newValue and oldValue.

Not sure if this could be changed, and still be backward-compatible. Or affect performance adversely. (OTOH, the propagation code in BindableObject didn't look cheap either.) We live with it.

ToolmakerSteve commented 5 days ago

Brainstorming. A way to distinguish between BindingContext set "directly" on a control, vs "inherited" context:

public partial class SkiaControl : VisualElement
{
    public object DirectBindingContext
    {
        get => _directBindingContext;
        set
        {
            _directBindingContext = value;
            BindingContext = value;
        }
    }
    private object _directBindingContext;

    public virtual void ApplyBindingContext()
    {
        // Whenever there is a DirectBindingContext, propagate that.
        object context = DirectBindingContext ?? BindingContext;

        foreach (var content in this.Views)
        {    // propagate to descendant's BindingContext.
             content.BindingContext = context;
        }

        foreach (var content in this.VisualEffects)
        {
            content.Attach(this);
        }

        if (FillGradient != null)
            FillGradient.BindingContext = context;
    }
}

In subclasses that wish to set their own BindingContext:

public partial class SubclassNameHere
{
    ...
    public SubclassNameHere()
    {
        ...
        //was: BindingContext = ...;
        DirectBindingContext = ...;
        ...
    }
}

All code that sets "BindingContext" would have to be examined. To determine whether it should instead set DirectBindingContext.

"client" code (an app) would typically use DirectBindingContext, to avoid their context being overwritten by an ancestor.

taublast commented 4 days ago

This is a very interesting subject.

When the parent was set, adopt its binding context - call an internal virtual SetChildInheritedBindingContext, since this is internal like most of the MAUI mechanics we cannot use it. When the parent is lost (parent becomes null) sets binding context to null.

When the binding context changes then propagate binding context to children. This leads to a situation when a child cannot affect this, only the parent can, by overriding a public ApplyBindingContext. This method is similar to MAUI internal SetInheritedBindingContext. At the same time when the parent is lost (parent becomes null) we set binding context to null.

This led, for example, to a hacky workaround for SkiaScroll, to avoid setting its biding context to a child when it already has its own:

   public override void ApplyBindingContext()
   {
       base.ApplyBindingContext();

       if (this.Content != null && Content.BindingContext == null) //todo remove this last condition!
       {
           Content.BindingContext = BindingContext;
       }
   }

So basically when this was all brought to light, thanks to You, I would tend to solve that by implementing similar to MAUI **protected*** SetChildInheritedBindingContext alternative that would be invoked from OnParentChanged, while we wouldn't propagate context to children anymore. This way every control could implement it's own logic to accept or deny a new binding context.

Thoughts?

ToolmakerSteve commented 3 days ago

SetInheritedBindingContext would be called on the child?

Yes, that would work. I would consider also adding a public virtual SetBindingContext, for situations where the goal is to tell an instance it should definitely use this new context. But still give it a chance to react to the change.

Train ourselves and anyone who uses DrawnUI to avoid doing "BindingContext = ..." directly. Instead call one of these two methods. Then all classes could override, to change how they handle the two different requests.

Default behavior something like this:

// Use ourBindingContext. If it is null, use inheritedBindingContext.
protected object ourBindingContext;
protected object inheritedBindingContext;   // Maybe this can be omitted.

// Definitely public. Use instead of existing `public object BindingContext` setter.
public virtual void SetBindingContext(object context)
{
    if (ourBindingContext != context)
    {
        ourBindingContext = context;
        // Use our context. If ours gets cleared, use inherited one.
        BindingContext = context ?? inheritedBindingContext;
    }
}
// I think public, so app devs can extend DrawnUI controls, with full freedom.
public virtual void SetInheritedBindingContext(object context)
{
    if (inheritedBindingContext != context)
    {
        inheritedBindingContext = context;
        // Only use inherited context if ours is null.
        if (ourBindingContext == null)
            BindingContext = context;
    }
}

Maybe it is a bit "overkill", to add TWO new binding context members. Could reduce to ONE, omit inheritedBindingContext, if there is a way for child to trigger ancestors' cascade of SetInheritedBindingContext, when SetBindingContext(null).

Or maybe it can rely on parent, do:

public virtual void SetBindingContext(object context)
...
        BindingContext = context ?? Parent?.BindingContext;   // "?" needed in case parent is null.

Then inherited method is simply:

protected virtual void SetInheritedBindingContext(object context)
{
    // Only use inherited context if ours is null.
    if (ourBindingContext == null)
        BindingContext = context;
}