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

[Windows] Fix wrong Layout from hidden CollectionView #17659

Merged
merged 2 commits into from Oct 19, 2023
Merged

Conversation

jsuarezruiz
Copy link
Contributor

@jsuarezruiz jsuarezruiz commented Sep 26, 2023

Description of Change

Fix wrong Layout from hidden CollectionView.
fix-17400

Issues Fixed

Fixes #17400
Fixes #18395

@jsuarezruiz jsuarezruiz added t/bug Something isn't working platform/windows 🪟 area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Sep 26, 2023
}
_formsEmptyView?.Layout(new Rect(0, 0, finalSize.Width, finalSize.Height));

if (_wrapGrid is null)
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 an invisible CollectionView, the _wrapGrid will be null (VisualTreeHelper.GetChildrenCount will return 0) and will not apply the correct sizes.

Copy link
Member

Choose a reason for hiding this comment

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

not sure to what extent this matters, but it looks like you can subscribe to ChoosingItemContainer and call FindItemsWrapGrid from there.

It just feels a bit weird to set the size information of the WrappedGrid inside the ArrangeOverride call.

Thoughts on just calling it from there?

public FormsGridView()
{
	// Using the full style for this control, because for some reason on 16299 we can't set the ControlTemplate
	// (it just fails silently saying it can't find the resource key)
	DefaultStyleKey = typeof(FormsGridView);

	RegisterPropertyChangedCallback(ItemsPanelProperty, ItemsPanelChanged);

	this.ChoosingItemContainer += FormsGridView_ChoosingItemContainer;
}

private void FormsGridView_ChoosingItemContainer(ListViewBase sender, ChoosingItemContainerEventArgs args)
{
	if (_wrapGrid is not null)
		FindItemsWrapGrid();
}

And then even for good measure

protected override global::Windows.Foundation.Size MeasureOverride(global::Windows.Foundation.Size availableSize)
{
	var result =  base.MeasureOverride(availableSize); // it looks like the grid comes into existence here.

	FindItemsWrapGrid();

	return result;
}

I also wonder if we should null at the grid and unsubscribe when the orientation is changed.

The Orientation applies a different template so the wrapgrid is going to change.
But maybe this GridView is recreated when the orientation gets changed?

		public Orientation Orientation
		{
			get => _orientation;
			set
			{
				_orientation = value;
				if (_orientation == Orientation.Horizontal)
				{
					ItemsPanel = (ItemsPanelTemplate)UWPApp.Current.Resources["HorizontalGridItemsPanel"];
					ScrollViewer.SetHorizontalScrollMode(this, WScrollMode.Auto);
					ScrollViewer.SetHorizontalScrollBarVisibility(this, UWPControls.ScrollBarVisibility.Auto);
				}
				else
				{
					ItemsPanel = (ItemsPanelTemplate)UWPApp.Current.Resources["VerticalGridItemsPanel"];
				}
			}
		}

@samhouts samhouts modified the milestones: .NET 8 SR1, .NET 8 GA Oct 5, 2023
Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Currently this PR is targeting main (SR1) with a GA milestone.
Can you please update the Milestone to SR1 or update the targeted branch to net8.0.

@jsuarezruiz
Copy link
Contributor Author

Currently this PR is targeting main (SR1) with a GA milestone. Can you please update the Milestone to SR1 or update the targeted branch to net8.0.

Retarget to net8.0

@samhouts
Copy link
Member

@jsuarezruiz Failing tests?

@samhouts samhouts added the s/pr-needs-author-input PR needs an update from the author label Oct 16, 2023
@dotnet dotnet deleted a comment Oct 17, 2023
@jsuarezruiz jsuarezruiz changed the base branch from net8.0 to main October 17, 2023 15:26
@jsuarezruiz jsuarezruiz changed the base branch from main to net8.0 October 17, 2023 15:40
@jsuarezruiz jsuarezruiz marked this pull request as ready for review October 17, 2023 15:40
@jsuarezruiz jsuarezruiz requested a review from a team as a code owner October 17, 2023 15:40
@jsuarezruiz
Copy link
Contributor Author

@jsuarezruiz Failing tests?

Updated the snapshop (from CI) from the golden test.

@jsuarezruiz jsuarezruiz removed the s/pr-needs-author-input PR needs an update from the author label Oct 17, 2023
}
_formsEmptyView?.Layout(new Rect(0, 0, finalSize.Width, finalSize.Height));

if (_wrapGrid is null)
Copy link
Member

Choose a reason for hiding this comment

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

not sure to what extent this matters, but it looks like you can subscribe to ChoosingItemContainer and call FindItemsWrapGrid from there.

It just feels a bit weird to set the size information of the WrappedGrid inside the ArrangeOverride call.

Thoughts on just calling it from there?

public FormsGridView()
{
	// Using the full style for this control, because for some reason on 16299 we can't set the ControlTemplate
	// (it just fails silently saying it can't find the resource key)
	DefaultStyleKey = typeof(FormsGridView);

	RegisterPropertyChangedCallback(ItemsPanelProperty, ItemsPanelChanged);

	this.ChoosingItemContainer += FormsGridView_ChoosingItemContainer;
}

private void FormsGridView_ChoosingItemContainer(ListViewBase sender, ChoosingItemContainerEventArgs args)
{
	if (_wrapGrid is not null)
		FindItemsWrapGrid();
}

And then even for good measure

protected override global::Windows.Foundation.Size MeasureOverride(global::Windows.Foundation.Size availableSize)
{
	var result =  base.MeasureOverride(availableSize); // it looks like the grid comes into existence here.

	FindItemsWrapGrid();

	return result;
}

I also wonder if we should null at the grid and unsubscribe when the orientation is changed.

The Orientation applies a different template so the wrapgrid is going to change.
But maybe this GridView is recreated when the orientation gets changed?

		public Orientation Orientation
		{
			get => _orientation;
			set
			{
				_orientation = value;
				if (_orientation == Orientation.Horizontal)
				{
					ItemsPanel = (ItemsPanelTemplate)UWPApp.Current.Resources["HorizontalGridItemsPanel"];
					ScrollViewer.SetHorizontalScrollMode(this, WScrollMode.Auto);
					ScrollViewer.SetHorizontalScrollBarVisibility(this, UWPControls.ScrollBarVisibility.Auto);
				}
				else
				{
					ItemsPanel = (ItemsPanelTemplate)UWPApp.Current.Resources["VerticalGridItemsPanel"];
				}
			}
		}

@samhouts samhouts added the s/pr-needs-author-input PR needs an update from the author label Oct 18, 2023
@ghost
Copy link

ghost commented Oct 18, 2023

Hi @jsuarezruiz. We have added the "s/pr-needs-author-input" label to this issue, which indicates that we have an open question/action for you before we can take further action. This PRwill be closed automatically in 14 days if we do not hear back from you by then - please feel free to re-open it if you come back to this PR after that time.

@hartez hartez removed their request for review October 18, 2023 18:23
@PureWeen PureWeen merged commit ea9fc41 into net8.0 Oct 19, 2023
47 checks passed
@PureWeen PureWeen deleted the fix-17400 branch October 19, 2023 14:32
@PureWeen
Copy link
Member

/backport to release/8.0.1xx-rc2.1

@github-actions
Copy link
Contributor

Started backporting to release/8.0.1xx-rc2.1: https://github.com/dotnet/maui/actions/runs/6576174770

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView platform/windows 🪟 s/pr-needs-author-input PR needs an update from the author t/bug Something isn't working
Projects
None yet
5 participants