Skip to content

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Oct 21, 2024

Description of Change

CarouselViewHandler on Android has a measurement logic that converts back and forth between sizes in different units (Context.FromPixels/Context.ToPixels). The logic in SizedItemContentView was inadvertently rounding the values, resulting in some size measurement having an off-by-one width on certain devices and emulators (eg. 1079 instead of 1080). This in turn resulted in the carousel being scrolled to incorrect item on re-measurement triggered from closing a modal page.

Before After
carousel-before carousel-after

Issues Fixed

@filipnavara filipnavara requested a review from a team as a code owner October 21, 2024 10:54
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Oct 21, 2024
@filipnavara filipnavara added platform/android area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution and removed community ✨ Community Contribution labels Oct 21, 2024
rmarinho
rmarinho previously approved these changes Oct 21, 2024
@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

The AddItemsToCarouselViewWorks UITest is failing on Android. In the test we add a new item and try to scroll to it.

This are the differences:
image
The scroll seems to not be working, the CurrentItem keeps the first item.

@filipnavara
Copy link
Member Author

filipnavara commented Oct 23, 2024

The AddItemsToCarouselViewWorks UITest is failing on Android.

I am not surprised it fails. I'm surprised it ever worked...

The item is added, which triggers MauiCarouselRecyclerView.CollectionItemsSourceChanged. This schedules the following code block to the dispatcher:

Carousel.
Handler.
MauiContext.
GetDispatcher()
.Dispatch(() =>
{
SetCurrentItem(carouselPosition);
UpdatePosition(carouselPosition);
//If we are adding or removing the last item we need to update
//the inset that we give to items so they are centered
if (e.NewStartingIndex == count - 1 || removingLastElement)
{
UpdateItemDecoration();
}
UpdateVisualStates();
});

Next, TestCarouselView.ScrollTo(5, animate: false); correctly scrolls the carousel to 5th item...
... which is then reset back by the dispatched code above.

Removing the delayed dispatch fixes this particular test. I'm not sure what was it supposed to handle. Unfortunately, this breaks this part of the logic:

// Queue the rest up for later after the Adapter has finished processing item change notifications

@filipnavara
Copy link
Member Author

I made a pragmatic change that should fix the test.

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@filipnavara
Copy link
Member Author

Seems like there was a lot of timeouts ... and a diff for the test above, which actually shows the fixed size:
image

@filipnavara
Copy link
Member Author

Was there a specific reason why this got closed?

@rmarinho
Copy link
Member

@filipnavara sorry it was my fault I screwed up the force push I think . Can you reopen with your local copy ? Thanks

@filipnavara
Copy link
Member Author

@filipnavara sorry it was my fault I screwed up the force push I think . Can you reopen with your local copy ? Thanks

Gotcha. I'll reopen and force push a rebased version.

@filipnavara filipnavara reopened this Jan 14, 2025
@rmarinho rmarinho self-assigned this Jan 14, 2025
@rmarinho rmarinho added this to the .NET 9 SR4 milestone Jan 14, 2025
@rmarinho rmarinho requested a review from jsuarezruiz January 14, 2025 14:49
@rmarinho
Copy link
Member

We need to update images tests for CarouselView

@PureWeen PureWeen removed this from the .NET 9 SR4 milestone Feb 10, 2025
@filipnavara
Copy link
Member Author

Is it possible to rebase and also add a UI test that replicates the issue?

I can rebase it but it will likely fail the UI tests again because of the changed baseline and the images will need to be updated. Frankly, it's becoming quite cumbersome doing this for several months now.

Replicating the issue with wrong scrolling may be a bit tricky. Replicating the wrong rounding resulting in wrongly sized items is already covered by existing tests.

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho rmarinho moved this from Changes Requested to Ready To Review in MAUI SDK Ongoing Mar 18, 2025
@rmarinho rmarinho requested a review from mattleibow March 18, 2025 16:42
@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

rmarinho
rmarinho previously approved these changes Mar 20, 2025
@rmarinho rmarinho moved this from Ready To Review to Approved in MAUI SDK Ongoing Mar 20, 2025
@rmarinho
Copy link
Member

/azp run MAUI-DeviceTests-public

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rmarinho
Copy link
Member

/azp run MAUI-public

@rmarinho
Copy link
Member

/azp run MAUI-UITests-public

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rmarinho rmarinho merged commit 173ca2b into dotnet:main Mar 21, 2025
126 of 128 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Done in MAUI SDK Ongoing Mar 21, 2025
@filipnavara
Copy link
Member Author

@rmarinho Thanks!

@filipnavara filipnavara deleted the sized-rounding branch March 21, 2025 10:32
@github-actions github-actions bot locked and limited conversation to collaborators Apr 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution partner/syncfusion/review platform/android
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants