-
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?
Changes from all commits
2583243
ac4ad86
ac59eac
2a5f847
522aa0b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,10 @@ public class SpacingItemDecoration : RecyclerView.ItemDecoration | |
|
||
public int VerticalOffset { get; } | ||
|
||
int _span = 1; | ||
|
||
IItemsLayout _itemsLayout; | ||
|
||
public SpacingItemDecoration(Context context, IItemsLayout itemsLayout) | ||
{ | ||
// The original "SpacingItemDecoration" applied spacing based on an item's current span index. | ||
|
@@ -35,6 +39,7 @@ public SpacingItemDecoration(Context context, IItemsLayout itemsLayout) | |
case GridItemsLayout gridItemsLayout: | ||
horizontalOffset = gridItemsLayout.HorizontalItemSpacing / 2.0; | ||
verticalOffset = gridItemsLayout.VerticalItemSpacing / 2.0; | ||
_span = gridItemsLayout.Span; | ||
break; | ||
case LinearItemsLayout listItemsLayout: | ||
if (listItemsLayout.Orientation == ItemsLayoutOrientation.Horizontal) | ||
|
@@ -56,16 +61,76 @@ public SpacingItemDecoration(Context context, IItemsLayout itemsLayout) | |
|
||
HorizontalOffset = (int)context.ToPixels(horizontalOffset); | ||
VerticalOffset = (int)context.ToPixels(verticalOffset); | ||
_itemsLayout = itemsLayout; | ||
} | ||
|
||
public override void GetItemOffsets(ARect outRect, AView view, RecyclerView parent, RecyclerView.State state) | ||
{ | ||
base.GetItemOffsets(outRect, view, parent, state); | ||
|
||
int position = parent.GetChildAdapterPosition(view); | ||
int itemCount = state.ItemCount; | ||
outRect.Left = HorizontalOffset; | ||
outRect.Right = HorizontalOffset; | ||
outRect.Bottom = VerticalOffset; | ||
outRect.Top = VerticalOffset; | ||
|
||
|
||
if (_itemsLayout is GridItemsLayout gridItemsLayout) | ||
{ | ||
if (gridItemsLayout.Orientation == ItemsLayoutOrientation.Vertical) | ||
{ | ||
int row = position / _span; | ||
int totalRows = (int)Math.Ceiling((double)itemCount / _span); | ||
|
||
if (row == 0) | ||
{ | ||
outRect.Top = 0; | ||
} | ||
else if (row == totalRows - 1) | ||
{ | ||
outRect.Bottom = 0; | ||
} | ||
} | ||
else | ||
{ | ||
int column = position / _span; | ||
int totalColumns = (int)Math.Ceiling((double)itemCount / _span); | ||
|
||
if (position < _span) | ||
{ | ||
outRect.Left = 0; | ||
} | ||
else if (column == totalColumns - 1) | ||
{ | ||
outRect.Right = 0; | ||
} | ||
} | ||
} | ||
else if (_itemsLayout is LinearItemsLayout linearItemsLayout) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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;
}
} |
||
{ | ||
if (linearItemsLayout.Orientation == ItemsLayoutOrientation.Vertical) | ||
{ | ||
if (position == 0) | ||
{ | ||
outRect.Top = 0; | ||
} | ||
else if (position == itemCount - 1) | ||
{ | ||
outRect.Bottom = 0; | ||
} | ||
} | ||
else | ||
{ | ||
if (position == 0) | ||
{ | ||
outRect.Left = 0; | ||
} | ||
else if (position == itemCount - 1) | ||
{ | ||
outRect.Right = 0; | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<ContentPage xmlns:controls="clr-namespace:Maui.Controls.Sample.Issues" | ||
xmlns="http://schemas.microsoft.com/dotnet/2021/maui" | ||
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" | ||
x:Class="Maui.Controls.Sample.Issues.Issue24511"> | ||
<CollectionView BackgroundColor="Green" | ||
AutomationId="CV" | ||
x:Name="CV"> | ||
<CollectionView.ItemsLayout> | ||
<GridItemsLayout | ||
Orientation="Vertical" | ||
HorizontalItemSpacing="10" | ||
Span="3" | ||
VerticalItemSpacing="10"/> | ||
</CollectionView.ItemsLayout> | ||
<CollectionView.ItemTemplate> | ||
<DataTemplate> | ||
<VerticalStackLayout BackgroundColor="Blue"> | ||
<Image | ||
Source="dotnet_bot.png" | ||
BackgroundColor="Red" | ||
HorizontalOptions="Fill"/> | ||
<Label Text="Hello, .NET MAUI!"/> | ||
</VerticalStackLayout> | ||
</DataTemplate> | ||
</CollectionView.ItemTemplate> | ||
</CollectionView> | ||
</ContentPage> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
namespace Maui.Controls.Sample.Issues | ||
{ | ||
[Issue(IssueTracker.Github, 24511, "Spacing in the ItemsLayout of CollectionView stops it from scrolling", PlatformAffected.Android)] | ||
public partial class Issue24511 : ContentPage | ||
{ | ||
public Issue24511() | ||
{ | ||
InitializeComponent(); | ||
CV.ItemsSource = Enumerable.Range(0, 100).Select(x => $"Item{x}").ToList(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
using NUnit.Framework; | ||
using UITest.Appium; | ||
using UITest.Core; | ||
|
||
namespace Microsoft.Maui.TestCases.Tests.Issues | ||
{ | ||
public class Issue24511 : _IssuesUITest | ||
{ | ||
public Issue24511(TestDevice testDevice) : base(testDevice) | ||
{ | ||
} | ||
|
||
public override string Issue => "Spacing in the ItemsLayout of CollectionView stops it from scrolling"; | ||
|
||
[Test] | ||
[Category(UITestCategories.CollectionView)] | ||
public void CollectionViewWithSpacingShouldScroll() | ||
{ | ||
App.WaitForElement("CV"); | ||
App.ScrollDown("CV"); | ||
|
||
VerifyScreenshot(); | ||
mattleibow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} |
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?