[Bug]: SortAndBind has different behaviour to .Sort().Bind()

[giard-alexandre]

Describe the bug 🐞

I'm specifically calling this out on the Avalonia TreeDataGrid, but I suspect this will be true on any control that shows lists and allows selection, across any framework. I do not have this issue when using the old .Sort(sorter).Bind(out list) method of sorting, no matter what optimization options I pass into Sort or SortAndBind.

Step to reproduce

1. Define a stream of data that uses TransformWithInlineUpdate to update items inline
2. Use .Sort().Bind()
3. Select an item, item remains selected despite updates being processed.
4. Change to .SortAndBind()
5. Select an Item
6. Selection is deselected after every tick of data.

Reproduction repository

https://github.com/giard-alexandre/TickingTdg

Expected behavior

Selection should be preserved when we update items inline.

Screenshots 🖼️

No response

IDE

No response

Operating system

No response

Version

No response

Device

No response

DynamicData Version

9.0.4

Additional information ℹ️

No response

[giard-alexandre]

Closing this as it seems the issue is actually in TreeDataGrid, which does not support moving a selection atm

[JakenVeina]

I mean, if you were able to observe a difference in behavior between .Sort().Bind() and .SortAndBind() there may well be a bug here, regardless of what TreeDataGrid is doing. Did you just find that there wasn't actually a difference?

[giard-alexandre]

Correct! The issue was a small difference in the first and second test. I'm neck deep in debugging it now so if something else comes up I'll reopen/open a new ticket :)

I actually figured out there was an error in my testing when I tried to modify the repro repo to better represent the issue.

[JakenVeina]

Godspeed. :)

[giard-alexandre]

Ok, following up on the repro of this issue, it seems there IS a difference in how SortAndBind and .Sort().Bind() works! (Assuming I didn't make a horribly wrong assumption throughout all this 😅 ). The TreeDataGrid selection being lost is NOT caused by this though, that's still a TDG issue iteslf but as explained below: the changes applied to the bound collection when using SortAndBind are inconsistent (and, I suspect, less efficient).

The issue is reproduced under this specific branch: https://github.com/giard-alexandre/TickingTdg/tree/repro/dd-sortandbind-selection

If you go to this line in the viewmodel, you can comment out one or the other for testing. My findings are as follows:

Preface

• The stream sorts by FirstName
• Once you start the app, data only ticks once you enable it.
• Once data starts ticking, we Console.WriteLine both the change counts from the IChangeSet as well as the collection changes to the bound ReadOnlyObservableCollection.
• We would expect these logs to be the same for each operator but they are not (details below)
• We use TransformWithInlineUpdate to update the elements inline, as we're binding to a control in an MVVM framework so INotifyPropertyChanged is preferred.

When using .Sort().Bind()

✅ ChangeSets are propagating Refreshes, as expected
✅ As expected, we get some Move operations for the ReadOnlyObservableCollection elements since the sort is changing the order.

When using .SortAndBind()

✅ ChangeSets are still propagating Refreshes, as expected
❌ The ReadOnlyObservableCollection is getting Remove and Add operations instead of Move.

Comparison of outputs

`.Sort().Bind()`

`.SortAndBind`

.Sort().Bind // Begin adding initial items Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Adds: 20, Refreshes: 0, Removes: 0, Updates: 0 // End adding initial items. // Moves the items, as expected for a resort/refresh Collection Change Reason: Move Collection Change Reason: Move Collection Change Reason: Move // Items correctly being refreshed in the ChangeSet stream. Adds: 0, Refreshes: 4, Removes: 0, Updates: 0 // Repeat Collection Change Reason: Move Collection Change Reason: Move Collection Change Reason: Move Collection Change Reason: Move Adds: 0, Refreshes: 4, Removes: 0, Updates: 0 // seems moves aren't ALWAYS // necessary for each item changed // I assume that if the name did not change enough // to affect ordering, we get less than // 3 move operations per tick. Collection Change Reason: Move Collection Change Reason: Move Collection Change Reason: Move Adds: 0, Refreshes: 4, Removes: 0, Updates: 0 | | | |

SortAndBind: // Begin adding initial items Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Collection Change Reason: Add Adds: 20, Refreshes: 0, Removes: 0, Updates: 0 // End adding initial items. // Removes and adds items instead of moving, when doing a refresh. Collection Change Reason: Remove Collection Change Reason: Add Collection Change Reason: Remove Collection Change Reason: Add Collection Change Reason: Remove Collection Change Reason: Add Collection Change Reason: Remove Collection Change Reason: Add // Items correctly being refreshed in the ChangeSet stream. Adds: 0, Refreshes: 4, Removes: 0, Updates: 0 Collection Change Reason: Remove Collection Change Reason: Add Collection Change Reason: Remove Collection Change Reason: Add Collection Change Reason: Remove Collection Change Reason: Add Collection Change Reason: Remove Collection Change Reason: Add // Repeat, not all updates trigger a resort for all 4 updates Adds: 0, Refreshes: 4, Removes: 0, Updates: 0 Collection Change Reason: Remove Collection Change Reason: Add Collection Change Reason: Remove Collection Change Reason: Add Collection Change Reason: Remove Collection Change Reason: Add

Again, hopefully I'm not doing something dratically wrong but, seems to me that these 2 operators should be returning the same information!

[dwcullop]

Just to confirm: this is using the SourceList and not SourceCache? Also, could this have been addressed by #939?

[giard-alexandre]

Just to confirm: this is using the SourceList and not SourceCache? Also, could this have been addressed by #939?

It's using a SourceCache!
Also, I don't think this is addressed by that PR.

[JakenVeina]

Okay, so, implementation of Move changes is actually a tricky topic. There are definitely scenarios where you WANT to perform Add/Remove changes, instead of Moves, but that's obviously troublesome when it comes to some UI frameworks and bindings. Part of the strategy for dealing with this in older .Bind() operators was the concept of a binding adapter, which is a class responsible for actually applying Changes to target collections, in a way that is optimal for a particular scenario. Then you can have different adapter implementations that optimize for different things, or that pair up with specific UI frameworks. That concept wasn't translated into the initial implementation of .SortAndBind() and I'm not sure if its intended to come over later, or if the intention is to maybe just lean into the Options pattern.

I'll defer to @RolandPheasant for this one as well, he's done far more work with sorting implementations and operators than I have.

[giard-alexandre]

Ah yes that makes sense, I guess I may have been a little too keen to move to SortAndBind for the efficiency gains.
This does play nicely into the conversation we're having in #976 which, if I'm understanding things correctly, would mean that the best solution would be some kind of Bind operator that also maybe accept some kind of mapping method to define an inline property updater? This would turn row remove/adds into inline updates?

I'd be more than willing to look into getting a PR going for this if no one else has enough interest to look into but a little assistance to get pointed in the right direction would be extremely appreciated! 😄

[RolandPheasant]

I've just looked into this and I noticed it works and uses move instead remove and replace. Further investigation shows it was fixed here https://github.com/reactivemarbles/DynamicData/pull/936/files which has not yet been deployed.

I think it's time to deploy a new version!

[giard-alexandre]

Ah thank you! Good find!

[RolandPheasant]

@giard-alexandre I have deployed v9.1.1

Can you upgrade to that and let me know if the issue is fixed?

[giard-alexandre]

Confirmed that the new version does indeed fix this issue! Thanks again @RolandPheasant & @JakenVeina :)

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.