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 Android crashing issue #4860

Merged
merged 7 commits into from
Mar 24, 2022
Merged

Fix Android crashing issue #4860

merged 7 commits into from
Mar 24, 2022

Conversation

rachelkang
Copy link
Member

Description of Change

Fixes #4169

Android templates app is crashing when starting up after backing out because MainPage, being set from App and not from CreateWindow, was getting cleared

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)
  • Targets a single property for a single control (or intertwined few properties)
  • Adds the property to the appropriate interface
  • Avoids any changes not essential to the handler property
  • Adds the mapping to the PropertyMapper in the handler
  • Adds the mapping method to the WinUI, Android, iOS, and Standard aspects of the handler
  • Implements the actual property updates (usually in extension methods in the Platform section of Core)
  • Tags ported renderer methods with [PortHandler]
  • Adds an example of the property to the sample project (MainPage)
  • Adds the property to the stub class
  • Implements basic property tests in DeviceTests

Does this PR touch anything that might affect accessibility?

  • Does this PR introduce a new control? (If yes, add an example using SemanticProperties to the SemanticsPage)
  • APIs that modify focusability?
  • APIs that modify any text property on a control?
  • Does this PR modify view nesting or view arrangement in anyway?
  • Is there the smallest possibility that your PR will change accessibility?
  • I'm not sure, please help me

If any of the above checkboxes apply to your PR, then the PR will need to provide testing to demonstrate that accessibility still works.

@rachelkang rachelkang added do-not-merge Don't merge this PR high It doesn't work at all, crashes or has a big impact. platform/android 🤖 area-templates Project templates, Item Templates for Blazor and MAUI labels Feb 23, 2022
@@ -66,6 +63,9 @@ void IApplication.CloseWindow(IWindow window)

internal void RemoveWindow(Window window)
{
if (_pendingMainPage != null)
Copy link
Member

Choose a reason for hiding this comment

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

This will cause the window to never be removed I think (down in line 87).

Copy link
Member

@PureWeen PureWeen Feb 23, 2022

Choose a reason for hiding this comment

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

I feel like that's fine

If you're not overriding CreateWindow then we're keeping around a _pendingMainPage that will always be used.

So, it seems like there's really no point in creating/recreating the xplat window.

It seems like the _pendingMainPage and the window it inhabits should just always exist

Basically this PR is just disabling all XXXXWindow behavior if you set MainPage

Copy link
Member

Choose a reason for hiding this comment

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

This will leave the window in the visual tree though, I'm not sure that's ideal.

Also concerned about any lifecycle related things this might not fire as a result?

Copy link
Member

Choose a reason for hiding this comment

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

Because it still is in the logical visual tree. If you're only using APIs that support a single window then your single window that you get never goes away. It's the same thing as how your _pendingMainPage is reused. There's never a scenario that follows this path where you would want to close a window. These events will only fire if you background the app or if you click the back button. So this path only runs when the user isn't looking at the app. Once they go back to the app then the same window is used.

Also concerned about any lifecycle related things this might not fire as a result?

We don't tie any lifecycle events to the xplat window being removed from the App's logical children

If we're going to remove the window then we should also force the user to recreate MainPage if we're going to retain the same MainPage then it makes sense to retain the same window

Copy link
Member

Choose a reason for hiding this comment

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

I'm not opposed to forcing a new main page in this instance. The right thing to do here is follow what android is doing which is destroying the activity and recreating it.

Copy link
Member

Choose a reason for hiding this comment

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

@Redth that makes Android incongruent with WinUI/iOS though. Because this is a very specific to Android scenario that we are working around so that if someone is 17 pages deep on a NavigationPage it doesn't completely reset their application if they get a phone call or a txt message

Copy link
Member

Choose a reason for hiding this comment

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

@PureWeen let's back up a second here then as maybe there's conflating issues. Just switching to another app (ie, hit home button, or task switcher) shouldn't usually cause the activity to be destroyed. Opening a notification to a text probably shouldn't trigger the activity to be destroyed. If that's happening we need to investigate that, maybe we're missing a configuration change being handled by default.

What I'm advocating for here is that if the activity is actually destroyed we clear out the page because that is just like a window being closed.

Now, the back button I believe is a special case. I think that may cause the activity to be destroyed by default, but I think we might want to consider overriding OnBackPressed and calling MoveTaskToBack(true) or the C# api equivalent.

Copy link
Member

Choose a reason for hiding this comment

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

@Redth

Just switching to another app (ie, hit home button, or task switcher) shouldn't usually cause the activity to be destroyed.

If the device is low end enough I'm pretty sure it does. If you take a really low end device and background your app the activity will get destroyed.

That's what this setting in "Developer options" is for testing

image

For example you can test for this inside android as well

https://developer.android.com/guide/components/activities/testing#recreate

If a device is low on resources, the system might destroy an activity, requiring your app to recreate that activity when the user returns to your app. To simulate these conditions, call recreate():

If a user is using the API

App.MainPage = new MainPage()

There's never a scenario where the intention is to "close" your window because when you're using the above API you're only allowed one window. The code in this PR is basically mimicking how iOS/Windows work. iOS/Windows will never kill it's respective platform window out from under you.

Now, if the user is overriding CreateWindow that's a a different story. Now the user is in charge of the windows creation themselves so they can decide what to do when the OS wants to create a new window. That scenario isn't changed by this PR.

This PR is purely for the case where the user doesn't even know that we added Window to .NET MAUI they just want to use App.MainPage = new MainPage() as if nothing changed.

Copy link
Member

Choose a reason for hiding this comment

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

I still disagree 😆 however I think it's probably safe enough to leave this to be an implementation detail we can revisit later and merge this workaround for now.

@rachelkang rachelkang removed the do-not-merge Don't merge this PR label Feb 23, 2022
@PureWeen PureWeen requested a review from Redth March 3, 2022 21:35
Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

Tests are failing

image

@Redth Redth added this to the 6.0.300-rc.1 milestone Mar 21, 2022
@Redth Redth assigned rmarinho and rachelkang and unassigned rmarinho Mar 21, 2022
@PureWeen PureWeen requested review from rmarinho and PureWeen March 23, 2022 20:23
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@PureWeen PureWeen enabled auto-merge (squash) March 24, 2022 14:05
mattleibow added a commit that referenced this pull request Apr 20, 2022
Only skip the cleanup on the non-MainPage windows. Extension of #4860
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2023
@samhouts samhouts added the fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-templates Project templates, Item Templates for Blazor and MAUI fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! high It doesn't work at all, crashes or has a big impact. platform/android 🤖
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] App crashes when start from background
6 participants