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

Fix CollectionView not displaying header or footers #16870

Merged
merged 11 commits into from
Sep 6, 2023

Conversation

jknaudt21
Copy link
Contributor

@jknaudt21 jknaudt21 commented Aug 18, 2023

Problem

The footer and header's of a CollectionView would not display on Windows.

This was happening because they were created as WrapperControl in ViewToHandlerConverter.cs , and when being initialized they didn't have parents (although they should!). This, in turn, prevents them from getting the appropriate MauiContext when calling view.FindMauiContext(). Without the context, they can't render.

The reason why they ended up having no parents was because we make a call to ClearLogicalChildren when updating an ItemsSource. This was made to prevent memory leaks but had the unintended side effect of not rendering the headers and footers. #13530

Solution

We ensure to only clear up the items in the ItemSource of the CollectionView. That way any logical child that should persist is not wiped, and can thereby render correctly.

Before & After

I'm using an empty collection view with a footer and header

Sample code
    <VerticalStackLayout>
        <CollectionView MaximumWidthRequest="300">
            <CollectionView.Header>
                <Button BackgroundColor="AliceBlue" Text="Foo"/>
            </CollectionView.Header>
            <CollectionView.Footer>
                <Button Text="Bar" TextColor="Red" />
            </CollectionView.Footer>
        </CollectionView>
    </VerticalStackLayout>
Before After
(Nothing rendered) image

Issues Fixed

Fixes #14557

Remaining work

  • Write a test

@jknaudt21 jknaudt21 requested a review from a team as a code owner August 18, 2023 22:19
@jknaudt21 jknaudt21 self-assigned this Aug 19, 2023
@Eilon Eilon added the area-controls-collectionview CollectionView, CarouselView, IndicatorView label Aug 19, 2023
@samhouts
Copy link
Member

Paging @jonathanpeppers for memory leaks implications

@samhouts samhouts added this to the .NET 8 GA milestone Aug 21, 2023
@samhouts samhouts removed the request for review from jfversluis August 21, 2023 19:39
hartez
hartez previously approved these changes Aug 22, 2023
@jknaudt21
Copy link
Contributor Author

Got spooked on memory leaks so I bolstered the current memory leak test.
image

hartez
hartez previously approved these changes Aug 22, 2023
@jknaudt21 jknaudt21 requested a review from hartez August 22, 2023 23:42
hartez
hartez previously approved these changes Aug 23, 2023
@rmarinho
Copy link
Member

image

Failing tests on android

@jknaudt21
Copy link
Contributor Author

jknaudt21 commented Aug 23, 2023

Failing tests on android

So this seems to be a new bug. The Android collection view leaks if it has a header or footer :(

Edit: The failing Android tests and leak have been fixed: #17012

@jknaudt21 jknaudt21 dismissed PureWeen’s stale review September 1, 2023 19:22

Requested changes have been made

@PureWeen PureWeen merged commit 3c1d3ba into main Sep 6, 2023
38 checks passed
@PureWeen PureWeen deleted the dev/jd/collectionviewfoot branch September 6, 2023 15:23
tj-devel709 pushed a commit to tj-devel709/maui that referenced this pull request Sep 8, 2023
* Make ViewHandlerConverter use apps MauiContext

Tentative change

* Improve comment

* Fix render issue from logical child perspective

* Add UI test

* Only clear items in the ItemSource

We only cant to remove chlidren that are inside the items.

* Address PR feedback

* Bandaid fix android tests

* Add missing link

* Simplify code

* Fix merge conflicts

* Newline nit
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CollectionView Header & Footer not showing
8 participants