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

[Windows] Update CollectionView changing ItemsLayout #14532

Merged
merged 10 commits into from
Apr 28, 2023
Merged

[Windows] Update CollectionView changing ItemsLayout #14532

merged 10 commits into from
Apr 28, 2023

Conversation

jsuarezruiz
Copy link
Contributor

@jsuarezruiz jsuarezruiz commented Apr 12, 2023 •
edited by samhouts
Loading

Description of Change

Update CollectionView changing ItemsLayout on Windows.

fix-7747

To test the changes, launch the .NET MAUI Gallery on Windows and navigate to the CollectionView samples. Select the added "Adaptive CollectionView" sample. This sample change the CollectionView ItemsLayout property based on the Page Width. Resize the Window to validate that the CollectionView updates the layout.

Issues Fixed

Fixes (but reverted) #7747

@jsuarezruiz jsuarezruiz added t/bug Something isn't working platform/windows 🪟 area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Apr 12, 2023
rmarinho
rmarinho reviewed Apr 12, 2023
View reviewed changes
src/Controls/src/Core/Handlers/Items/ItemsViewHandler.Windows.cs

UpdateItemTemplate();
Copy link
Member

Choose a reason for hiding this comment

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

why is this removed? were is It being called now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the PR changes nowhere, check this comment and let me know if you have any questions.

rmarinho
rmarinho reviewed Apr 12, 2023
View reviewed changes
src/Controls/src/Core/Handlers/Items/ItemsViewHandler.Windows.cs
@@ -32,6 +32,10 @@ public abstract partial class ItemsViewHandler<TItemsView> : ViewHandler<TItemsV
bool _emptyViewDisplayed;
double _previousHorizontalOffset;
double _previousVerticalOffset;
double _previousItemSpacing;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid to recreate and inject the ItemContainerStyle if the spacing not changed. For example, resizing the Window. In this case, without checking the changes will change the ItemContainerStyle once every time the size changes.

jsuarezruiz
jsuarezruiz commented Apr 12, 2023
View reviewed changes
src/Controls/src/Core/Handlers/Items/ItemsViewHandler.Windows.cs
@@ -279,18 +283,40 @@ protected virtual void UpdateEmptyView()

protected virtual void UpdateItemsLayout()
{
ListViewBase.IsSynchronizedWithCurrentItem = false;
if (ListViewBase is FormsGridView gridView)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here can find the main change.

In Xamarin.Forms, what happens is that, based on the ItemsLayout, a native ListView or GridView is used on Windows. This forces to dynamically change the native control on runtime needing to map everything and inject items etc.
In .NET MAUI, the code is very similar but the logic to change controls on the fly is missing. For that reason, it didn't work. I have carried out a test, where I basically changed the platformview, removing and adding it to the parent. While it is possible, I wonder, is it necessary? Resizing the Window performs this very expensive task. Isn't it enough to always use a GridView and change the Layout properties? This is what is proposed with the changes in this PR.

@rmarinho rmarinho enabled auto-merge (squash) April 28, 2023 15:14
@rmarinho rmarinho merged commit d569287 into main Apr 28, 2023
@rmarinho rmarinho deleted the fix-7747 branch April 28, 2023 15:16
@jsuarezruiz jsuarezruiz added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Jun 5, 2023
@jsuarezruiz jsuarezruiz mentioned this pull request Jun 5, 2023
Open
@hartez hartez added the backport/NO This change should not be backported. It may break customers. label Jun 7, 2023
@Foda
Copy link
Member

Foda commented Jun 14, 2023 •
edited
Loading

FYI: this PR broke #14391 and breaks the Scrolled event on Windows...

@drasticactions drasticactions mentioned this pull request Jul 13, 2023
Closed
@PureWeen PureWeen mentioned this pull request Sep 7, 2023
Closed
@samhouts samhouts mentioned this pull request Sep 19, 2023
Closed
@sjorsmiltenburg
Copy link

Can this fix be merged into an update for 7.x? It's a blocking issue for my app.

This was referenced Oct 23, 2023
Closed
Closed
emaf added a commit that referenced this pull request Oct 25, 2023
@emaf
Partially revert "[Windows] Update CollectionView changing ItemsLayout (
6afa516 
#14532)"

This partially reverts commit d569287,
keeping the AdaptiveCollectionView sample.

The reverted change was switching to only use FormsGridView as backing
control for CollectionView what causes issues like
#16234.
emaf added a commit that referenced this pull request Oct 25, 2023
@emaf
Partially revert "[Windows] Update CollectionView changing ItemsLayout (
1382213 
#14532)"

This partially reverts commit d569287,
keeping the AdaptiveCollectionView sample.

The reverted change was switching to only use FormsGridView as backing
control for CollectionView what causes issues like
#16234.
emaf added a commit that referenced this pull request Oct 25, 2023
@emaf
Partially revert "[Windows] Update CollectionView changing ItemsLayout (
7f99b06 
#14532)"

This partially reverts commit d569287,
keeping the AdaptiveCollectionView sample.

The reverted change was switching to only use FormsGridView as backing
control for CollectionView what causes issues like
#16234.
PureWeen pushed a commit that referenced this pull request Oct 26, 2023
@emaf
Partially revert "[Windows] Update CollectionView changing ItemsLayout (
ac996a0 
#14532)" (#18356)

This partially reverts commit d569287,
keeping the AdaptiveCollectionView sample.

The reverted change was switching to only use FormsGridView as backing
control for CollectionView what causes issues like
#16234.
@sjorsmiltenburg
Copy link

sjorsmiltenburg commented Nov 12, 2023 •
edited
Loading

@samhouts
I read above that this merge is partially reverted, but the status of the issue is still 'merged'.
One might conclude that only some changes are reverted and the issue is still resolved.
I want to make clear that this is not the case and the bug of not listening to ItemsLayout changes is now back when moving from 8.0.0-rc.2.9530 -> 8.0.0-rc.29530
Should this issue be re-set to 'open'?
To be clear I would really like to see this issue resolved.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.4.8333 Look for this fix in 8.0.0-preview.4.8333! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mandel-macaque mandel-macaque mandel-macaque approved these changes

@rmarinho rmarinho rmarinho approved these changes

Assignees
No one assigned
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView backport/NO This change should not be backported. It may break customers. backport/suggested The PR author or issue review has suggested that the change should be backported. fixed-in-8.0.0-preview.4.8333 Look for this fix in 8.0.0-preview.4.8333! platform/windows 🪟 reverted t/bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jsuarezruiz @Foda @sjorsmiltenburg @rmarinho @mandel-macaque @hartez @samhouts