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

Android: remove dependency on SplashScreen. Fix startup issues. #3376

Closed
wants to merge 12 commits into from
Closed

Android: remove dependency on SplashScreen. Fix startup issues. #3376

wants to merge 12 commits into from

Conversation

softlion
Copy link
Contributor

@softlion softlion commented Apr 13, 2019

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Remove dependency on SplashScreen on Android.
Also remove startup lifecycle from within the base activity.
Also fix all android lifecycle problems.
Also fix static android shortcuts that were not working.
Also fix the usage of AppCenter Distribute, where the first launch could crash the app after coming back from appcenter screen.

⤵️ What is the current behavior?

An Android app must have a SplashScreenActivity derived from MvxSplashScreenAppCompatActivity.
Also all Activities implements the startup code.

🆕 What is the new behavior (if this is a feature change)?

SplashScreenActivity can be of any kind. Especially it can have no layout at all, when its layout is being defined by it's style, to get the fastest possible startup time.

💥 Does this PR introduce a breaking change?

Yes (impact: low): the handling of activity startup is done inside the StartupLifecycleCallback class only, and no more on all the activities that derived from MvxActivity.

Yes (impact: moderate): the app must have an Application class, which results in all static variables and variables linked to the application class behind held in memory, saved and restored. If memory management is not carefully crafted, it will quickly lead to out of memory exceptions.

🐛 Recommendations for testing

Android playground updated

📝 Links to relevant issues/docs

🤔 Checklist before submitting

@softlion softlion requested a review from a team April 13, 2019 16:22
@nickrandolph
Copy link
Contributor

Hey @softlion does this assume now that all Android applications will have an application class? I think the current implementation allows for the scenario where there is no application class, and thus the splash screen initiates the Mvx startup.

@softlion

This comment was marked as abuse.

@nickrandolph
Copy link
Contributor

Sorry I wasn't very clear with my previous comment - I meant that this would mean that all developers would have to explicitly include an application class in their Android project which is a change in behaviour for Mvx. I think this might make things simpler and definitely like the implementation you've added.
@martijn00 any thoughts on the impact of these changes?

@kmiterror kmiterror mentioned this pull request Apr 15, 2019
7 tasks
@martijn00
Copy link
Contributor

martijn00 commented Apr 16, 2019

Wouldn't it be possible to detect the splashscreen based on the [Activity] attribute with MainLauncher = true? Same would go for Intents with Attribute i guess. Also doesn't this do some similair stuff as https://github.com/MvvmCross/MvvmCross/blob/develop/MvvmCross/Platforms/Android/Presenters/MvxAndroidViewPresenter.cs#L76 ?

GitHub
The .NET MVVM framework for cross-platform solutions, including Xamarin.iOS, Xamarin.Android, Windows and Mac. - MvvmCross/MvvmCross

@softlion

This comment was marked as abuse.

@softlion

This comment was marked as abuse.

@softlion

This comment was marked as abuse.

@Cheesebaron Cheesebaron added t/breaking Breaking Change type t/bug Bug type t/maintenance Maintenance type labels Jan 2, 2020
@Cheesebaron
Copy link
Member

This PR is actually pretty cool. I think we should get it into MvvmCross 7.0.0. Sorry for dragging this out @softlion

@softlion

This comment was marked as abuse.

@softlion

This comment was marked as abuse.

@softlion

This comment was marked as abuse.

@DavidMarquezF
Copy link
Contributor

Are there any plans to migrate this to android x and merge? It would help a lot since now we're forced to use an Activity for initialization (in our case, we follow the single activity pattern so we end up needing two activities: the splash and the one where all the navigation happens)

Hackmodford added a commit to Hackmodford/MvvmCross that referenced this pull request Aug 17, 2021
@Hackmodford
Copy link
Contributor

I branched off of develop and hand copied the changes @softlion made. MvvmCross doesn't build in macOS so I don't have a way to test if the changes carried over. Maybe you'd like to take a look at what I did and force push? I'd like to see this effort continue!

https://github.com/Hackmodford/MvvmCross/tree/android-no-splashscreen-copy

@Hackmodford
Copy link
Contributor

Hackmodford commented Aug 20, 2021

@softlion How does this work with no splash screen? I tried modifying the playground to not use a splash screen and I seem to get two instances of the root activity.

Also is the original MvxSplashScreenActivity supposed to still be an option? Because I noticed it crashes if you start the app, then immediately press the back button.

@Cheesebaron
Copy link
Member

@Hackmodford the approach here with having to create Application class is fine. However, coming in MAUI and I've also been thinking about it, we would need to provide an AppHost.

So this means that taking a dependency on Microsoft.Extensions.DependencyInjection and creating our own App host would very much obsolete this work and also obsolete how MvvmCross starts up and registers all its dependencies currently.

So while this code might work, I was thinking about changing everything. Just need to find some time to spend on this.

@softlion

This comment was marked as abuse.

@Hackmodford
Copy link
Contributor

@Cheesebaron Does the AppHost comment apply if we were building a native app instead of a MAUI (Forms) type app?

@Paul-N
Copy link

Paul-N commented Aug 20, 2021

Recently I've exploring some MAUI code and I think we can have similar IStartup approach in Mvx. What we need is dependency of these three libs: MAUI.Core, MAUI.Essentials and Microsoft.Maui.Graphics. If we will add them we can stay closer to modern .NET developing. Btw those libs can be built against 'old' pre-6 Xamarin targets and netStd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/breaking Breaking Change type t/bug Bug type t/maintenance Maintenance type
Development

Successfully merging this pull request may close these issues.

None yet

9 participants