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

[Android] CarouselView: Remove rounding from SizedItemContentView that results in off-by-one pixel error #25411

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

filipnavara
Copy link
Member

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:
carousel-before

After:
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
@PureWeen PureWeen added this to the .NET 9 SR5 milestone Feb 10, 2025
@PureWeen
Copy link
Member

I wonder if this is still an issue after

@PureWeen PureWeen added partner/syncfusion Issues / PR's with Syncfusion collaboration partner/syncfusion/review and removed partner/syncfusion Issues / PR's with Syncfusion collaboration labels Feb 10, 2025
@filipnavara
Copy link
Member Author

I wonder if this is still an issue after

The two PRs fixed unrelated issues even if they touched on the same topic.

@rmarinho
Copy link
Member

/rebase

@rmarinho
Copy link
Member

/rebase

@dotnet dotnet deleted a comment from azure-pipelines bot Feb 24, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Feb 24, 2025
@dotnet dotnet deleted a comment from PureWeen Feb 24, 2025
@rmarinho
Copy link
Member

/azp run

@dotnet dotnet deleted a comment from azure-pipelines bot Feb 24, 2025
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Feb 24, 2025
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 community ✨ Community Contribution partner/syncfusion/review platform/android 🤖
Projects
Status: Ready To Review
Development

Successfully merging this pull request may close these issues.

4 participants