-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Force off-main-thread ItemsSource updates to update on main thread #11235
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build failing ...
'MarshalingObservableCollectionTests.MarshalingTestPlatformServices' does not implement interface member 'IPlatformServices.GetHash(string)'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
// collection which are made off of the main thread remain invisible to consumers on the main thread | ||
// until they have been processed by the main thread. | ||
|
||
public class MarshalingObservableCollection : List<object>, INotifyCollectionChanged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used by Platform.Android, so it needs to be public.
case IList _ when itemsSource is INotifyCollectionChanged: | ||
return new ObservableItemsSource(itemsSource as IList, notifier); | ||
case IList list when itemsSource is INotifyCollectionChanged: | ||
return new ObservableItemsSource(new MarshalingObservableCollection(list), notifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about iOS? Would it be necessary to use MarshalingObservableCollection
in iOS too?
@hartez, Still having this issue. My case.
|
Are you saying you have something like |
No, no. Let review the design. My collegue told me that the test regard this issue covers just the
Issue also happens when code is refactored to
|
Description of Change
If a developer
for the most part, these changes will be automatically marshaled by ObservableItemsSource to the main thread, and all will be well.
However, if the changes are timed correctly (or occur quickly enough) and there is any scrolling involved (e.g., using KeepLastItemInView, or opening/closing the keyboard, or an ill-timed user scrolling action), it's possible that the RecyclerView will attempt to access the underlying collection in a way which will make the viewholder collection inconsistent. At this point, the user may see errors like:
java.lang.IndexOutOfBoundsException: Inconsistency detected. Invalid item position
or
java.Lang.IndexOutOfBoundsException: Inconsistency detected. Invalid view holder adapter position
The issue is that the RecyclerView (via the adapter) is accessing the underlying source between the time the changes have been made (from a non-UI thread) and the time the adapter has been notified of the changes (which must happen on the UI thread).
To work around this problem, these changes provide a wrapper class which queues up the changes requested on the non-ui-thread and later processes them on the UI thread; in the intervening time, any requests for data are answered with a snapshot of the data before it was modified. The wrapper class is injected by the ItemsSourceFactory on Android.
Additionally, these changes address an issue where the ItemsViewRenderer was using the adapter's item count for scrolling rather than the item count in the layout manager. This would occasionally cause issues with the KeepLastItemInView option.
Issues Resolved
API Changes
Added to Core:
public class MarshalingObservableCollection
Platforms Affected
Behavioral/Visual Changes
None
Before/After Screenshots
Not applicable
Testing Procedure
Issue 10735 manual test
UI tests
Automated platform tests
PR Checklist