RecyclerView doesn't reflect move operation from an ObservableCollection

[tttyy]

Steps to reproduce

1. Declare an ObservableCollection public ObservableCollection Collection {get;set;} = new ObservableCollection();

   // Initialize somewhere Collection.Add("1"); Collection.Add("2"); Collection.Add("3");

2. Bind to a RecyclerView using local:MvxBind="ItemsSource Collection"
3. Call Collection.Move(1, 0);

   Expected behavior

Should visually see the second row move up.

Actual behavior

Nothing visually happens

Configuration

Version: 4.x

[kjeremy]

Is this with 4.3? Can you please upload a reproduction on github.

[tttyy]

I can make one tonight if needed. But I kinda get the idea how it breaks.

The related code is in MvxRecyclerAdapter.cs NotifyDataSetChanged

case NotifyCollectionChangedAction.Move:
    for (int i = 0; i < e.NewItems.Count; i++)
    {
        var oldItem = e.OldItems[i];
        var newItem = e.NewItems[i];
        this.NotifyItemMoved(this.ItemsSource.GetPosition(oldItem), this.ItemsSource.GetPosition(newItem));
        this.RaiseDataSetChanged();
    }
    break;

By moving one entry, both e.NewItems and e.OldItems contains exact same entry. As a result, this.NotifyItemMoved is called with exact same numbers.

I think e.NewStartingIndex and e.OldStartingIndex should be utilized instead.

[kjeremy]

Hm. RaiseDataSetChanged isn't called there... are you looking at the right code?

[kjeremy]

It should probably be something like NotifyItemMoved(e.OldStartingIndex + i, e.NewStartingIndex + i)

[kjeremy]

Maybe our Replace is wrong too... according to https://msdn.microsoft.com/en-us/library/system.collections.specialized.notifycollectionchangedeventargs.olditems(v=vs.110).aspx it should be using OldItems

[tttyy]

BTW I copied code from 4.2. But the main problematic part still keeps. And you get the idea.

Another thing to point out, I'm not sure if the Move operation can have multiple items in OldItems & NewItems. The new & old ranges can overlap which would make things more complicated. But luckily ObeservableCollection seems to only allow moving one item at a time.

[kjeremy]

Ugh those indexes can be -1 in the case of single moves/replaces...

[kjeremy]

Can you create an adapter inheriting from MvxRecyclerAdapter and paste the following code into your adapter

public override void NotifyDataSetChanged(NotifyCollectionChangedEventArgs e)
        {
            try
            {
                switch (e.Action)
                {
                    case NotifyCollectionChangedAction.Add:
                        if (e.NewStartingIndex == -1)
                            goto case NotifyCollectionChangedAction.Reset;
                        NotifyItemRangeInserted(e.NewStartingIndex, e.NewItems.Count);
                        break;
                    case NotifyCollectionChangedAction.Move:
                        if (e.OldStartingIndex == -1 || e.NewStartingIndex == -1)
                            goto case NotifyCollectionChangedAction.Reset;
                        for (int i = 0; i < e.NewItems.Count; i++)
                        {
                            var oldPosition = e.OldStartingIndex + i;
                            var newPosition = e.NewStartingIndex + i;
                            NotifyItemMoved(oldPosition, newPosition);
                        }
                        break;
                    case NotifyCollectionChangedAction.Replace:
                        if (e.OldStartingIndex == -1)
                            goto case NotifyCollectionChangedAction.Reset;
                        NotifyItemRangeChanged(e.OldStartingIndex, e.OldItems.Count);
                        break;
                    case NotifyCollectionChangedAction.Remove:
                        if (e.OldStartingIndex == -1)
                            goto case NotifyCollectionChangedAction.Reset;
                        NotifyItemRangeRemoved(e.OldStartingIndex, e.OldItems.Count);
                        break;
                    case NotifyCollectionChangedAction.Reset:
                        NotifyDataSetChanged();
                        break;
                }
            }
            catch (Exception exception)
            {
                Mvx.Warning(
                    "Exception masked during Adapter RealNotifyDataSetChanged {0}. Are you trying to update your collection from a background task? See http://goo.gl/0nW0L6",
                    exception.ToLongString());
            }
        }

Then test your move and some other operations.

[tttyy]

Yea I did similar workaround for move and it worked.

[kjeremy]

Is that the fix though?

On Thu, Oct 27, 2016 at 4:52 PM, tttyy notifications@github.com wrote:

Yea I did similar workaround for move and it worked.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MvvmCross/MvvmCross-AndroidSupport/issues/310#issuecomment-256765654, or mute the thread https://github.com/notifications/unsubscribe-auth/AEIBRM4wqX_a0qkB3KVSeY5WuUSJf-mRks5q4Q8YgaJpZM4Kio0Q .

[tttyy]

Looks good to me. Thanks!