Use EnteredBackground to trigger suspension logic #3101
Conversation
|
|
Cheesebaron
changed the title
|
Can you please rebase on develop or merge develop into your branch. |
|
@Cheesebaron I just merged from develop, I hope I did it correctly. I checked out develop in my repo and merged from upstream/develop (upstream is this repo) after fetching upstream. Then I checked out the bugfix in my repo and merged from develop. I pushed the resulting commits. |
| @@ -118,28 +117,18 @@ protected virtual void OnNavigationFailed(object sender, NavigationFailedEventAr | |||
| throw new Exception("Failed to load Page " + e.SourcePageType.FullName); | |||
| } | |||
|
|
|||
| private async void OnSuspending(object sender, SuspendingEventArgs e) | |||
| private async void OnEnteredBackground(object sender, object e) | |||
There was a problem hiding this comment.
This isn't sufficient - the point of deferrals in the OnSuspending is that if there is substantial amount of work to be done the app gets a chance to finish it before the app is suspended. I agree that we should move the Suspend logic to entering background but we need to make sure that if the app gets suspended whilst the Suspend logic is being run, we capture the deferral and only release it once the Suspend logic is complete.
Also, we can't just ignore the Resume logic - what happens if the app does in fact get suspended. As we just going to ignore resuming the application?
There was a problem hiding this comment.
@nickrandolph Thanks for the feedback. I didn't realize you could get the deferral off the EnteredBackgroundEventArgs. Silly of me to leave it out. I have wrapped the suspension logic in the deferral calls. Please let me know if you still see a problem there.
As for the Resuming handler, do we actually have to resume the suspension in MvvmCross? Per what @MartinZikmund wrote in the original issue:
This means when the user switches back to the minimized app, it first restores the state. But that is not necessary at all, because the full app state is still in memory and there is no additional work required. Basically, suspension can be seen as a precautionary measure in case the system runs out of resources and needs to kill the app to free some up.
When the system kills a suspended app, when the user launches it or switches back to it, it will now go through the full launch lifetime
Calling "Resume(suspension)" when the app was suspended but not terminated is what causes the original issue. As Martin mentioned, in the case of termination, we could call resume from OnLaunch/OnActivated. His opinion on this was:
the better solution may just be to leave this up to the developer to decide if she wants to restore the state saved in suspension or not. This would give better flexibility to the developers using MvvmCross and would also preserve the current behavior which is that state is not restored.
At the moment, I'm not sure which I prefer. But what I have changed will allow the app to be suspended and resumed as long as it is not terminated by windows. If it is, it will not resume from the suspension point, which is currently does not since it is broken. Developers could handle the relevant events (Launch/Activated) themselves if they want to resume from suspension after termination.
|
My view is that we should handle the resume from background, even if we don't do anything (since app is already in memory). I would suggest we handle the resume, grab the deferral (....actually there might not be one for resume) and call an overridable protected method to "handle" the resume. The default handling would be to do nothing but at least developers know that if they do want to handle it, they just need to override the Mvx method |
|
@nickrandolph I think that is fine. Resume does not have deferral as far as I remember. The only thing is that we should not restore from suspension manager inside the resume, but otherwise it is a good thing to achieve consistency of providing all lifetime methods. |
|
@nickrandolph @MartinZikmund what you guys have said makes sense to me. I think my latest commit complies with your reasoning. The handler is back but it does not restore by default. |
|
Great but thinking about it, should we use the opposite method from OnEnteringBackground instead of Resuming? |
|
Good question... Docs talk about the suspension only, so it is hard to say. In any case, if the user wants he can add additional handler for |
| { | ||
| var deferral = e.SuspendingOperation.GetDeferral(); | ||
| var deferral = e.GetDeferral(); | ||
|
|
||
| var suspension = Mvx.IoCProvider.GetSingleton<IMvxSuspensionManager>(); | ||
| await Suspend(suspension); |
There was a problem hiding this comment.
Before we accept this, we should tidy up the methods a bit. We're no longer calling "Suspend" from within the OnSuspending - this could be a breaking change for some developers.
I suggest we keep the OnSuspending handler so that we can grab the deferral and call Suspend method.
Have an additional OnEnteredBackground where we grab the deferral invoke an EnteredBackground method.
I also don't think we should be calling SaveAsync in the OnEnteredBackground given that we don't call RestoreAsync - this feels like we're breaking this pattern
|
@mmangold7 can you make the last requested changes so we can merge this? |
|
Any news here? |
|
Can someone in @MvvmCross/core please grab this and address the review comments? |
…Resuming handler to fix suspension/resume crash bug
… in EnteredBackground handler
|
@mmangold7 / @Cheesebaron / @martijn00 take a look at how I've refactored the suspend/resume methods and let me know what you think. I've aimed to make it easy for developers to hook into enter/leave as well as suspend/resume events. The only anomaly is that we save session on entering background, as well as suspending but we do not load session information by default. It's easy enough for developers to now add their own logic but as was pointed out in the comments, in most cases the application stays memory resident so there's no work required to resume the state of the app. |
…Resuming handler to fix suspension/resume crash bug
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Bug fix
app crashes after resuming on UWP (such as from lock screen)
🆕 What is the new behavior (if this is a feature change)?
app now longer crashes after resuming
💥 Does this PR introduce a breaking change?
no
🐛 Recommendations for testing
run a UWP project based on MvvmCross. lock the device. log in to the device. app should no longer crash upon resume/login.
📝 Links to relevant issues/docs
Fixes #3090
🤔 Checklist before submitting