-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Android] SpacingItemDecoration improvement #27093
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
base: main
Are you sure you want to change the base?
[Android] SpacingItemDecoration improvement #27093
Conversation
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
src/Controls/src/Core/Handlers/Items/Android/MauiRecyclerView.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
It also fixes the issue #18367 |
Thank you for letting me know! |
It would be really nice to get it in .NET 8 too <3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kubaflo I re run but seems we have tests failing still . Can you look at this one CollectionViewWithSpacingShouldScroll ? Maybe screenshot is wrong?
also the other failures on iOS and cache one on Android seems it-s because this branch needs rebase on main. As main is green right now.
7f2e1a7
to
cd57f86
Compare
@rmarinho Done. Rebased and re-uploaded the screenshot. |
Yeah sorry, but all the ongoing changes can only be in .NET 9 |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
cd57f86
to
17e0e29
Compare
Thank you very much for the fix of an issue that is at least 3 years old at this point. |
Also facing this issue -. in Android, the Scroll is not smooth and stops. |
Bumping this again - since it is performance related, and CV where so many users have issues :( |
Its so annoying to live with this issue for so many years! |
17e0e29
to
522aa0b
Compare
/rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the spacing behavior of CollectionView on Android by fully utilizing ItemDecoration for handling item spacing, thereby eliminating the unintended effects caused by vertical padding on the RecyclerView. Key changes include new test cases for Issue24511, updated spacing offset calculations in SpacingItemDecoration, and revised padding adjustments in MauiRecyclerView to account for grid orientation.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
TestCases.Shared.Tests/Tests/Issues/Issue24511.cs | Adds a UI test case to verify scrolling behavior with the improved spacing logic. |
TestCases.HostApp/Issues/Issue24511.xaml.cs | Hosts the updated CollectionView with the new spacing approach and sample data. |
TestCases.HostApp/Issues/Issue24511.xaml | Updates the CollectionView layout to support the new spacing mechanism. |
Handlers/Items/Android/SpacingItemDecoration.cs | Revises the offset calculation logic for grid and linear layouts to remove external spacing around the RecyclerView. |
Handlers/Items/Android/MauiRecyclerView.cs | Adjusts padding logic based on grid orientation to complement the new ItemDecoration implementation. |
Comments suppressed due to low confidence (3)
src/Controls/src/Core/Handlers/Items/Android/SpacingItemDecoration.cs:99
- Consider adding tests for scenarios where the total number of items is not a multiple of the span value to ensure that the edge offsets are correctly applied for horizontal grids.
if (position < _span)
src/Controls/src/Core/Handlers/Items/Android/MauiRecyclerView.cs:484
- [nitpick] Review the new padding logic for horizontal grid layouts to ensure that setting the horizontal padding to zero does not inadvertently affect the layout in edge cases.
SetPadding(0, verticalPadding, 0, verticalPadding);
src/Controls/src/Core/Handlers/Items/Android/MauiRecyclerView.cs:489
- [nitpick] Verify that for vertical grid layouts the adjustment of horizontal padding exclusively via ItemDecoration does not leave behind unintended spacing at the outer edges.
SetPadding(horizontalPadding, 0, horizontalPadding, 0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Just a nit on the double amount of code in the checks.
But also, maybe add another test for the grid in the other orientation just to make sure we are not missing anything.
int column = position / _span; | ||
int totalColumns = (int)Math.Ceiling((double)itemCount / _span); | ||
|
||
if (position < _span) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is different with the <
. Is this intentional? This is a horizontal grid, which means it is a grid of _span rows or all the items in the first column. Which is basically column == 0?
} | ||
} | ||
} | ||
else if (_itemsLayout is LinearItemsLayout linearItemsLayout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this can be condensed into a single level where we just look at vertical/horizontal. I worry for the big block of code that is basically a duplicate. Then we also don't need the _itemsLayout
field.
The grid has extra "steps", but a list basically has a span of 1.
This can maybe be condensed to a check of:
int rowCol = _span <= 1 ? position : position / _span;
int totalRowsCols = _span <= 1 ? itemCount : (int)Math.Ceiling((double)itemCount / _span);
int lastRowCol = totalRowsCols - 1;
if (linearItemsLayout.Orientation == ItemsLayoutOrientation.Vertical)
{
if (rowCol == 0)
{
outRect.Top = 0;
}
else if (rowCol == lastRowCol)
{
outRect.Bottom = 0;
}
}
else
{
if (rowCol == 0)
{
outRect.Left = 0;
}
else if (rowCol == lastRowCol)
{
outRect.Right = 0;
}
}
Screenshot added.
Description of Change
Vertical padding set on the recyclerView affected the rendering process. It is much more efficient to fully rely on ItemDecoration to manage spacing between items.
Issues Fixed
Fixes #24511
Fixes #8422
Fixes #18367
Fixes #17127
Screen.Recording.2025-01-13.at.11.51.36.mov
Screen.Recording.2025-01-13.at.11.49.06.mov
Screen.Recording.2025-01-14.at.10.49.40.mov
Screen.Recording.2025-01-14.at.10.46.01.mov