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

[Feature]: Bi-directional support for BindingList<T> adaptor #853

Open
Metadorius opened this issue Feb 15, 2024 · 3 comments
Open

[Feature]: Bi-directional support for BindingList<T> adaptor #853

Metadorius opened this issue Feb 15, 2024 · 3 comments

Comments

@Metadorius
Copy link
Contributor

Describe the functionality desired 🐞

Per official docs BindingList<T>, which SourceCache can adapt to, supports bi-directional addition/removal/etc. DynamicData bindings to BindingList do not actually support that. This is problematic because many of third parrty controls, like DevExpress, assume that it's possible to add/remove the items from the BindingList they're bound to, and as a result this doesn't work well because the binding adaptor actually doesn't support such operations.

The steps the functionality will provide

I have a few different ideas on how this could be implemented.

  1. Possibly some sort of an overload for Bind operator. Because we no longer have the original SourceCache to operate on, and/or maybe we have a read-only cache, it may be beneficial to redirect the additions/removals to another cache. The backwards additions/removals would be handled automatically.
cache.Connect()
    .Filter(someFilter)
    .Bind(bindingList, flowbackTargetCache: cache)
    .Subscribe();
  1. Manual flowback subscription configuration. Not currently sure what the API should be, but maybe a similar way, jsut instead of simply specifiying cache you specify an Rx observable stream that links the binding list back to the cache as a side effect and is used to prevent recursive event triggering by the adaptor (jsut thoughts aloud for now, not actually sure if it'll work this way).

Considerations

Using SourceCache directly - adds an unnecessary dependency on DynamicData to the control libraries, kinda adds to vendor lock-in.

@Metadorius Metadorius changed the title [Feature]: Bi-directional support for BindingList adaptor [Feature]: Bi-directional support for BindingList<T> adaptor Feb 15, 2024
@JakenVeina
Copy link
Collaborator

JakenVeina commented Feb 16, 2024

Applying an RX-mindset to this, if the changes being published up to the View layer come in the form of an IObservable<T> stream that the binding subscribes to, then requested changes coming down from the View layer should take the form of an IObservable<T> stream that the binding provides.

From what I can see, this is already supported, via the ToObservableChangeSet() extension for BindingList<T>. This allows you to observe changes to the BindingList<T> which you can then feed back into the source SourceCache<TObject, TKey>.

As a rough example, that probably wouldn't work as-is...

sourceCache
    .Connect()
    .Filter(...)
    .Sort(...)
    .Bind(bindingList)
    .Subscribe();

bindingList
    .ToObservableChangeSet(item => item.Key)
    .Subscribe(changes => sourceCache.Clone(changes));

Off the top of my head, the biggest problem I see is being able to filter out changes to bindingList that are coming from sourceCache, to avoid trying to re-apply those back to sourceCache. This seems like an obvious performance cost to process the changes, as well as a potential for errors to occur when trying to, for example, add an item to the source that already exists. This also adds a risk for infinite feedback looping.

A proof-of-concept of the above would help identify if a new .Bind() operator is warranted. If so, I would say the most fundamental form of it would be...

public static IDisposable Bind<TObject, TKey>(
        this    IObservable<IChangeSet<TObject, TKey>>              source,
                BindingList<TObject>                                bindingList,
                Func<IObservable<IChangeSet<TObject>>, IDisposable> subscribeUpstreamUpdateRequested)
    where TObject not null
    where TKey not null;

Usage would look something like this...

using var subscription = sourceCache
    .Connect()
    .Filter(...)
    .Sort(...)
    .Bind(
        bindingList:                        bindingList,
        subscribeUpstreamUpdateRequested:   upstreamUpdateRequested => upstreamUpdateRequested
            .AddKey(...)
            .Subscribe(changes => sourceCache.Clone(changes)));

This provides the most flexibility to the consumer, regarding how updates from the View layer are funneled back to the source. The lifetime of the "feedback" subscription also gets bundled together with the lifetime of the binding.

Alternative signatures could be....

A) Don't embed the subscription within the operator, which is what other .Bind() operators do. Might be good for consistency, but I think there's value in the concept for forcing subscriptions for operators that produce side-effects, to prevent those side-effects from being accidentally duplicated.

public static IObservable<IChangeSet<TObject, Key>> Bind<TObject, TKey>(
        this    IObservable<IChangeSet<TObject, TKey>>              source,
                BindingList<TObject>                                bindingList,
                Func<IObservable<IChangeSet<TObject>>, IDisposable> subscribeUpstreamUpdateRequested)
    where TObject not null
    where TKey not null;
using var subscription = sourceCache
    .Connect()
    .Filter(...)
    .Sort(...)
    .Bind(
        bindingList:                        bindingList,
        subscribeUpstreamUpdateRequested:   upstreamUpdateRequested => upstreamUpdateRequested
            .AddKey(...)
            .Subscribe(changes => sourceCache.Clone(changes)))
    .Subscribe();

B) Same point about not forcing subscription, but for the "feedback" stream as well.

public static IObservable<IChangeSet<TObject, Key>> Bind<TObject, TKey>(
        this    IObservable<IChangeSet<TObject, TKey>>                          source,
                BindingList<TObject>                                            bindingList,
                Func<IObservable<IChangeSet<TObject>>, IObservable<Unit>> upstreamUpdateOperation)
    where TObject not null
    where TKey not null;
using var subscription = sourceCache
    .Connect()
    .Filter(...)
    .Sort(...)
    .Bind(
        bindingList:                bindingList,
        upstreamUpdateOperation:    upstreamUpdateRequested => upstreamUpdateRequested
            .AddKey(...)
            .Do(changes => sourceCache.Clone(changes))
            .Select(static _ => Unit.Default))
    .Subscribe();

C) Actually just make this a switching operator, where the "feedback" stream is just the downstream.

public static IObservable<IChangeSet<TObject>> Bind<TObject, TKey>(
        this    IObservable<IChangeSet<TObject, TKey>>  source,
                BindingList<TObject>                    bindingList)
    where TObject not null
    where TKey not null;
using var subscription = sourceCache
    .Connect()
    .Filter(...)
    .Sort(...)
    .Bind(bindingList)
    .AddKey(...)
    .Subscribe(changes => sourceCache.Clone(changes));

@Metadorius
Copy link
Contributor Author

Off the top of my head, the biggest problem I see is being able to filter out changes to bindingList that are coming from sourceCache, to avoid trying to re-apply those back to sourceCache. This seems like an obvious performance cost to process the changes, as well as a potential for errors to occur when trying to, for example, add an item to the source that already exists. This also adds a risk for infinite feedback looping.

I've been thinking about this issue as well. I think if the clone (or whatever) operation checked if the changes are already present in the cache (for example, item X was deleted? check if the SourceCache contains it and bail if it doesn't, thus breaking the loop). Not sure if BindingList supports ordering of the items, that's where the issue may arise.

@JakenVeina
Copy link
Collaborator

JakenVeina commented Feb 17, 2024

I think if the clone (or whatever) operation checked if the changes are already present in the cache (for example, item X was deleted?

Sure, but that puts the burden on the feedback listener to validate operations at the source, after they've already been run through the full feedback pipeline, I.E. after processing power has already been spent to transform and otherwise process these changes, then you have to spend MORE processing power to see if they're even relevant.

The much more optimal solution would be to filter them at the source (I.E. the BindingList<T>). The simplest thing would, in fact, probably be within the Bind() operator itself, as follows:

private void OnSourceNext(IChangeSet<TObject, TKey> changes)
{
    _isProcessingSourceChanges = true;
    try
    {
        _bindingList.Clone(changes);
    }
    finally
    {
        _isProcessingSourceChanges = false;
    }
}

private void OnBindingListListChanged(ListChangedEventArgs e)
{
    if (!_isProcessingSourceChanges)
    {
        _upstreamChangesRequested.OnNext(e.ToChangeSet());
    }
}

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

2 participants