Fixes for Forms presenter #2321
Conversation
| { | ||
| try | ||
| { | ||
| CurrentFragmentManager.PopBackStackImmediate(); |
There was a problem hiding this comment.
Can we run this code only if there are entries on the backstack instead of running it inside of a try/catch block?
There was a problem hiding this comment.
This is too prevent AppCompat and normal fragment type exceptions.
There was a problem hiding this comment.
Alright, I think that makes sense as long as we print the exception. Does it make sense to call base in this context, when base uses a different fragment manager @martijn00?
Sounds like in all cases two exceptions will be written in the output because of that.
There was a problem hiding this comment.
That is exactly why i call base, because base has a different manager which might be the correct type.
There was a problem hiding this comment.
Sorry for insisting but... When using the AppCompatViewPresenter there's no way to show a non-compat Fragment, ShowFragment does not call base in any case.
I think there are no scenarios where the base FragmentManager could resolve the request
There was a problem hiding this comment.
Hmm yeah you are right with that. I would still leave the try catch but remove the base call.
| catch (Exception ex) | ||
| { | ||
| return base.CloseFragments(); | ||
| MvxTrace.Trace("Cannot close any fragments", ex); |
There was a problem hiding this comment.
MvxTrace is obsolete now, should we use the new logging system here?
There was a problem hiding this comment.
It is used all over the code base. I think we need to see how we can replace it all.
|
|
||
| public virtual bool ClosePlatformViews() | ||
| { | ||
| return false; |
There was a problem hiding this comment.
If the feature isn't implemented here can we leave a trace to make it easier to discover?
| /// Also, it exposes a method to be launched when this root page is activated due to navigation popping in Detail | ||
| /// | ||
| /// </summary> | ||
| internal interface IMvxMasterDetailViewModel : IMvxViewModel |
There was a problem hiding this comment.
Isn't this a breaking change? Same with MvxMasterDetailViewModel
There was a problem hiding this comment.
This already was removed in 5.3 but i forgot to remove the viewmodels.
| { | ||
| try | ||
| { | ||
| CurrentFragmentManager.PopBackStackImmediate(); |
There was a problem hiding this comment.
Same here about running the code without try/catch
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Bug fixes
🆕 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
Fixes #2292
Fixes #2307
Fixes #2306
🤔 Checklist before submitting