Skip to content

Add interface to expose out UIView methods - #16868

Merged
rmarinho merged 11 commits into
mainfrom
lifecycle_interface
Aug 24, 2023
Merged

rmarinho merged 11 commits into
mainfrom
lifecycle_interface

Conversation

@PureWeen

@PureWeen PureWeen commented Aug 18, 2023 •

Copy link
Copy Markdown
Member

Description of Change

Currently we're using KO's to monitor changes on iOS UIViews to trigger loaded/unloaded. This is very imprecise but also appears to cause occasional crashes. This PR begins the process of moving us to a more precise mechanism for monitoring when views are added/removed from the platform window.

Issues Fixed

Fixes #16860
Fixes #13245

@PureWeen
PureWeen marked this pull request as ready for review August 21, 2023 14:12
@PureWeen
PureWeen requested a review from a team as a code owner August 21, 2023 14:12
@samhouts samhouts added this to the .NET 8 GA milestone Aug 21, 2023
@PureWeen
PureWeen force-pushed the lifecycle_interface branch from 0acd35e to 701425d Compare August 21, 2023 17:42

@mattleibow mattleibow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems valid and all that. Just had a Q on how the builds works when unloaded and how it can be used to detect unloaded. And I spotted a WillMoveToWindow calling MovedToWindow. Not sure if that should have been 2 events?

@PureWeen
PureWeen requested a review from mattleibow August 22, 2023 15:09
mattleibow
mattleibow previously approved these changes Aug 23, 2023

@rmarinho rmarinho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ExcludedWithChildren device test on iOS failing


// Because we can't inherit from Button we use the container to handle
// Life cycle events and things like monitoring focus changed
public override bool NeedsContainer => true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this a big change wrapping every button now on a UIView, border, shadows etc?!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ish? Containers in theory shouldn't be a big change. They should be mostly unnoticeable from a behavior stand point. This is iOS as well so not as worried about performance

@PureWeen
PureWeen requested a review from rmarinho August 23, 2023 17:06
@rmarinho
rmarinho merged commit 7c035c6 into main Aug 24, 2023
@rmarinho
rmarinho deleted the lifecycle_interface branch August 24, 2023 18:35
@jsuarezruiz jsuarezruiz added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Sep 28, 2023
@PureWeen PureWeen added the backport/NO This change should not be backported. It may break customers. label Nov 6, 2023
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 7, 2023
@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

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-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[regression/8.0.0-preview.7.8842] iOS: Shell navigation crashes the app with SIGSEGV Loaded and Unloaded events are not fired correctly

5 participants