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

Setup fixes #2731

Merged
merged 18 commits into from
Mar 28, 2018
Merged

Setup fixes #2731

merged 18 commits into from
Mar 28, 2018

Conversation

martijn00
Copy link
Contributor

@martijn00 martijn00 commented Mar 26, 2018

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

⤵️ What is the current behavior?

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

💥 Does this PR introduce a breaking change?

🐛 Recommendations for testing

📝 Links to relevant issues/docs

🤔 Checklist before submitting

@martijn00 martijn00 added this to the 6.0.0 milestone Mar 26, 2018
nmilcoff
nmilcoff previously approved these changes Mar 26, 2018
@@ -87,6 +87,8 @@ protected override void OnCreate(Bundle bundle)
{
base.OnCreate(bundle);
ViewModel?.ViewCreated();

//TODO: Add setup init and startup here
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this comment mean here? Is that needed to enable android apps start without splash screens, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@martijn00 did you need me to add the setup logic, or are you going to add this before we merge this PR?

Copy link
Contributor Author

@martijn00 martijn00 Mar 26, 2018

Choose a reason for hiding this comment

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

@nickrandolph Yeah it's pretty late here, would be good if you have time. Should be a matter of copying some logic in here and it should work.

@nmilcoff yes, exactly.

@nickrandolph
Copy link
Contributor

@martijn00 I did a bit of refactoring of setup logic across some of the platform classes. However, I'm not sure whether we need to add RegisterSetupType to MvxFragmentActivity and MvxTabActivity. We only need to add RegisterSetup to any class that acts as an entry point for a specific platform (eg App class for UWP, AppDelegate for iOS, Application for Android). I guess the difference is that Android can be launched with any Activity, (or Fragment??) - does this mean we should add RegisterSetup to every one of these?

The RegisterSetup is really just a helper to make it easier for developers so that they don't need to register their Setup class. My thoughts are that if a developer is launching their application using anything other than the generic MvxAppCompatApplication or generic MvxAndroidApplication they should either implement the RegisterSetup method themselves, or call the new RegisterSetupType extension method from their platform class.

I've updated this PR by removing all RegisterSetupType methods, other than the extension method. If I've missed a scenario that you think is hard for developers to get started, let me know and I can come up with a solution.

nickrandolph
nickrandolph previously approved these changes Mar 27, 2018
Copy link
Contributor

@nickrandolph nickrandolph left a comment

Choose a reason for hiding this comment

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

See previous comment

Cheesebaron
Cheesebaron previously approved these changes Mar 28, 2018
@martijn00 martijn00 merged commit 49cda94 into MvvmCross:develop Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants