[UWP] Fixes CollectionChanged events in ListView

[paymicro]

Description of Change

CollectionChanged events are fired, when collection changed in user code.

Issues Resolved

• fixes UWP Regression: Drag and drop reordering not firing CollectionChanged #3273

API Changes

/

Platforms Affected

• UWP

PR Checklist

• Has automated tests
• Rebased on top of the target branch at time of PR
• Changes adhere to coding standard

[PureWeen]

So I don't think this is quite the right fix for this. Those events should just happen based on the ListView reflecting its changes onto the source list

I'm thinking this is the change that broke this
https://github.com/xamarin/Xamarin.Forms/pull/2512/files

I don't think the added UITest quite reflects the issue reported. The issue reported is using a renderer to enable reordering on the list

control.AllowDrop = true;
                control.CanDragItems = true;
                control.CanReorderItems = true;
                control.ReorderMode = Windows.UI.Xaml.Controls.ListViewReorderMode.Enabled;

So the changes in that repro are coming from the native ListView itself and propagated to the ItemSource on the native listview.

Now that there is a disconnect between the ProxyList bound to the native Listview and the users List

			var allSourceItems = new ObservableCollection<object>();

			if (Element?.ItemsSource != null)
			{
				foreach (var item in Element.ItemsSource)
					allSourceItems.Add(item);
			}

Those changes don't propagate to the Element?.ItemsSource.

But then you kind of get into a ping pong effect because now when syncing the proxy list down, the original list tries to sync back up to the proxy.

Just to test it out a bit here's a quick hack I did just to test a way to fix. I don't think this is the best solution but it's a direction

bool ignoreSTuff = false;
		/// <summary>
		/// Call all methods subscribed to the "CollectionChanged" event in the original ItemsSource
		/// </summary>
		void OnCollectionChangedInUserCode(object sender, NotifyCollectionChangedEventArgs e)
		{
			ignoreSTuff = true;
			if (Element?.ItemsSource is IList list)
			{
				switch (e.Action)
				{
					case NotifyCollectionChangedAction.Add:
						if (e.NewStartingIndex >= SourceItems.Count)
						{
							for (int i = 0; i < e.NewItems.Count; i++)
								list.Add((e.NewItems[i]));
						}
						else
						{
							for (int i = e.NewItems.Count - 1; i >= 0; i--)
								list.Insert(e.NewStartingIndex, (e.NewItems[i]));
						}

						break;
					case NotifyCollectionChangedAction.Remove:
						for (int i = e.OldItems.Count - 1; i >= 0; i--)
							list.RemoveAt(e.OldStartingIndex);
						break;
				}
			}
			ignoreSTuff = false;
		}

		void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
		{
			if (ignoreSTuff)
				return;

[paymicro]

Hi @PureWeen. Thanks for review!

I don't think the added UITest quite reflects the issue reported. The issue reported is using a renderer to enable reordering on the list

I tested the behavior when you turn on drag & drop and it is no different from the call Move elements of SourceItems. So I think that this test completely reflects the issue.

I refactored the code in PR. And now wrapping in ObservableCollecion<object> happens only if it is necessary.

[PureWeen]

I tested the behavior when you turn on drag & drop and it is no different from the call Move elements of SourceItems. So I think that this test completely reflects the issue.

The issue is where the event starts from. When you're moving things around in the UI the propagation is

Native List View => Moves Items on Its proxy list (which is different than the list the user knows about)=> propagates => and now this change needs to propagate to the list the user knows about

The UI Test here goes the opposite direction
User calls Move on the list they know about => changes propagate to proxy => proxy propagates to Native List View

The other reason I don't feel like the UI test properly reflects the changes is that if I roll back the changes in the PR the UI Test still gives me a "Success"

I checked in a variation of the unit test here.
https://github.com/xamarin/Xamarin.Forms/tree/fix-gh3273
I rolled back the fix in this PR just to demonstrate how clicking the button will fire the CollectionChanged Event and set the Label to Success. Whereas when dragging the items around the CollectionChanged never fires which is what the original issue is about

That being said your latest changes look like they propagate correctly (I still need to digest them a bit) though it looks like there are some cases a null ItemSource causes NRE Exception in the code.

For example if you just add a blank list view

			Content = new StackLayout
			{
				Children = {
					statusLabel,
					actionLabel,
					new Button {
						Text = "Move items",
						Command = new Command(() =>
						{
							actionLabel.Text = string.Empty;
							statusLabel.Text = "Failed";
							Items.Move(0, 1); })
					},
					listView,
					new ListView()
				}
			};

It causes NRE

[paymicro]

@PureWeen thank you for such a comprehensive review and for your UI test! 👍 This helped me a lot. I hope that ListView will work more correctly =)

[PureWeen]

@paymicro This resorting from the UWP side of things has made this an interesting one :-/

Does fixing it this way unwind this PR a bit too far?
#2512

Which fixed this
#2485

I think to stay inline with the PR above and the issues that it fixed this logic has to be inverted

				if (IsObservableCollection(Element?.ItemsSource.GetType()))
				{
					_collection = (IList)Element?.ItemsSource;
					_collectionIsWrapped = false;
				}
				else
				{
					_collection = new ObservableCollection<object>();
					foreach (var item in Element.ItemsSource)
						_collection.Add(item);
					_collectionIsWrapped = true;
				}

And we only wrap collections that implement INotifyCollectionChanged which I realize conflicts with the intent of this PR.

It seems like the only way to make all the issues happy would be to

• wrap an ItemSource if it implements INotifyCollectionChanged
• If the ItemSource then also IList then changes made to the Wrapper need to propagate to the users list

[velocitysystems]

Great work @paymicro @PureWeen!

Love the detection of whether the source collection is observable vs wrapping it in a proxy collection. Having two disconnected ObservableCollection's didn't really make sense. This makes ListView behave much more like the native control (and as it did before!).

[velocitysystems]

@paymicro @PureWeen Just following up - what is the current status on this PR?

[paymicro]

@PureWeen this fix correctly works on UItest 56771. ( #2512 #2485 )
UWP ListView does not work correctly with the inheritors of the ObservableCollection class, therefore not enough wrap an ItemSource if it not implements INotifyCollectionChanged. We need to wrap everything that is not an ObservableCollection.
I can add synchronization if the original ItemSource implements INotifyCollectionChanged, but is not an ObservableCollection class. It necessary to do?

[PureWeen]

@paymicro this looks like it's breaking grouping

If you check UI test Bugzilla32148
The grouped items no longer show up on UWP
If I roll back this PR they do show up

I can add synchronization if the original ItemSource implements INotifyCollectionChanged, but is not an ObservableCollection class. It necessary to do?

I'd say this isn't necessary. I messed with this idea a little bit and from what I can tell UWP only syncs changes back to the source list if the source list is an ObservableCollection. I create a class that implemented List that implemented from INotifyCollectionChanged and the UWP ListView didn't sync back to it. I think it'll add an unnecessary level of complexity for us to sync back like that. If the user is creating renderers to modify the native ListView on UWP then they can just use an ObservableCollection :-)

[PureWeen]

Getting closer :-)

This change is causing Issue1975 to crash on UWP

[PureWeen]

Can you add
[Preserve (AllMembers = true)]
To this?

Android gets annoyed with out it which will sometimes mess up UI Tests :-/

[PureWeen]

Can you annotate this with the ListView UI Test Category?

[paymicro]

This change is causing Issue1975 to crash on UWP

This looks like issue #2959. I tried to fix it, but without success. Maybe it's crash inside UWP...

[velocitysystems]

@paymicro @PureWeen Thanks for all your hard work on this. What is left to do here?

[jassmith]

build --uitests

[PureWeen]

Alright @paymicro @samhouts let me know what you think of the changes I just checked in

I removed the WinRT binding hack that was being used and that fixed the AccessViolationError
From what I can tell going via binding was keeping the WeakEventHandler alive in ListProxy longer then it needed to be

I also added a missing using statement. I do agree that there seems to be some odd issue internally with how CVS interacts with LV. If I set the ItemsSource on the UWP ListView to null it throws an AccessViolationException still

I ran all the UWP tests marked as ListView and they all passed

I don't think this is related to #2959

[PureWeen]

build --uitests

[PureWeen]

rebased to master to fix those android targeting errors

[velocitysystems]

@samhouts Will this make it into 3.2.0 RC?

[velocitysystems]

I know this is slated for 3.3.0, but any chance of this making a 3.2.0 service release?