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 Flyout Crash (Windows) #16800

Merged
merged 8 commits into from
Aug 25, 2023
Merged

Fix Flyout Crash (Windows) #16800

merged 8 commits into from
Aug 25, 2023

Conversation

Foda
Copy link
Member

@Foda Foda commented Aug 16, 2023

Description of Change

This PR fixes flyouts on custom controls created in constructors. Most of it was done w/ assistance and guidance from @PureWeen -- hence the additional modifications to OnAdd, etc.

Additionally, some add + remove item tests have been added for Context Menu/Flyout on Windows.

Issues Fixed

Fixes #16663

@Foda Foda added platform/windows 🪟 area-controls-flyout Flyout i/regression This issue described a confirmed regression on a currently supported version labels Aug 16, 2023
@Foda Foda requested a review from a team as a code owner August 16, 2023 21:39
@samhouts samhouts added this to the .NET 8 GA milestone Aug 16, 2023
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Eilon Eilon added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Aug 18, 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.

I think we also need to change all

RemoveLogicalChild(element, index); to RemoveLogicalChild(element)

I don't think we have a guarantee that this index will match up with the index of the logical child since this might have other logical children.

If it's not too much work it would be cool to give

public abstract partial class Layout<T> 

The same treatment.

I'd like to just get rid of everywhere that we're using

	private protected override IList<Element> LogicalChildrenInternalBackingStore
		=> InternalChildren;

src/Controls/src/Core/Layout/Layout.cs Show resolved Hide resolved
@PureWeen
Copy link
Member

/rebase

@Foda Foda requested a review from PureWeen August 24, 2023 16:31
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.

I think Clear needs to be fixed to just iterate over the _children and removes those otherwise it'll remove extra stuff.

image

I'm debating just changing ClearLogicalChildren to be internal or removing it because I think it's always the wrong API to use

@PureWeen PureWeen merged commit d77142a into main Aug 25, 2023
34 checks passed
@PureWeen PureWeen deleted the foda/FlyoutCrash branch August 25, 2023 20:01
rmarinho added a commit that referenced this pull request Aug 29, 2023
* [Android] Fix Keyboard.Numeric issue (#16163)

* Fix Keyboard.Numeric issue on DisplayPromptAsync on Android

* Remove AlertsPage sample

---------

Co-authored-by: Rachel Kang <rachel.j.kang@gmail.com>

* Invalidate shell tab title on Windows (#16593)

Co-authored-by: Rui Marinho <me@ruimarinho.net>

* Fix Flyout Crash (Windows) (#16800)

* Fix flyout crash due to invalid casting of child

* Add tests

* Add additional tests, PR feedback

* Fix missing call to `RemoveLogicalChild`

* Update Clear

---------

Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>

* [core] factory methods for registering services, part 2 (#17004)

b0bba51 was incomplete, in that it missed registrations with 1 type
argument:

    builder.Services.AddScoped<HideSoftInputOnTappedChangedManager>();

So we need to add entries with "`1" for all of the banned methods:

    ++M:Microsoft.Extensions.DependencyInjection.ServiceCollectionServiceExtensions.AddScoped`1(Microsoft.Extensions.DependencyInjection.IServiceCollection);Use a Factory method to register the service instead
    M:Microsoft.Extensions.DependencyInjection.ServiceCollectionServiceExtensions.AddScoped`2(Microsoft.Extensions.DependencyInjection.IServiceCollection);Use a Factory method to register the service instead

We can improve startup time by using a factory method instead:

    builder.Services.AddScoped(_ => new HideSoftInputOnTappedChangedManager());

This also found a couple more calls to fix throughout .NET MAUI.

* [create-pull-request] automated change (#17017)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Ignore failures from newly added UITests temporarily (#17003)

* Ignore failures from WhenQueryingCarouselItemsInViewThenSingleItemIsRetrieved

* Update Ignore to include all platformss

* Center window on WinUI, so it's always fully on screen in CI

* Set agent screen resolution bigger for UI tests

* Update baseline snapshots for bigger screen

* Just set screen resolution on Windows

* Ignore Issue16320 on Android

* Fix Android text alignment in migrated projects (#16986)

* Fix Android text alignment in migrated projects

* Add (manual) device tests

* Add clarifying comment

* [tentative] Move input tests to generic code

* Wrap up tests

* Make masks constants

* Fix title to be consistent with other test names

* Fix the order of logical modifications (#17020)

* Fix the order of logical modifications

* - keep in sync while removing

* - fix clear

---------

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
Co-authored-by: Rachel Kang <rachel.j.kang@gmail.com>
Co-authored-by: Mike Corsaro <corsarom@gmail.com>
Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Bret Johnson <bret.johnson@microsoft.com>
Co-authored-by: Juan Diego Herrera <juherrera@microsoft.com>
Co-authored-by: Shane Neuville <shneuvil@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2023
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
@samhouts samhouts added the fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-flyout Flyout fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 i/regression This issue described a confirmed regression on a currently supported version platform/windows 🪟
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[regression/8.0.0-preview.7.8842] Maui MenuFlyOut Crashes
5 participants