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

[Android] Fix for the CarouselView doesn't scroll to the right Position after changing the ItemSource with Loop enabled. #24431

Merged
merged 5 commits into from
Aug 28, 2024
Merged

[Android] Fix for the CarouselView doesn't scroll to the right Position after changing the ItemSource with Loop enabled. #24431

merged 5 commits into from
Aug 28, 2024

Conversation

Tamilarasan-Paranthaman
Copy link
Contributor

@Tamilarasan-Paranthaman Tamilarasan-Paranthaman commented Aug 26, 2024 •
edited
Loading

Root Cause

When reloading the ItemSource, the CurrentItem was set to null in the UpdateAdapter, which caused the UpdateInitialPosition method to miss calculating the itemCount based on the CurrentItem. As a result, the LoopScale value was incorrect in the LoopedPosition method, causing centerPosition to be set to 0 in the GetGoToIndex method of the CarouselViewLoopManager class. This led to a negative index position in the GetGoToIndex and prevented scrolling.

Description of change

Updated the ItemCount based on the Carousel loop and position, ensuring proper calculations to maintain the correct looped position value when reloading the items.

Issue Fixed

Fixes #17283

Before Fix

CarouselWithIssue.mp4

After Fix

CarouselLoopafterfix.mp4

@Tamilarasan-Paranthaman Tamilarasan-Paranthaman requested a review from a team as a code owner August 26, 2024 07:13
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Aug 26, 2024
@Tamilarasan-Paranthaman Tamilarasan-Paranthaman marked this pull request as draft August 26, 2024 07:14
@Ahamed-Ali Ahamed-Ali mentioned this pull request Aug 26, 2024
Closed
@rmarinho
Copy link
Member

rmarinho commented Aug 26, 2024 •
edited by jfversluis
Loading

/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 August 26, 2024 14:51
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen
PureWeen requested changes Aug 26, 2024
View reviewed changes
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue17283.cs Outdated
@@ -0,0 +1,27 @@
#if ANDROID
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to run this test on iOS and WinUI as well?

We typically let these tests run on all platforms even if the original issue is only happening on Android. This also lets us make sure that we have consistent behavior between all platforms.

If it's broken on iOS and/or WinUI then log a bug and make a note of the bug inside this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I have modified the script to run the test on both iOS and WinUI. Could you please take a look and let me know?

@rmarinho
Copy link
Member

rmarinho commented Aug 27, 2024 •
edited by jfversluis
Loading

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen
PureWeen requested changes Aug 27, 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.

it looks like windows is crashing

Just ignore windows and log an issue and then we'll be able to merge this one!

@samhouts samhouts added the area-controls-collectionview CollectionView, CarouselView, IndicatorView label Aug 27, 2024
@Tamilarasan-Paranthaman
Copy link
Contributor Author

it looks like windows is crashing

Just ignore windows and log an issue and then we'll be able to merge this one!

I have created a bug report for the CarouselView issue on Windows and added a note in the test. Could you please take a look into it ?

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

  • Failing test is unrelated

@PureWeen PureWeen merged commit 84a6544 into dotnet:main Aug 28, 2024
95 of 97 checks passed
@PureWeen PureWeen mentioned this pull request Aug 28, 2024
Open
@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 Sep 5, 2024
@samhouts samhouts added fixed-in-9.0.0-rc.2.24503.2 and removed fixed-in-net9.0-nightly This may be available in a nightly release! labels Oct 14, 2024
@samhouts samhouts removed the fixed-in-net8.0-nightly This may be available in a nightly release! label 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 approved these changes

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

@StephaneDelcroix StephaneDelcroix Awaiting requested review from StephaneDelcroix StephaneDelcroix 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 partner/syncfusion Issues / PR's with Syncfusion collaboration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Android] CarouselView doesn't scroll to the right Position after changing the ItemSource with Loop enabled
6 participants
@Tamilarasan-Paranthaman @rmarinho @PureWeen @samhouts @sheiksyedm @Ahamed-Ali