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

Allow connecting to any IEnumerable<T> #132

Closed
DaRosenberg opened this issue Jul 9, 2018 · 19 comments
Closed

Allow connecting to any IEnumerable<T> #132

DaRosenberg opened this issue Jul 9, 2018 · 19 comments
Labels

Comments

@DaRosenberg
Copy link

Now that we have AutoRefresh() and Filter() it would make sense to be able to connect to any type of enumerable, even immutable ones, because the combination of AutoRefresh() and Filter() basically introduces the observability.

Now we have to do something like this:

var o = new ObservableCollection<StaticRowInfo>(someArray)
        .ToObservableChangeSet()
        .AutoRefresh(x => x.IsVisible)
        .Filter(x => x.IsVisible)

I.e. wrap the array in an observable collection that will not be used for anything, just take up unnecessary memory.

Would be nicer if we could do something like this:

var o = someArray
        .ToObservableChangeSet()
        .AutoRefresh(x => x.IsVisible)
        .Filter(x => x.IsVisible)

The implementation should preferably not simply create a wrapper ObservableCollection<T> behind the scenes, because then this improvement would be little more than syntactic sugar.

@robsonj
Copy link
Contributor

robsonj commented Jul 9, 2018

Maybe some thin wrapper over IEnumerable that implements IObservable<IChangeSet> ?

@RolandPheasant
Copy link
Collaborator

RolandPheasant commented Jul 9, 2018

Something like this (for both list and cache)

var myObservableChangeSet =  myEnumerable.ToObservableChangeSet()

and implement a light weight source to act as the backing store

public class OneTimeSourceList<T>: IObservableList<T>
{
       \\implement with simplified internals
}

@DaRosenberg
Copy link
Author

Yes, exactly!

@robsonj
Copy link
Contributor

robsonj commented Jul 9, 2018

Would this not work...


    public static class ObservableListEx
    {
        public static IObservable<IChangeSet<T>> ToObservableChangeSet<T>(this IEnumerable<T> self)
            => self.ToObservable().ToObservableChangeSet();
    }

@glennawatson
Copy link
Member

The implementation should preferably not simply create a wrapper ObservableCollection behind the scenes, because then this improvement would be little more than syntactic sugar.

I believe that would violate that line. Would work but a bit of extra overhead.

@robsonj
Copy link
Contributor

robsonj commented Jul 9, 2018

Is it extra overhead though? I gotta download the MS source to see what it is actually doing.

@robsonj
Copy link
Contributor

robsonj commented Jul 10, 2018

Next version...

    internal class OneTimeSourceList<T> : IObservableList<T>
    {
        internal OneTimeSourceList(IEnumerable<T> source)
            => Items = source;

        public IEnumerable<T> Items { get; }

        public IObservable<int> CountChanged
            => Observable.Empty<int>();

        public int Count
            => Items.Count();

        public IObservable<IChangeSet<T>> Connect(Func<T, bool> predicate = null)
            => Observable.Defer(() =>
        {
            var change = new Change<T>(ListChangeReason.Add, Items);
            var changeSet = new ChangeSet<T>(new[] {change});
            return Observable.Return(changeSet);
        });

        public void Dispose()
        {
        }
    }

    public static class ObservableListEx
    {
        public static IObservable<IChangeSet<T>> ToObservableChangeSet<T>(this IEnumerable<T> self)
            => new OneTimeSourceList<T>(self).Connect();
    }

@RolandPheasant
Copy link
Collaborator

Something like that is perfect

@robsonj
Copy link
Contributor

robsonj commented Jul 10, 2018

Not sure the OneTimeSourceList is even needed to be honest, the Observable.Defer() code could be inlined into the extension method

@glennawatson
Copy link
Member

So like a ienumerable.tochangeset or something.

@robsonj
Copy link
Contributor

robsonj commented Jul 10, 2018

    public static class EnumerableExtensions
    {
        public static IObservable<IChangeSet<T>> ToObservableChangeSet<T>(this IEnumerable<T> self)
            => Observable.Defer(() =>
            {
                var change = new Change<T>(ListChangeReason.Add, self);
                var changeSet = new ChangeSet<T>(new[] { change });
                return Observable.Return(changeSet);
            });
    }

Example usage...

IObservable<IChangeSet<T>> result = myEnumerable.ToObservableChangeSet();

@RolandPheasant
Copy link
Collaborator

That's even better

@glennawatson
Copy link
Member

I guess the only disadvantage of the above is it's not really observable. Although it keeps it consistent so it's transparent for observable collections etc.

@robsonj
Copy link
Contributor

robsonj commented Jul 10, 2018

Why isn't it Observable? I guess if you mean, if you then add an item to the source enumerable after calling this extension, then no you wont get any additional changeset notification for that add, but I don't see any way around that due to the source being a plain old enumerable.

@DaRosenberg
Copy link
Author

The inline approach looks pretty much perfect to me. I was going to point out that the OneTimeSourceList should either have an overridable Dispose() implementation or be sealed, but the extension method approach makes that a moot point.

"Not observable" in that sense is precisely the benefit of this proposal - my suggestion was inherently intended for scenarios where the underlying IEnumerable<T> is assumed to be immutable, or where we otherwise don't care about changes past the point of subscription.

@robsonj
Copy link
Contributor

robsonj commented Jul 10, 2018

Yea, I think the extension method should cover your needs then. I'll submit a pull request later this evening with some unit tests if the proposed solution is acceptable

@glennawatson
Copy link
Member

Yeah I agree, just the only other alternative is instead of using ToObservableChangeSet() as a name use ToChangeSet()

I think the I prefer keeping one consistent naming scheme myself though.

@RolandPheasant
Copy link
Collaborator

Awesome. Could you do the same but add a key selector overload so it can equally apply to cache.

Also could you add the extensions to ObservableListEx and ObservableCacheEx respectively

@robsonj
Copy link
Contributor

robsonj commented Jul 10, 2018

Yep, will do Roland.

RolandPheasant added a commit that referenced this issue Jul 12, 2018
#132 - Allow connecting to any IEnumerable<T>
@lock lock bot added the outdated label Jul 9, 2019
@lock lock bot locked and limited conversation to collaborators Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants