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

Changing ItemsView Layout.Orientation removes the item from ItemsRepeater #9422

Closed
ghost1372 opened this issue Mar 11, 2024 · 6 comments
Closed
Assignees
Labels
area-ItemsRepeater bug Something isn't working fix-released The fix has been in a release (experimental, preview, stable, or servicing). team-Controls Issue for the Controls team
Milestone

Comments

@ghost1372
Copy link
Contributor

ghost1372 commented Mar 11, 2024

Describe the bug

00

<SelectorBar local:SelectorAttach.Orientation="Horizontal"/>

WinUI 3 SelectorBar Source Code:

<Style TargetType="SelectorBar">
    <Setter Property="Padding" Value="{ThemeResource SegmentedPadding}" />
    <Setter Property="Background" Value="{ThemeResource SegmentedBackground}" />
    <Setter Property="BorderBrush" Value="{ThemeResource SegmentedBorderBrush}" />
    <Setter Property="BorderThickness" Value="1" />
    <Setter Property="CornerRadius" Value="{ThemeResource ControlCornerRadius}" />
    <Setter Property="HorizontalAlignment" Value="Left" />
    <Setter Property="VerticalAlignment" Value="Top" />
    <Setter Property="IsTabStop" Value="False" />
    <Setter Property="TabNavigation" Value="Once" />
    <Setter Property="Template">
        <Setter.Value>
            <ControlTemplate TargetType="SelectorBar">
                <Grid
                    Background="{TemplateBinding Background}"
                    BorderBrush="{TemplateBinding BorderBrush}"
                    ColumnDefinitions="Auto"
                    CornerRadius="{TemplateBinding CornerRadius}"
                    RowDefinitions="Auto">
                    <Grid.ChildrenTransitions>
                        <TransitionCollection>
                            <RepositionThemeTransition />
                        </TransitionCollection>
                    </Grid.ChildrenTransitions>

                    <ItemsView
                        x:Name="PART_ItemsView"
                        MaxWidth="{TemplateBinding MaxWidth}"
                        MaxHeight="{TemplateBinding MaxHeight}"
                        Padding="{TemplateBinding Padding}"
                        ItemsSource="{TemplateBinding Items}"
                        TabNavigation="{TemplateBinding TabNavigation}">
                        <ItemsView.Layout>
                            <StackLayout Orientation="Horizontal" />
                        </ItemsView.Layout>
                    </ItemsView>
                </Grid>
            </ControlTemplate>
        </Setter.Value>
    </Setter>
</Style>

Steps to reproduce the bug

1.Create a ItemsView with a StackLayout
2.Try to Change StackLayout Orientation in Runtime
3.Select Different Items
4.Change Orientation again

Expected behavior

No response

Screenshots

No response

NuGet package version

WinUI 3 - Windows App SDK 1.5.0: 1.5.240227000

Windows version

Windows 11 (22H2): Build 22621

Additional context

No response

@ghost1372 ghost1372 added the bug Something isn't working label Mar 11, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the needs-triage Issue needs to be triaged by the area owners label Mar 11, 2024
Copy link

Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one. Thank you!

Closed similar issues:

Note: You can give me feedback by thumbs upping or thumbs downing this comment.

@codendone codendone added area-ItemsRepeater team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Mar 14, 2024
@ranjeshj
Copy link
Contributor

@ghost1372 can you please share your repro project?

@ranjeshj ranjeshj added the needs-author-feedback Asked author to supply more information. label Mar 28, 2024
@ghost1372
Copy link
Contributor Author

Hi @ranjeshj
Please Download sample
1.Run App
2.Select item
3.Change Orientation in runtime
image
4.Select another item
5.Change Orientation Again
6.you can see crash

App5.zip

@microsoft-github-policy-service microsoft-github-policy-service bot added needs-triage Issue needs to be triaged by the area owners and removed needs-author-feedback Asked author to supply more information. labels Mar 29, 2024
@karkarl karkarl self-assigned this Apr 4, 2024
@karkarl
Copy link
Contributor

karkarl commented Apr 16, 2024

Fixed in internal repo: Pull Request 10572937: Changing ItemsView Layout.Orientation removes the item from ItemsRepeater Fix

@karkarl
Copy link
Contributor

karkarl commented Apr 16, 2024

Description of fix:

Repro has created a custom attached property to SelectorBar, where it toggles ItemsView's Layout.Orientation:

ItemsView itemsView = FindChild<ItemsView>(selectorBar, "PART_ItemsView");
itemsView.Layout = new StackLayout { Orientation = Orientation.Horizontal };

This implementation manually sets a new StackLayout with different orientations compared to simply toggling ItemsView.Layout.Orientation = Horizontal. As such, ItemsRepeater treats it as changing its layout completely, and goes through the whole virtualization process in recycling and re-realizing the elements.

There are multiple layers to this issue:

  • ItemsRepeater::OnLayoutChanged goes through and clears each element in the collection. If pinned, it goes to m_pinnedPool when cleared.
  • In this situation, the pinned element is added to m_pinnedPool twice. Once at line 694, and again when manually cleared at 706:
    image
  • When ViewManager goes through m_pinnedPool after layout changed at ViewManager::GetElementFromPinnedElements, the first is cleared properly, but the extra element in m_pinnedPool remains and is never cleared.
  • If one swaps the layout more times, the size in m_pinnedPool actually slowly creeps up.
  • When user goes to select another element, it falsely starts checking in pinnedPool at ViewManager::PrunePinnedElements() since there is an element that shouldn't be there.
  • The previously selected element is no longer pinned, thus the element is mistakenly removed from the Collection.

Fix: adds a check for uniqueness when adding to m_pinnedPool so there will be no duplicates. After layoutChanged, size of m_pinnedPool is reset to 0 as designed.

@karkarl karkarl closed this as completed Apr 16, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the needs-triage Issue needs to be triaged by the area owners label Apr 16, 2024
@ghost1372
Copy link
Contributor Author

Thank you
Can we expect it in v1.5.x?
Or we should wait for 1.6?

@microsoft-github-policy-service microsoft-github-policy-service bot added the needs-triage Issue needs to be triaged by the area owners label Apr 16, 2024
@codendone codendone added this to the WinAppSDK 1.6 milestone Apr 17, 2024
@codendone codendone removed the needs-triage Issue needs to be triaged by the area owners label Apr 17, 2024
@codendone codendone added the fix-released The fix has been in a release (experimental, preview, stable, or servicing). label Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ItemsRepeater bug Something isn't working fix-released The fix has been in a release (experimental, preview, stable, or servicing). team-Controls Issue for the Controls team
Projects
None yet
Development

No branches or pull requests

4 participants