Fix Forms implementation for MvvmCross #2035
Conversation
There was a problem hiding this comment.
This looks great. However, I'm worried about the ctor MvxIosSetup(IMvxApplicationDelegate applicationDelegate, IMvxIosViewPresenter presenter) that is gone now, because it's necessary to start the app for background fetch and other background services what happen without a Window
| { | ||
| _presenter = presenter; | ||
| _window = window; | ||
| _applicationDelegate = applicationDelegate; |
There was a problem hiding this comment.
_applicationDelegate and _window are already set in the call for : this (applicationDelegate, window), no need to assigning them again here
|
|
||
| public Application MvxFormsApp | ||
| private MvxFormsApplication _formsApplication; | ||
| public MvxFormsApplication FormsApplication |
There was a problem hiding this comment.
This rename is actually a breaking change, isn't it?
There was a problem hiding this comment.
Yes. But it is necessary because otherwise we can't extend this with MvvmCross methods.
| @@ -14,8 +14,8 @@ public Setup(MvxApplicationDelegate applicationDelegate, UIWindow window) | |||
| { | |||
| } | |||
|
|
|||
| public Setup(MvxApplicationDelegate applicationDelegate, IMvxIosViewPresenter presenter) | |||
| : base(applicationDelegate, presenter) | |||
| public Setup(MvxApplicationDelegate applicationDelegate, UIWindow window, IMvxIosViewPresenter presenter) | |||
There was a problem hiding this comment.
If I'm not missing anything, then this is a big breaking change. The constructor
public Setup(MvxApplicationDelegate applicationDelegate, IMvxIosViewPresenter presenter) doesn't exist anymore on MvxIosSetup. Do we need this change?
There was a problem hiding this comment.
The problem with this was that in this case Window was never set, so it was never working. In any case, i've never used this ctor, but always the other one.
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Makes Forms easy to use with MvvmCross.
Hacks and stuff not working
🆕 What is the new behavior (if this is a feature change)?
Easy to get started with a Forms app
💥 Does this PR introduce a breaking change?
No, apps keep working, but are recommended to update.
🐛 Recommendations for testing
📝 Links to relevant issues/docs
🤔 Checklist before submitting