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

ReactiveList.AddRange throws "Range actions are not supported" given a list of length 2<=n<=10, under certain circumstances #1354

Closed
domyd opened this issue May 23, 2017 · 10 comments
Labels

Comments

@domyd
Copy link
Contributor

domyd commented May 23, 2017

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

What is the current behavior?
ReactiveList.AddRange and ReactiveList.InsertRange throw a System.NotSupportedException "Range actions are not supported." when passing them a list with at least 2, but at most 10 number of elements, under certain circumstances that are still not entirely clear to me (see below examples).

RxApp.SupportsRangeNotifications is true.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem
The "cleanest" yet oddest example I came up with was this specimen:

ViewModel

public class MainViewModel : ReactiveObject
{
    public MainViewModel()
    {
        // This works for any number of elements
        SearchResults.AddRange(Create(5));

        Observable.Start(() => { })
            .Do(_ =>
            {
                // This also works for any number of elements
                SearchResults.AddRange(Create(5));
            })
            .ObserveOn(RxApp.MainThreadScheduler)
            .Subscribe(_ =>
            {
                // This fails for [2..10] elements
                SearchResults.AddRange(Create(5));
            });

        IEnumerable<string> Create(int numElements)
            => Enumerable.Range(1, numElements).Select(i => $"item_{i}");
    }

    public ReactiveList<string> SearchResults { get; } = new ReactiveList<string>();
}

View

<Grid>
    <ListView ItemsSource="{Binding SearchResults}"></ListView>
</Grid>

... which manifests itself in a more realistic scenario, like the following:

ViewModel

public class MainViewModel : ReactiveObject
{
    public MainViewModel()
    {
        // This works for any number of elements
        SearchResults.AddRange(Create(5));

        Search = ReactiveCommand.Create(() => { });
        Search.Subscribe(_ =>
        {
            // This fails for [2..10] elements
            SearchResults.AddRange(Create(5));
        });
    }

    private static IEnumerable<string> Create(int numElements)
        => Enumerable.Range(1, numElements).Select(i => $"item_{i}");

    public ReactiveCommand<Unit, Unit> Search { get; }

    public ReactiveList<string> SearchResults { get; } = new ReactiveList<string>();
}

View

<Grid>
    <Grid.RowDefinitions>
        <RowDefinition Height="Auto"></RowDefinition>
        <RowDefinition Height="*"></RowDefinition>
    </Grid.RowDefinitions>
    <Button Command="{Binding Search}" Content="Search" Grid.Row="0"></Button>
    <ListView ItemsSource="{Binding SearchResults}" Grid.Row="1"></ListView>
</Grid>

Code-behind of the above two examples:

public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
        DataContext = new MainViewModel();
    }
}

However, the following Button.Click based example also triggers the error:

ViewModel

public class MainViewModel : ReactiveObject
{
    public MainViewModel()
    {
        // This works for any number of elements
        SearchResults.AddRange(Create(5));
    }

    public static IEnumerable<string> Create(int numElements)
        => Enumerable.Range(1, numElements).Select(i => $"item_{i}");

    public ReactiveList<string> SearchResults { get; } = new ReactiveList<string>();
}

View

<Grid>
    <Grid.RowDefinitions>
        <RowDefinition Height="Auto"></RowDefinition>
        <RowDefinition Height="*"></RowDefinition>
    </Grid.RowDefinitions>
    <Button Click="ButtonBase_OnClick" Content="Search" Grid.Row="0"></Button>
    <ListView ItemsSource="{Binding SearchResults}" Grid.Row="1"></ListView>
</Grid>
public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
        DataContext = new MainViewModel();
    }

    private void ButtonBase_OnClick(object sender, RoutedEventArgs e)
    {
        // This fails for [2..10] elements
        (DataContext as MainViewModel).SearchResults.AddRange(MainViewModel.Create(5));
    }
}

What is the expected behavior?
For ReactiveList.AddRange and ReactiveList.InsertRange to not throw "Range actions are not supported" in a NET45 application, for any non-null list, like, ever.

Which versions of ReactiveUI, and which platform / OS are affected by this issue? Did this work in previous versions of ReativeUI?

  • OS is Windows 10 version 1703 (Creator's Update) with VS 2017 Update 2.
  • Platform is WPF/.NET Framework 4.5.2 and 4.7 (likely any .NET Framework >= 4.5, but I only tested those two).
  • ReactiveUI 7.*, 6.5.2, 6.0.0, 5.2.0 and 5.1.2 definitely exhibit this issue. Version 5.0.2 works as expected! So I guess it's safe to assume this bug exists since 5.1.2.

Other information (e.g. stacktraces, related issues, suggestions how to fix)
ReactiveList.cs, line 357 is the one that throws the Exception in the latest release (7.4.0).

#363 appears to be about the same problem, and indeed it works fine using the SixReactiveList<T> posted in that issue. It looks like it should be fixed, yet here I am ... 😕

@domyd domyd changed the title ReactiveList.AddRange throws "Range actions are not supported" given a list of length 2 <= n <= 10, under certain circumstances ReactiveList.AddRange throws "Range actions are not supported" given a list of length 2<=n<=10, under certain circumstances May 23, 2017
@ghuntley
Copy link
Member

Thank-you for the comprehensive issue 💯

@domyd
Copy link
Contributor Author

domyd commented May 24, 2017

Least I could do :)

So I did some more digging; this is the (relevant part of the) stack trace:

   at System.Windows.Data.ListCollectionView.ValidateCollectionChangedEventArgs(NotifyCollectionChangedEventArgs e)
   at System.Windows.Data.ListCollectionView.ProcessCollectionChanged(NotifyCollectionChangedEventArgs args)
   at System.Windows.Data.CollectionView.OnCollectionChanged(Object sender, NotifyCollectionChangedEventArgs args)
   at ReactiveUI.ReactiveList`1.raiseCollectionChanged(NotifyCollectionChangedEventArgs args)
   at System.Reactive.AnonymousSafeObserver`1.OnNext(T value)
   at System.Reactive.Linq.ObservableImpl.Where`1._.OnNext(TSource value)
   at System.Reactive.Observer`1.OnNext(T value)
   at System.Reactive.Subjects.Subject`1.OnNext(T value)
   at ReactiveUI.ReactiveList`1.AddRange(IEnumerable`1 collection)

ListCollectionView.ValidateCollectionChangedEventArgs has this bit of code in it:

case NotifyCollectionChangedAction.Add:
    if (e.NewItems.Count != 1)
        throw new NotSupportedException(SR.Get(SRID.RangeActionsNotSupported));
    break;

So, uh, yeah ... from what I gather, ReactiveList emits NotifyCollectionChangedAction.Reset when the diff list is more than 10 items large, which WPF has no problem with. Adding 1 item also works fine in AddRange. That leaves me wondering why it only throws while WPF is databound to the list; I suspect somewhere along the way it probably takes a different path in that case?

It's also clear to me now why the SixReactiveList<T> from #363 worked, given that it just replaced .Add/.Remove/.Replace/.Move with .Reset when the list wasn't exactly 1 item long.

It appears that the fix in this case would simply be to only issue NotifyCollectionChangedArgs.Reset when on NET45? I'll give it a look, see if I can fix it.

@flyingxu
Copy link

flyingxu commented Aug 17, 2017

I'm having the same issue and I'm not sure if it's related to this issue #363. After I added RxApp.SupportsRangeNotifications = false as suggested by the PR of #593, I can use AddRange without any problem.

@domyd
Copy link
Contributor Author

domyd commented Aug 25, 2017

Yeah, if I recall correctly, ReactiveList will simply do an Add for each item if RxApp.SupportsRangeNotifications = false. The underlying issue, though, is that WPF inherently doesn't support range notifications, yet ReactiveList issues them.

However, now that WPF has been split into a separate package (right?), there might be a better way of solving this than in my PR.

@ElijahReva
Copy link
Contributor

I'm having the same issue. Output bound to Listbox ItemSource property.

Buggy version

ReactiveList<T> Output { get; }
...
this.Output.AddRange(data);

Workaround

using (this.Output.SuppressChangeNotifications())
{
  foreach (T expression in data)
  {
     this.Output.Add(expression);
   }
}

@stale
Copy link

stale bot commented Oct 30, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this because it helps reduce the workload of our extremely busy maintainers. If you want this issue progressed faster please start talks with a maintainer with how you can help out. There are so many ways to contribute to open-source and the most valued are often not code related. We are always looking for more help and look after those who stick around. Say howdy to one of the maintainers or browse https://reactiveui.net/contribute/ for ideas on where to start your journey.

@ElijahReva
Copy link
Contributor

Looking into it right now.

ElijahReva pushed a commit to ElijahReva/ReactiveUI that referenced this issue May 9, 2018
As WPF does not actually support NotifyCollectionChangedEventArgs where
more than one item is changed, we check for those cases first before
passing the request along to WPF (when running on net45).

fixes reactiveui#1354
@ElijahReva
Copy link
Contributor

Sorry for this weird commit feed. Was trying to remove corporative email from GIT_COMMITER_NAME and this resulted in several forced pushes. Could someone take a look at this?

@stale
Copy link

stale bot commented Jul 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want this issue progressed faster please start a conversation about raising a pull-request or coordinating your pull-request with a maintainer to get it merged. Understand that if folks like yourself don't contribute, ReactiveUI won't grow. You may or may not know this but ReactiveUI is maintained by unpaid volunteers. The maintainers put up a big marketing front but at it's core is a couple of passionate folks. ReactiveUI cares about open-source sustainability as maintainers have a serious load on their shoulders. Consumers shouldn't be naive in thinking that the latest update to a nuget package just magically materializes from the ethers. These things happen because our peers make them happen. No-one wants a tragedy of the commons situation. I urge you to get involved. Thank-you.

@glennawatson
Copy link
Contributor

We have dropped ReactiveList and ReactiveDerivedList in the next version of ReactiveUI and latest. Closing.

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

5 participants