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

Add initial draft for "Enabling Batch Events for ObservableCollection" #320

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

terrajobst
Copy link
Member

@terrajobst terrajobst commented Aug 2, 2024

It's pretty clear that ObservableCollection<T> isn't a simple change to drive so I decided to write a design document that we can use at the beginning of .NET 10` to come up with a plan.

@eiriktsarpalis, if you got some time I'd appreciate your review on this.

@robertmclaws
Copy link

robertmclaws commented Aug 2, 2024

@terrajobst Thanks so much for putting this document together. It's fantastic and very well thought out.

I would point out in the note around contiguous blocks of items, I could be wrong but it appears that's only true if you set the Index properties in NotifyCollectionChangedEventArgs instead of using OldItems and NewItems.

One good thing that could come out of this would be simple guidance around the DOs and DON'Ts around using CollectionChanged (just like the NuGet public packaging documentation) so that everyone implements it consistently, and there can be explicit instruction not to throw exceptions in event handlers.

Might even be a good opportunity to think about marking OldStartingIndex and NewStartingIndex as obsolete if they lead to bad patterns or outcomes.

Also want to throw out there that a variant of the "Handle it in CollectionView" option could be to add an optional ObservableCollection constructor parameter to disable bulk events and default it to false, and then update CollectionView to use the true constructor variant.

  • Pro: It defaults to the new behavior but allows the WPF team to punt the changes to a future release without blocking anyone else or requiring the creation of global flags
  • Con: It wouldn't let WPF take advantage of the perf improvements until they fixed CollectionView correctly

Appreciate everyone's effort to try to make this happen in .NET 10!

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Thanks, this looks like a good write-up of the problem space. I wonder if we should try to commit to a specific solution before we merge this to the design repo. For the record I am supportive of the global switch approach since it does the right thing™️ and offers an off-ramp for impacted applications.

@terrajobst
Copy link
Member Author

@eiriktsarpalis

I wonder if we should try to commit to a specific solution before we merge this to the design repo.

I'd say yes. It's the primary reason why I marked it as draft. I would leave the other solutions in the document, with an explanation why we went with what we believe the best solution will be, for prosperity, but also has an alternative should the route we take become unworkable.

@terrajobst
Copy link
Member Author

@robertmclaws

Also want to throw out there that a variant of the "Handle it in CollectionView" option could be to add an optional ObservableCollection constructor parameter to disable bulk events and default it to false, and then update CollectionView to use the true constructor variant.

My understanding is that the party creating the ObservableCollection<T> is usually the author of the view model, i.e. the user, not the WPF infrastructure. So I'm not sure this helps much.

One way to deal with this in CollectionView would be to have customers ask for the collection view associated with a given ObservableCollection<T> (which I believe to be lazily allocated in a conditional weak table) and let the customer set properties on it, such as enabling/disabling bulk events. But don't understand WPF well enough to know whether that's a viable route.

I'm personally leaning towards the eventing adapter. We could have an extension method on INotifyCollectionChanged like ToSingleItemNotifications(). The instances could be stored in a conditional weak table which would avoid the need for consumers to keep track of them. This allows view models to be as simple as this:

public ObservableCollection<Customer> Customers { get; }

public INotifyCollectionChanged CustomersSingleItemEvents => Customers.ToSingleItemNotifications();

It's not entirely clear what the wrapper would need to implement in order to be useful and if we need a new public type to handle it. Maybe we could make it so that ObservableCollection<T> could wrap an existing ObservableCollection<T> so that modifying the data via either instance would raise events while one does bulking and one does single items. That would be most the flexible while not changing the API surface.

What I like about wrapping is that people don't need to change consuming code to subscribe to different events and that we don't need a brand-new collection type that view models would need to support.

@AmrAlSayed0
Copy link

AmrAlSayed0 commented Aug 7, 2024

How would ToSingleItemNotifications work? Because I could see bugs happening if we fire multiple events after the collection has been fully modified but the handler of say the first event assumes that only one modification has happened and accesses the collection based on that information.

Copy link

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together and providing a robust and definitive description of the issue! 🦙❤️

accepted/2024/observable_collection.md Show resolved Hide resolved
accepted/2024/observable_collection.md Outdated Show resolved Hide resolved
accepted/2024/observable_collection.md Show resolved Hide resolved
accepted/2024/observable_collection.md Show resolved Hide resolved
accepted/2024/observable_collection.md Outdated Show resolved Hide resolved
@terrajobst
Copy link
Member Author

@AmrAlSayed0

How would ToSingleItemNotifications work? Because I could see bugs happening if we fire multiple events after the collection has been fully modified but the handler of say the first event assumes that only one modification has happened and accesses the collection based on that information.

Excellent point. I believe you're correct -- this doesn't seem feasible. I've pushed 3450d45 to remove this option.

@robertmclaws
Copy link

robertmclaws commented Aug 8, 2024

I would recommend against a global switch, and here's why:

Let's say I have an application that I use to collect near-real-time data, and I choose to use WPF for the UI.

  • I use an ObservableCollection to store the results from an HTTP request that returns multiple items and repeats every 30 seconds.
  • The CollectionChanged event is monitored to drop the items into another ObservableCollection, which is bound to a WPF control to show the new items added on every cycle.

In this situation, I would want the first ObservableCollection to only throw a single CollectionChanged event to add the items to the second collection, but because of WPF I would then need the second ObservableCollection to throw single events so WPF doesn't break.

A global switch here would be detrimental because WPF's presence in my app means that NO ObservableCollection could ever be allowed to do use the new behavior. Just using WPF means your app is now less useful.

The developer would also be expecting certain behaviors that wouldn't happen and would have no indication at runtime why they wouldn't work (unless a Console warning was added in whenever the switch is on and the event is thrown).


I believe the only workable implementation alternative that doesn't force WPF to fix their problems would be to create a new property on ObservableCollection called NotificationMode to set the notification frequency, and then a change to CollectionView that sets the mode of its' connected collection to NotificationMode.Single.

The downside would be that something else could potentially witch the mode back, at which point the WPF controls would throw the expected exception, but maybe the exception could be modified to provide additional information about the need for setting the correct mode.

HTH!

@eiriktsarpalis
Copy link
Member

A global switch here would be detrimental because WPF's presence in my app means that NO ObservableCollection could ever be allowed to do use the new behavior. Just using WPF means your app is now less useful.

Understood, however I think the chicken-and-egg situation that both the libraries and WPF teams find themselves in is that we can't ship batched events OOTB because it breaks WPF and WPF can't fix their handlers because batched events haven't shipped yet. The point of a global flag is that it gives downstream consumers the ability to meaningfully fix any issues (as opposed to making speculative changes based on what one expects might break), at their own convenience, and without breaking the universe. I can see this happening in the following timeline:

  1. Libraries ship batched events with the global switch turned off by default in .NET 10 Preview 1.
  2. Consuming libraries including WPF work towards updating their handlers to support the new modality.
  3. Libraries flip the global switch to on by default (ideally by .NET 10 RC1 at the latest).

I realize that the situation between steps 1. and 2. is going to be less than ideal, but from my perspective it's the best way to safely progress the ecosystem towards batched events without introducing additional concepts in our APIs.

@terrajobst
Copy link
Member Author

terrajobst commented Aug 8, 2024

The point of a global flag is that it gives downstream consumers the ability to meaningfully fix any issues (as opposed to making speculative changes based on what one expects might break), at their own convenience, and without breaking the universe.

I think if we treat a global flag not as a shipping feature but as an "in between previews staging mechanism", the end result at GA is basically equivalent to "it just works". Now, this doesn't help other parties (say, Avalonia), which might force us to either add a global switch for GA or include an API.

However, I believe we can wait till the first previews and see how feasible it is to get most of the outside parties involved early on so have support by GA as well.

@eiriktsarpalis
Copy link
Member

I think if we treat a global flag not as a shipping feature but as an "in between previews staging mechanism"

I think shipping a global flag in GA is going to be inevitable for those apps using bespoke handlers that also don't account for batched events, and that is a good thing in my mind. The main open question is what the default value of said global flag is going to be in GA.

accepted/2024/observable_collection.md Show resolved Hide resolved
- > [!NOTE]
> For the same reason event adapters aren't viable just having a new
> interface is probably not sufficient. Rather, we'd need a different
> implementation like `BulkObservableCollection<T>`. However, for WinUI

Choose a reason for hiding this comment

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

We looked into this before and we concluded that actually we would not need a new interface at all for both WinUI 3 and UWP. INotifyCollectionChanged is projected to either Microsoft.UI.Xaml.Interop.INotifyCollectionChanged (for WinUI 3) or Windows.UI.Xaml.Interop.INotifyCollectionChanged (for UWP), and these interfaces already have all the support they need for multiple items (they kinda mirror the .NET event args). Which also means the underlying infrastructure (ie. WinUI 3 and UWP XAML controls) already work in term of that, we'd just need to do additional testing to be extra sure. But in general, all of this should "just work" for both WinUI 3 and modern UWP.

cc. @manodasanW

Choose a reason for hiding this comment

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

Yeah, I think if any work would be within like ListView vs. the projections of the data, similar to the work needed in WPF.

Choose a reason for hiding this comment

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

In theory, ListView and all other controls should also just work, because they've been built against that WinRT interface which also supports batch updates. As in, someone could've been sending batch updates already, eg. from C++. So I'd be surprised if those controls didn't support that scenario. Of course though, we should definitely test. But I'm saying at least the good news here is that on the .NET front, there should be no extra work/complexity 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

That's cool!

Presumably, the assumption still is that the ranges are contiguous, correct?

Copy link

@Sergio0694 Sergio0694 Aug 8, 2024

Choose a reason for hiding this comment

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

Looking at the properties for NotifyCollectionChangedEventArgs I assume so, yes. @jkoritzinsky do you perhaps know? Otherwise if we need we can reach out to XAML folks to confirm (anyone from the WinUI team), can't hurt.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the two event args types have a 1:1 projection; that is each can represent exactly the same set of states as the other. So if the .NET version only supports contiguous ranges, the WinRT versions also support only contiguous ranges.

Choose a reason for hiding this comment

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

Adding @ranjeshj who can maybe comment more on the UI work beyond the projection capabilities for the containerization aspects.

@michael-hawker
Copy link

I think if we treat a global flag not as a shipping feature but as an "in between previews staging mechanism"

I think shipping a global flag in GA is going to be inevitable for those apps using bespoke handlers that also don't account for batched events, and that is a good thing in my mind. The main open question is what the default value of said global flag is going to be in GA.

Beyond our own UX stacks where we know there's a problem, how many apps is this going to be though? If they're implementing something custom, isn't it usually to support batches for this interface? How much do we want to coddle invalid assumptions/shortcuts vs. just a cost of upgrading to the latest .NET and the cost of us having/maintaining separate paths?

Can we just better prepare folks in .NET 9 for this change? e.g. Is there an analyzer that could be shipped in .NET 9 to highlight when something implementing INotifyCollectionChanged doesn't handle all the Action cases? https://learn.microsoft.com/en-us/dotnet/api/system.collections.specialized.notifycollectionchangedaction (wondering if it could just detect each enum as they'd be referenced by a switch or set of if blocks normally, right? Would just be a warning if it doesn't see them all, probably a few patterns, but goes back to what real examples we have of these scenarios.)

@robertmclaws
Copy link

I think if we treat a global flag not as a shipping feature but as an "in between previews staging mechanism", the end result at GA is basically equivalent to "it just works".

If the global flag was transitory for the release cycle that would be a lot easier pill to swallow, certainly.

Would it be possible to add telemetry if the flag is set that warns the developer and points them to an aka.ms location that would explain what is happening?

@eiriktsarpalis
Copy link
Member

If the global flag was transitory for the release cycle that would be a lot easier pill to swallow, certainly.

I'm curious why you think that shipping a feature switch is problematic, regardless of its default value. We ship feature switches all the time for the most minor potential breaking change that we introduce, I don't see why we couldn't do the same for something this substantial.

@terrajobst
Copy link
Member Author

Would it be possible to add telemetry if the flag is set that warns the developer and points them to an aka.ms location that would explain what is happening?

Can you explain what you mean by "adding telemetry"? To me, this would mean that we'd just track how many people turns this on; this doesn't result in any kind of notification to the person enabling it.

@robertmclaws
Copy link

Can you explain what you mean by "adding telemetry"?

I mean like a Trace.Warn() or a Console.WriteLine(), or whatever standard Microsoft is using these days to insert diagnostic data into the runtime (OpenTelemetry, etc) so customers can analyze their own apps.

@robertmclaws
Copy link

robertmclaws commented Aug 8, 2024

I'm curious why you think that shipping a feature switch is problematic

I had hoped my detailed example showed that not being able to control it on a case-by-case basis would yield unpredictable results at runtime for developer customers. But I can put it a different way:

  • Picture I've written an API and a set of unit tests on .NET 10 that use ObservableCollection. I see the new Range features, and I turn the global flag on in my API code. I write unit tests that confirm I'm getting batched notifications, and all is right with the world.
  • Then I move onto UI, and I chose WPF. Now the Unit Tests will work (different executable, different flag setting) but the UI either crashes (flag on) or is just as slow as every other WPF app (flag off) without a clear explanation why.

If I experienced this, I'd be wicked pissed.

If that's a transitory situation through the .NET 10 release process that is thoroughly documented, that's one thing. If it ships in RTM and WPF decides to punt again, you'd be hard-pressed to counter the notion from customers that Microsoft is just enabling WPF to continue being lazy in not fixing their architectural flaws (as AFAIK every other UI framework has done work already for a feature like this).

In these types of situations, I would personally favor a solution that isolates the problem and creates the widest benefit (internal possibly-permanent NotificationMode property that can be set by CollectionView in a 3 new lines of code) vs a solution that creates more failure possibilities for early adopters (possibly-permanent all-or-nothing global flag that creates mixed-mode failures when enabled and likely causes devs to have to re-architect working code they just wrote).

But I also understand the intellectual consistency of "we use global flags for stuff like this so we'd do the same thing here too" and certainly wouldn't fault you for using it. I just wanted to provide a counter-viewpoint so you could see the situation from an "outside Microsoft" perspective.

I hope that is helpful. Thank you all so much for your hard work on this issue. :)

@terrajobst
Copy link
Member Author

I think there is a misunderstanding of what we mean by global switches. We usually think of them as compatibility switches that the app developer turns on, as opposed to a library author or a part of the .NET runtime, such as WPF.

In many cases the switches are not resulting in new behavior but rather restore previous behavior (i.e. they act as opting out from behavior that we identified as potentially disruptive).

In some cases we ship global switches for experimental features that consumers set in order to opt-into behavior that we would like to get feedback on. We have largely replaced this with <RequiresPreviewFeatures>. But the general principle of letting the app developer alone control this also holds here.

So in that sense I don't think the global switch would work as you describe, where console apps/unit tests experience one set of behaviors while WPF apps have a different behavior -- unless, of course, the app developer says so.

Here is what I could imagine we'd do:

  1. During previews we have a switch to turn this behavior on. This allows us to make progress, test various pieces, without interfering with parts of the product that don't care about this.

  2. If we can fix all core scenarios (WinForms, WPF, MAUI), we probably ship the feature turned on and provide a compatibility switch to turn it off. We'd still likely document it as a breaking change in order to communicate that there might be apps or external UI stacks that don't work with this new behavior.

  3. If we can't fix all the core scenarios (e.g. WPF is still broken) we'll either ship the experimental switch to turn this behavior on or we add an explicit API to turn it on (potentially on a per collection basis).

Does this make sense?

@robertmclaws
Copy link

robertmclaws commented Aug 9, 2024

Thanks for that explanation :) Ultimately I understand the purpose and how they are implemented. I'm just pointing out that:

a) A library developer could in fact set these switches. I run the BlazorEssentials library and I will likely turn that flag on in my IServiceCollectionExtensions that register that runtime, because Blazor doesn't have these legacy issues.

b) A developer could in fact set the switch to different values in different parts of their application framework that could cause problems they may or may not understand.

The global flag solution may solve the problem for you internally, but it is not "idiot-proof" externally. That's all I'm saying.

The only solution to that for the .NET 10 release cycle may just be good solid documentation about how the feature is supposed to work, what UI frameworks are planned for support, and how to deal with various situations.

And I'm just trying to point that out up front so it gets planned for and handled.

Sorry for beleaguering the point, I'll shut up now :)

@terrajobst
Copy link
Member Author

terrajobst commented Aug 9, 2024

The global flag solution may solve the problem for you internally, but it is not "idiot-proof" externally. That's all I'm saying.

Totally. My take is that if the final design requires setting a global flag to use bulk notifications, then we failed. If that's all we can do to ship the feature, I'd start by questioning if shipping it is even worth it, due to the very interop problems you cite.

I see global flags only as a temporary band-aid to allow customers to work around breaking changes while the ecosystem responds to it. The design of the feature must mean that eventually it just works.

Sorry for beleaguering the point, I'll shut up now :)

No no no, I created this very place so we talk about the implications of the design in depth! :-)

@eiriktsarpalis
Copy link
Member

A library developer could in fact set these switches. I run the BlazorEssentials library and I will likely turn that flag on in my IServiceCollectionExtensions that register that runtime

You mean calling AppContext.SetSwitch programmatically from library code? I would strongly recommend against doing this because of all the unpredictability it can give rise to, but your point remains that this is possible and it can result in unexpected crashes or slowdowns. I think we're all in agreement that there is going to be a transition period where such problems will exist, but ultimately I think we'll be able to find ourselves in a stable state where the switch is going to be turned on by default and all well-behaved apps (or libraries 🤢) will have no reason to turn it off.

@hez2010
Copy link

hez2010 commented Aug 14, 2024

All modern UI frameworks (Avalonia, WinUI 3, UWP, uno and etc.) support batch event out-of-box. You can simply use the following code to verify this:

public partial class MainWindow : Window
{
    private readonly MyViewModel _viewModel = new();
    public MainWindow()
    {
        InitializeComponent();
        DataContext = _viewModel;
    }

    private void Button_Click(object? sender, RoutedEventArgs e)
    {
        _viewModel.List.AddRange(["Item 1", "Item 2", "Item 3"]);
    }
}

public class MyViewModel
{
    public MyList<string> List { get; set; } = new();
}

public class MyList<T> : List<T>, INotifyCollectionChanged
{
    public event NotifyCollectionChangedEventHandler? CollectionChanged;

    public new void AddRange(IEnumerable<T> collection)
    {
        var index = this.Count;
        var list = collection.ToList();
        foreach (var item in collection)
        {
            Add(item);
        }
        CollectionChanged?.Invoke(this, new(NotifyCollectionChangedAction.Add, list, index));
    }
}

The issue here is only WPF doesn't support batch event, making all other frameworks pay the bill for WPF is not a great decision. The right way should really be adding batch APIs to ObservableCollection, while introducing NO feature switch, and adding batch event support in WPF.

@ThomasGoulet73
Copy link

I see WPF mentionned a lot but is it really a WPF specific problem ? I know WPF needs to be fixed for batching to be enabled but I think that WPF having this problem shows that other code bases probably also have this problem. Things like control vendors (Like DevExpress, Telerik, etc), open-source control libraries or any other code that hooks onto CollectionChanged could potentially silently break.

Also, code bases could look like it supports batching but since ObservableCollection never supported it, it probably wasn't thoroughly tested to make sure it works.

Even if WPF was fixed tomorrow I don't think enabling batching by default would be a wise decision.

My comment seems a bit negative but I'm supportive of allowing apps to enable batching one way or another.

@hez2010
Copy link

hez2010 commented Aug 22, 2024

Why not let it throw if it's not supported? Existing code still uses non-batch apis so that it will still work. New code developers will know whether batch apis are supported or not immediately after the exception being thrown.
I really don't see why we want to make sure batch apis work for all in-box workloads. We already have many APIs that don't work for some inbox workloads, what makes batch apis so special so that we want to ensure it always work on all inbox workloads?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.