Skip to content

SortAndBind fixes and improvements #939

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.

Sign up for GitHub

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

Merged
RolandPheasant merged 7 commits into from
Jan 20, 2025
Merged

SortAndBind fixes and improvements #939

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ae5a789
SortAndBind: Enable opt-in to ResetOnFirstTimeLoad + SortAndBind spec…
RolandPheasant Sep 4, 2024
259fa73
Merge branch 'main' into feature/bring-back-resetonfirsttimeload
dwcullop Sep 4, 2024
8c028a0
Add MainThreadScheduler option to DynamicDataOptions and use this for…
RolandPheasant Sep 5, 2024
9547f08
Add scheduler overloads for SortAndBind and bind which targets virtua…
RolandPheasant Sep 6, 2024
5e5b02e
Add scheduler overloads for SortAndBind and bind which targets page c…
RolandPheasant Sep 6, 2024
c2523fe
Merge branch 'main' into feature/bring-back-resetonfirsttimeload
dwcullop Jan 16, 2025
bc2810f
Remove IScheduler overload from sort and bind. Instead, add the sched…
RolandPheasant Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/DynamicData.Tests/API/ApiApprovalTests.DynamicDataTests.DotNet8_0.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,9 @@ namespace DynamicData.Binding
{
public SortAndBindOptions() { }
public int InitialCapacity { get; init; }
public bool ResetOnFirstTimeLoad { get; init; }
public int ResetThreshold { get; init; }
public System.Reactive.Concurrency.IScheduler? Scheduler { get; init; }
public bool UseBinarySearch { get; init; }
public bool UseReplaceForUpdates { get; init; }
}
Expand Down
49 changes: 48 additions & 1 deletion src/DynamicData.Tests/Cache/SortAndBindFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,19 @@ protected override (ChangeSetAggregator<Person, string> Aggregrator, IList<Perso
}


// Bind to a binding list
public sealed class SortAndBindToBindingList : SortAndBindFixture

{
protected override (ChangeSetAggregator<Person, string> Aggregrator, IList<Person> List) SetUpTests()
{
var list = new ObservableCollection<Person>(new BindingList<Person>());
var aggregator = _source.Connect().SortAndBind(list, _comparer).AsAggregator();
return (aggregator, list);
}
}


// Bind to a readonly observable collection
public sealed class SortAndBindToReadOnlyObservableCollection: SortAndBindFixture
{
Expand Down Expand Up @@ -151,7 +164,7 @@ public void NeverFireReset()
using var sorted = _source.Connect().SortAndBind(out var list, _comparer, options).Subscribe();
using var collectionChangedEvents = list.ObserveCollectionChanges().Select(e => e.EventArgs).Subscribe(_collectionChangedEventArgs.Add);

// fire 5 changes, should always reset because it's below the threshold
// fire 5 changes, should not reset because it's below the threshold
_source.AddOrUpdate(Enumerable.Range(0, 5).Select(i => new Person($"P{i}", i)));
_collectionChangedEventArgs.Count.Should().Be(5);
_collectionChangedEventArgs.All(a => a.Action == NotifyCollectionChangedAction.Add).Should().BeTrue();
Expand All @@ -168,6 +181,40 @@ public void NeverFireReset()

}

[Fact]
[Description("Check reset is fired on first time load. This checks historic first time load opt-in.")]
public void FireResetOnFirstTimeLoad()
{
var options = new SortAndBindOptions { ResetThreshold = 10, ResetOnFirstTimeLoad = true};

using var sorted = _source.Connect().SortAndBind(out var list, _comparer, options).Subscribe();
using var collectionChangedEvents = list.ObserveCollectionChanges().Select(e => e.EventArgs).Subscribe(_collectionChangedEventArgs.Add);

// fire 5 changes, should always reset even though it's below the threshold
_source.AddOrUpdate(Enumerable.Range(0, 5).Select(i => new Person($"P{i}", i)));
_collectionChangedEventArgs.Count.Should().Be(1);
_collectionChangedEventArgs.All(a => a.Action == NotifyCollectionChangedAction.Reset).Should().BeTrue();


_collectionChangedEventArgs.Clear();

// fire 15 changes, we should get a refresh event
_source.AddOrUpdate(Enumerable.Range(10, 15).Select(i => new Person($"P{i}", i)));
_collectionChangedEventArgs.Count.Should().Be(1);
_collectionChangedEventArgs[0].Action.Should().Be(NotifyCollectionChangedAction.Reset);

_collectionChangedEventArgs.Clear();

// fires further 5 changes, should result individual notifications
_source.AddOrUpdate(Enumerable.Range(-10, 5).Select(i => new Person($"P{i}", i)));
_collectionChangedEventArgs.Count.Should().Be(5);
_collectionChangedEventArgs.All(a => a.Action == NotifyCollectionChangedAction.Add).Should().BeTrue();

list.Count.Should().Be(25);

}



public void Dispose() => _source.Dispose();
}
Expand Down
34 changes: 15 additions & 19 deletions src/DynamicData/Binding/BindingListEventsSuspender.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,30 +2,26 @@
// Roland Pheasant licenses this file to you under the MIT license.
// See the LICENSE file in the project root for full license information.

#if SUPPORTS_BINDINGLIST
using System.ComponentModel;
using System.Reactive.Disposables;

namespace DynamicData.Binding
{
internal sealed class BindingListEventsSuspender<T> : IDisposable
{
private readonly IDisposable _cleanUp;
namespace DynamicData.Binding;

public BindingListEventsSuspender(BindingList<T> list)
{
list.RaiseListChangedEvents = false;
internal sealed class BindingListEventsSuspender<T> : IDisposable
{
private readonly IDisposable _cleanUp;

_cleanUp = Disposable.Create(
() =>
{
list.RaiseListChangedEvents = true;
list.ResetBindings();
});
}
public BindingListEventsSuspender(BindingList<T> list)
{
list.RaiseListChangedEvents = false;

public void Dispose() => _cleanUp.Dispose();
_cleanUp = Disposable.Create(
() =>
{
list.RaiseListChangedEvents = true;
list.ResetBindings();
});
}
}

#endif
public void Dispose() => _cleanUp.Dispose();
}
37 changes: 30 additions & 7 deletions src/DynamicData/Binding/SortAndBind.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Roland Pheasant licenses this file to you under the MIT license.
// See the LICENSE file in the project root for full license information.

using System.ComponentModel;
using System.Reactive.Disposables;
using System.Reactive.Linq;
using DynamicData.Cache;
Expand Down Expand Up @@ -33,12 +34,17 @@ public SortAndBind(IObservable<IChangeSet<TObject, TKey>> source,
// static one time comparer
var applicator = new SortApplicator(_cache, target, comparer, options);

_sorted = source.Do(changes =>
if (options.Scheduler is not null)
source = source.ObserveOn(options.Scheduler);

@JakenVeina JakenVeina Sep 5, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is insufficient. Refer to #932, the logical query this needs to serve as a replacement for is...

.Sort(...)
.ObserveOn(...)
.Bind(...)

I.E. the scheduling needs to be applied to both input streams, source and comparer. Otherwise, notifications coming in on comparer can trigger the ObservableCollection be manipulated off of the main thread.

Another .ObserveOn() call applied to comparer should be sufficient here, since there is no change stream "between" the sorting and binding steps anymore.

@RolandPheasant RolandPheasant Sep 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair call, I'll fix that.

Also I was wondering whether to add scheduler overloads to SortAndBind, or instead to instruct the use of

DynamicDataOptions.SortAndBind = DynamicDataOptions.SortAndBind with
{
    Scheduler = myMainThreadScheduler
};

which is neat but not obvious.

Or should I do both? Thoughts?

EDIT:

Or better yet set the scheduler like RxUI:

DynamicDataOptions.MainThreadScheduler= myMainThreadScheduler;

@RolandPheasant RolandPheasant Sep 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've applied the scheduler to the comparer changed observable, and moved the UI thread scheduler option to DynamicDataOptions.MainThreadScheduler.

So the one remaining question is whether it's worth adding 12 new overloads to pass an optional comparer to SortAndBind or to rely on the global main thread scheduler only?

@JakenVeina JakenVeina Sep 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say both. There's value in being able to specify on a per-call basis, and in being able to set it globally and not have to worry about it.

Not sure about whether a global-level scheduler should be universal, or per-operator. Again, I think there's value in both, but in this case, I think it's a little cumbersome to HAVE both.

Rather than MainThreadScheduler, I would suggest BindScheduler or BindingScheduler because that clarifies how it's actually being USED if you set it. It would only be pulled for binding operations.

@RolandPheasant RolandPheasant Sep 6, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to BindingScheduler + added scheduler overloads for new generation of binding operators.

@dwcullop dwcullop Sep 6, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I was wondering whether to add scheduler overloads to SortAndBind, or instead to instruct the use of

I suggest both: Giving the ability to provide all the options for their special cases AND providing a helper method that facilitates the most common use case(s).


_sorted = source.Select((changes, index) =>
{
// clone to local cache so that we can sort the entire set when threshold is over a certain size.
_cache.Clone(changes);

applicator.ProcessChanges(changes);
applicator.ProcessChanges(changes, index == 0);

return changes;
});
}

Expand All @@ -48,6 +54,9 @@ public SortAndBind(IObservable<IChangeSet<TObject, TKey>> source,
IList<TObject> target)
=> _sorted = Observable.Create<IChangeSet<TObject, TKey>>(observer =>
{
if (options.Scheduler is not null)
source = source.ObserveOn(options.Scheduler);

var locker = new object();
SortApplicator? sortApplicator = null;

Expand All @@ -61,14 +70,17 @@ public SortAndBind(IObservable<IChangeSet<TObject, TKey>> source,

// Listen to changes and apply the sorting
var subscriber = source.Synchronize(locker)
.Do(changes =>
.Select((changes, index) =>
{
_cache.Clone(changes);

// the sort applicator will be null until the comparer change observable fires.
if (sortApplicator is not null)
sortApplicator.ProcessChanges(changes);
}).SubscribeSafe(observer);
sortApplicator.ProcessChanges(changes, index == 0);

return changes;
})
.SubscribeSafe(observer);

return new CompositeDisposable(latestComparer, subscriber);
});
Expand All @@ -92,10 +104,12 @@ public void ApplySort()
}

// apply sorting as a side effect of the observable stream.
public void ProcessChanges(IChangeSet<TObject, TKey> changeSet)
public void ProcessChanges(IChangeSet<TObject, TKey> changeSet, bool isFirstTimeLoad)
{
var forceReset = isFirstTimeLoad && options.ResetOnFirstTimeLoad;

// apply sorted changes to the target collection
if (options.ResetThreshold > 0 && options.ResetThreshold < changeSet.Count)
if (forceReset || (options.ResetThreshold > 0 && options.ResetThreshold < changeSet.Count))
{
Reset(cache.Items.OrderBy(t => t, comparer), true);
}
Expand All @@ -122,6 +136,15 @@ private void Reset(IEnumerable<TObject> sorted, bool fireReset)
observableCollectionExtended.Load(sorted);
}
}
else if (fireReset && target is BindingList<TObject> bindingList)
{
// suspend count as it can result in a flood of binding updates.
using (new BindingListEventsSuspender<TObject>(bindingList))
{
target.Clear();
target.AddRange(sorted);
}
}
else
{
target.Clear();
Expand Down
16 changes: 16 additions & 0 deletions src/DynamicData/Binding/SortAndBindOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Roland Pheasant licenses this file to you under the MIT license.
// See the LICENSE file in the project root for full license information.

using System.Reactive.Concurrency;

namespace DynamicData.Binding;

/// <summary>
Expand All @@ -28,4 +30,18 @@ public record struct SortAndBindOptions()
/// Set the initial capacity of the readonly observable collection.
/// </summary>
public int InitialCapacity { get; init; } = -1;

/// <summary>
/// Reset on first time load.
///
/// This is opt-in only and is only required for consumers who need to maintain
/// backwards compatibility will the former BindingOptions.ResetOnFirstTimeLoad.
/// </summary>
public bool ResetOnFirstTimeLoad { get; init; }

/// <summary>
/// The default main thread scheduler. If left null, it is the responsibility of the consumer
/// to ensure binding takes place on the main thread.
/// </summary>
public IScheduler? Scheduler { get; init; }
}
5 changes: 3 additions & 2 deletions src/DynamicData/List/ListEx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// See the LICENSE file in the project root for full license information.

using System.Collections.ObjectModel;

using System.ComponentModel;
using DynamicData.Kernel;

// ReSharper disable once CheckNamespace
Expand Down Expand Up @@ -101,7 +101,8 @@ public static void AddRange<T>(this IList<T> source, IEnumerable<T> items)
extendedList.AddRange(items);
break;
default:
items.ForEach(source.Add);
foreach (var t in items)
source.Add(t);
break;
}
}
Expand Down