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

Typed hint on AppStart and documentation #2901

Merged
merged 6 commits into from
May 24, 2018

Conversation

martijn00
Copy link
Contributor

@martijn00 martijn00 commented May 23, 2018

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

Adds possibility to use a typed hint at appstart.

⤵️ What is the current behavior?

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

💥 Does this PR introduce a breaking change?

Yes, but tried to keep it minimal.

🐛 Recommendations for testing

📝 Links to relevant issues/docs

Fixes #2854
Fixes #2900

🤔 Checklist before submitting

@martijn00 martijn00 added this to the 6.1.0 milestone May 23, 2018
@martijn00 martijn00 changed the title Appstart hints Typed hint on AppStart and documentation May 23, 2018
MvxLog.Instance.Trace("Hint ignored in default MvxAppStart");
}
//There is no way to pass the hint back
var typedHint = typedApplication.Startup(parameter);

Choose a reason for hiding this comment

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

What's the point of doing this since we do nothing with the result? Should it not be above the base call and pass the hint to the base.ApplicationStartup instead?

@martijn00 martijn00 mentioned this pull request May 23, 2018
4 tasks
@@ -12,8 +12,13 @@ public interface IMvxApplication : IMvxViewModelLocatorCollection

void Initialize();

void Startup(object hint);
object Startup(object hint);
Copy link
Contributor

Choose a reason for hiding this comment

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

@martijn00 I'm not sure why we're keeping the object parameter and return type since this can be done by using MvxApplication with THint set to object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to keep this to not break to many things. But it doesn't make a lot of sense. Developers could even just put object as type on the typed Startup.

{
MvxLog.Instance.Trace("Hint ignored in default MvxAppStart");
//There is no way to pass the hint back
return typedApplication.Startup(parameter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this issue, perhaps we should consider passing around a new type, IStartupHint (and associated IStartupHint) that has a property Hint. Rather than passing back an updated hint, the Hint within the IStartupHint object could be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a leftover comment, not an issue anymore since breaking the interface method.

@martijn00 martijn00 merged commit feb8fc6 into MvvmCross:release/6.1.0 May 24, 2018
@martijn00 martijn00 deleted the appstart-hints branch March 18, 2019 14:48
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