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

Multiple issues in iOS CollectionView selection synchronization #14535

Open
filipnavara opened this issue Apr 12, 2023 · 5 comments
Open

Multiple issues in iOS CollectionView selection synchronization #14535

filipnavara opened this issue Apr 12, 2023 · 5 comments
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView delighter-sc platform/iOS 🍎 s/triaged Issue has been reviewed t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Milestone

Comments

@filipnavara
Copy link
Member

Description

There are at least three problems in the CollectionView SelectedItem/SelectedItems synchronization in iOS handler code:

void SynchronizePlatformSelectionWithSelectedItems()
{
var selectedItems = ItemsView.SelectedItems;
var selectedIndexPaths = CollectionView.GetIndexPathsForSelectedItems();
foreach (var path in selectedIndexPaths)
{
var itemAtPath = GetItemAtIndex(path);
if (ShouldNotBeSelected(itemAtPath, selectedItems))
{
CollectionView.DeselectItem(path, true);
}
}
foreach (var item in selectedItems)
{
SelectItem(item);
}
}
bool ShouldNotBeSelected(object item, IList<object> selectedItems)
{
for (int n = 0; n < selectedItems.Count; n++)
{
if (selectedItems[n] == item)
{
return false;
}
}

  1. The ShouldNotBeSelected method uses reference equality instead of Equals. If the item source returns a different instance of an object then this will fail and deselect items.
  2. The algorithm is actually O(n^2) complexity which is unacceptable. A HashSet could bring this down to more reasonable logarithmic complexity.
  3. If the user selects an item it updates SelectedItem/SelectedItems which in turns runs the selection synchronization unnecessarily:
MailClient.Mobile.UI.Views.ISelectView.IsSelectedPropChangedS (/Users/filipnavara/Projects/emclient-maui/MailClient.Mobile/MailClient.Mobile/UI/Views/ISelectView.cs:54)
Microsoft.Maui.Controls.BindableObject.ClearValue (:0)
Microsoft.Maui.Controls.BindableObject.ClearValue (:0)
Microsoft.Maui.Controls.Setter.UnApply (:0)
Microsoft.Maui.Controls.VisualStateManager.GoToState (:0)
Microsoft.Maui.Controls.Handlers.Items.TemplatedCell.UpdateVisualStates (:0)
Microsoft.Maui.Controls.Handlers.Items.TemplatedCell.set_Selected (:0)
ObjCRuntime.Messaging.void_objc_msgSend_NativeHandle_bool (:0)
UIKit.UICollectionView.DeselectItem (:0)
Microsoft.Maui.Controls.Handlers.Items.SelectableItemsViewController<Microsoft.Maui.Controls.ReorderableItemsView>.SynchronizePlatformSelectionWithSelectedItems (:0)
Microsoft.Maui.Controls.Handlers.Items.SelectableItemsViewController<Microsoft.Maui.Controls.ReorderableItemsView>.UpdatePlatformSelection (:0)
Microsoft.Maui.Controls.Handlers.Items.SelectableItemsViewHandler<Microsoft.Maui.Controls.ReorderableItemsView>.MapSelectedItems (:0)
Microsoft.Maui.PropertyMapper<Microsoft.Maui.Controls.CollectionView,Microsoft.Maui.Controls.Handlers.Items.CollectionViewHandler>. (:0)
Microsoft.Maui.PropertyMapper.UpdatePropertyCore (:0)
Microsoft.Maui.PropertyMapper.UpdateProperty (:0)
Microsoft.Maui.Handlers.ElementHandler.UpdateValue (:0)
Microsoft.Maui.Controls.Element.OnPropertyChanged (:0)
MailClient.Mobile.UI.Views.CollectionViewEx.OnPropertyChanged (/Users/filipnavara/Projects/emclient-maui/MailClient.Mobile/MailClient.Mobile/UI/Views/CollectionViewEx.cs:76)
Microsoft.Maui.Controls.SelectableItemsView.SelectedItemsPropertyChanged (:0)
Microsoft.Maui.Controls.SelectionList.Add (:0)
Microsoft.Maui.Controls.Handlers.Items.SelectableItemsViewController<Microsoft.Maui.Controls.ReorderableItemsView>.FormsSelectItem (:0)
Microsoft.Maui.Controls.Handlers.Items.SelectableItemsViewController<Microsoft.Maui.Controls.ReorderableItemsView>.ItemSelected (:0)
Microsoft.Maui.Controls.Handlers.Items.SelectableItemsViewDelegator<Microsoft.Maui.Controls.ReorderableItemsView,MailClient.Mobile.iOS.Handlers.ReorderableItemsViewController<Microsoft.Maui.Controls.ReorderableItemsView>>.ItemSelected (:0)
ObjCRuntime.Messaging.void_objc_msgSendSuper_NativeHandle_NativeHandle (:0)
UIKit.UIResponder.TouchesEnded (:0)
Microsoft.Maui.Platform.MauiSwipeView.TouchesEnded (:0)
UIKit.UIApplication.xamarin_UIApplicationMain (:0)
UIKit.UIApplication.UIApplicationMain (:0)
UIKit.UIApplication.Main (:0)
MailClient.Mobile.iOS.Application.Main (/Users/filipnavara/Projects/emclient-maui/MailClient.Mobile/MailClient.Mobile.iOS/Main.cs:17)

Steps to Reproduce

TBD

Link to public reproduction project repository

https://github.com/filipnavara/CollectionViewIsBroken

Version with bug

7.0 (current)

Last version that worked well

Unknown/Other

Affected platforms

iOS

Affected platform versions

iOS 16.4

Did you find any workaround?

No, short of copying the whole handler code and fixing it myself.

Relevant log output

No response

@filipnavara filipnavara added the t/bug Something isn't working label Apr 12, 2023
@drasticactions
Copy link
Contributor

@filipnavara
Copy link
Member Author

(I'm working on pushing a reduced sample; my machine is in a bit of disrepair after yesterday's updates)

@filipnavara
Copy link
Member Author

Checking Forms, it's the same logic there, copied to MAUI.

I wonder if XF was avoiding the problem number 3 because the mapping was done differently? I only noticed the first two because of the third one.

@filipnavara
Copy link
Member Author

Updated the sample: https://github.com/filipnavara/CollectionViewIsBroken

Notice that the user selection on tap doesn't "stick". Putting a breakpoint here shows that the item gets instantly deselected:
https://github.com/filipnavara/CollectionViewIsBroken/blob/84e9d406ce13d16a84e4ad9b3a5a744a3f585dd3/CollectionViewIsBroken/MainPage.xaml.cs#L86

@samhouts samhouts added platform/iOS 🍎 area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Apr 12, 2023
@jsuarezruiz jsuarezruiz added the legacy-area-perf Startup / Runtime performance label Apr 12, 2023
@jsuarezruiz jsuarezruiz added this to the Backlog milestone Apr 12, 2023
@ghost
Copy link

ghost commented Apr 12, 2023

We've added this issue to our backlog, and we will work to address it as time and resources allow. If you have any additional information or questions about this issue, please leave a comment. For additional info about issue management, please read our Triage Process.

@Zhanglirong-Winnie Zhanglirong-Winnie added the s/triaged Issue has been reviewed label Jan 4, 2024
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView delighter-sc platform/iOS 🍎 s/triaged Issue has been reviewed t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
None yet
Development

No branches or pull requests

6 participants