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.

Sign up for GitHub

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

Jump to bottom

[iOS] Fix for the CollectionView to prevent duplicate groups or crashes when adding a new item to a group or when adding a new group. #24873

Merged
merged 3 commits into from
Sep 26, 2024
Merged

[iOS] Fix for the CollectionView to prevent duplicate groups or crashes when adding a new item to a group or when adding a new group. #24873

merged 3 commits into from
Sep 26, 2024

Conversation

Tamilarasan-Paranthaman
Copy link
Contributor

Root Cause:

1. Add item in the group:

  • Dequeuing a supplementary view for measurement caused multiple instances of the header and footer to appear in the view.

2. Adding new group:

  • We checked if _collectionView.NumberOfSections() == 0 to determine whether a reload was required, in conjunction with the NotLoadedYet() condition.
  • However, using _collectionView.NumberOfSections() caused a crash when the collection view was empty and a new group was added, as NumberOfSections() == 0 is always true for an empty collection. This check prevented the adding a new group.

Description of Change:

1. Add item in the group:

  • We removed the version check and dequeuing logic to prevent the issue of duplicate header/footer templates.
  • Now, we always use MeasureSupplementaryView, which calculates the size without dequeuing the view, ensuring that the supplementary is only rendered when necessary.

2. Adding new group:

  • Modified the condition to check only if the collection view has not yet been loaded using the NotLoadedYet() method.
  • By removing the section check and focusing solely on whether the collection view has been loaded, we ensure that the data reloads properly in both empty and populated states.

Validated the behaviour in the following platforms,

  • Android
  • Windows
  • iOS
  • Mac

Issues Fixed

Fixes #17969
Fixes #20336

Screenshots

After Issue Fix

Screen.Recording.mov

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Sep 23, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Hey there @Tamilarasan-Paranthaman! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@Tamilarasan-Paranthaman Tamilarasan-Paranthaman added platform/iOS 🍎 area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Sep 23, 2024
@PureWeen
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Tamilarasan-Paranthaman Tamilarasan-Paranthaman marked this pull request as ready for review September 23, 2024 18:37
@Tamilarasan-Paranthaman Tamilarasan-Paranthaman requested a review from a team as a code owner September 23, 2024 18:37
@rmarinho
Copy link
Member

rmarinho commented Sep 23, 2024 •
edited by jfversluis
Loading

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

rmarinho
rmarinho reviewed Sep 24, 2024
View reviewed changes
Copy link
Member

@rmarinho rmarinho left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this shifts the problem a little bit, if the REloadRequired is still called it will crash. Should we instead of checking the NumberofSections of the CollectionView can we try look at the ItemsSource Count ?

PureWeen
PureWeen requested changes Sep 24, 2024
View reviewed changes
Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#24873 (review)

@Tamilarasan-Paranthaman
Copy link
Contributor Author

I think this shifts the problem a little bit, if the REloadRequired is still called it will crash. Should we instead of checking the NumberofSections of the CollectionView can we try look at the ItemsSource Count ?

@rmarinho / @PureWeen, currently, ReloadRequired is only called when removing a group in the Remove method. In the case of group removal, it make sense to check if there are any groups available to remove. However, this check is unnecessary when adding a group. Additionally, this check alone was not the cause of the crash. The actual issue occurred when a data reload was triggered inside the if condition, leading to the crash.

To explain in more detail: when the ReloadRequired condition is true, the code enters the if block and calls ReloadData and ResetGroupTracking on the CollectionView. This is what caused the crash.

This issue specifically arises when clearing the ItemsSource and immediately adding a group, as reported by the customer. However, the crash does not occur if these actions are performed individually.

It seems that during a reset, we call Reload(true), which internally invokes _collectionView.LayoutIfNeeded() based on the boolean parameter. Similarly, an additional layout call may be required when adding a group immediately after clearing. The issue is resolved if we call _collectionView.LayoutIfNeeded, which can be achieved by using Reload(true) in the group-adding case as well.

However, it seems that the InsertSections method of the CollectionView handles this. Therefore, I believe checking the NumberOfSections (which will be 0) is unnecessary in this scenario. I have resolved the issue by removing the ReloadRequired check.

Could you please share your thoughts on this approach?

rmarinho
rmarinho approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification, this solution looks good and addresses the root cause of the crash effectively. The explanation clearly outlines how the issue occurs, and removing the unnecessary ReloadRequired check when adding a group seems like a smart optimization.
This PR is good to go, and I approve it. Excellent work in identifying the underlying cause and providing a clear solution!

@FlavioGoncalves-Cayas
Copy link

FlavioGoncalves-Cayas commented Sep 26, 2024 •
edited
Loading

@Tamilarasan-Paranthaman
Thanks for this PR. This looks very promising. I tested this with my reproduction for #17969 and the issue seems to be fixed.
Also tested this in the app where I initially encountered this issue. It also works there.

@FlavioGoncalves-Cayas FlavioGoncalves-Cayas mentioned this pull request Sep 26, 2024
Closed
@rmarinho rmarinho merged commit a5a6625 into dotnet:main Sep 26, 2024
97 checks passed
@velocitysystems velocitysystems mentioned this pull request Sep 27, 2024
Closed
@PureWeen
Copy link
Member

PureWeen commented Oct 1, 2024

/backport to release/8.0.1xx-sr9

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 1, 2024

Started backporting to release/8.0.1xx-sr9: https://github.com/dotnet/maui/actions/runs/11127580408

@github-actions github-actions bot mentioned this pull request Oct 1, 2024
Merged
@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Oct 1, 2024
@drasticactions drasticactions mentioned this pull request Oct 9, 2024
Closed
@PureWeen PureWeen added this to the .NET 8 SR9.2 milestone Oct 14, 2024
@samhouts samhouts added fixed-in-9.0.0-rc.2.24503.2 fixed-in-net8.0-nightly This may be available in a nightly release! and removed fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Oct 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2024
@sheiksyedm sheiksyedm added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Dec 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PureWeen PureWeen PureWeen requested changes

@rmarinho rmarinho rmarinho approved these changes

@jsuarezruiz jsuarezruiz Awaiting requested review from jsuarezruiz jsuarezruiz is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

@tj-devel709 tj-devel709 Awaiting requested review from tj-devel709 tj-devel709 is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

Assignees
No one assigned
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution fixed-in-9.0.0-rc.2.24503.2 fixed-in-net8.0-nightly This may be available in a nightly release! partner/syncfusion Issues / PR's with Syncfusion collaboration platform/iOS 🍎
Projects
MAUI SDK Ongoing
Status: Done
Milestone
.NET 8 SR9.2
6 participants
@Tamilarasan-Paranthaman @PureWeen @rmarinho @FlavioGoncalves-Cayas @samhouts @sheiksyedm