Proposal: throw from ItemChanging / ItemChanged when ChangeTrackingEnabled is false

[kentcb]

Note: for support questions, please ask on StackOverflow: https://stackoverflow.com/questions/tagged/reactiveui . This repository's issues are reserved for feature requests and bug reports.

Do you want to request a feature or report a bug?

Widen pit of success for consumers.

What is the current behavior?

If you hook into ItemChanging or ItemChanged on a ReactiveList<T> nothing happens unless you've also set ChangeTrackingEnabled to true. I've never really understood why this is a good idea and feel that we should either:

1. Throw when accessing ItemChanging / ItemChanged when ChangeTrackingEnabled is false. The exception message should obviously point the developer in the right direction and explain why change tracking isn't enabled by default (perf).
2. Remove ChangeTrackingEnabled altogether and automatically enable it when needed (i.e. when ItemChanging or ItemChanged are subscribed to - not just accessed)

Obviously both changes are potentially breaking, with 2 being the bigger break. The point of this issue is to come to a consensus as to which course of action we take, if any.

I feel like there must be use cases I'm unaware of that justify the way this class currently works. I'd be very interested to hear from people who rely on ItemChanging / ItemChanged in such a way that this proposal would break their use case.

What is the expected behavior?

To me, the expected behavior is for it to just work. If I hook into ItemChanging or ItemChanged, that should ensure the requisite infrastructure is in place to support it. At worst, I would expect some kind of exception telling me I need to enable that feature before those observables do anything.

What is the motivation / use case for changing the behavior?

Widen the pit of success for consumers.

Which versions of ReactiveUI, and which platform / OS are affected by this issue? Did this work in previous versions of ReativeUI? Please also test with the latest stable and snapshot (http://docs.reactiveui.net/en/contributing/snapshot/index.html) versions.

7

[Qonstrukt]

Hmm, we disable ChangeTrackingEnabled temporarily when we're refilling the list completely. Where we want gradual updates for the remainder of the time. Now we just have to toggle the property, and we can leave the subscriptions in place. Pretty workable system for us to be honest!

[kentcb]

@Qonstrukt Thanks for the feedback! That makes perfect sense and I'm happy to hear there's a valid scenario for this.

I do think it would have made more sense to include a SuppressItemChangeNotifications method alongside the existing SuppressChangeNotifications, but I guess that's water under the bridge now. I mean, we could deprecate ChangeTrackingEnabled and introduce a replacement SuppressItemChangeNotifications, but unless this becomes an issue in the future then there's probably no great need.

Closing for now.