Change OnApplicationQuit to OnDestroy in EOSManager.cs

[EllieTellie]

Hi,

I would like to propose changing OnApplicationQuit to OnDestroy in EOSManager. Mainly because OnApplicationQuit can be called and the application can keep running for example if you override https://docs.unity3d.com/ScriptReference/Application-wantsToQuit.html

This would be similar to how SteamManager performs shutting down the native side.

You can test how this works by preventing application shutdown with:
Application.wantsToQuit += () => { return false; };
and put a MonoBehaviour with:
OnApplicationQuit() Debug.Log("Quit Attempted")
Then press the X on the window to attempt to quit.

I do know there is a boolean where I can stop this behaviour from happening, but it might be better for this to be the default.

[WispyMouse]

That's really interesting, thank you for sharing! I didn't know about this facet of Unity. I'll talk about the implementation and get this in to our internal task tracking software.

There is a boolean that stops it from attempting to shutdown. The EOSManager prefab should have a variable named ShouldShutdownOnApplicationQuit, defaulted to true. If you set this to false, the Manager will not attempt to shutdown on application closing.

Part of the reason not to do OnDestroy was because it should only ever shutdown once, during the lifetime of the application. If a user doesn't mark the EOSManager as DontDestroyOnLoad, then it might be destroyed as the user transitions between scenes. But as you have demonstrated here, this still isn't a fool-proof solution. Looking further at Application (https://docs.unity3d.com/ScriptReference/Application-quitting.html) we should be able to set Application.quitting to definitely know that the application is shutting down.

[WispyMouse]

#841 is a branch with this work in it. Working on getting it vetted through our pull review process and such.

[EllieTellie]

@WispyMouse that's a good observation I forgot that the EOSManager creation isn't handled internally so could not have DontDestroyOnLoad.

The commit looks good, but you may want to change the documentation of ShouldShutdownOnApplicationQuit boolean to match as it still references OnApplicationQuit there.

[EllieTellie]

https://docs.unity3d.com/ScriptReference/Application-quitting.html also has some weird caveats similar to https://docs.unity3d.com/ScriptReference/MonoBehaviour.OnApplicationQuit.html but slightly different. It depends if the native side gets disposed regardless when the app quits in background on Android for example.

I guess all this could be fixed by making EOSManager create itself and enforce DontDestroyOnLoad, but I guess that's quite a breaking change as people are already handling that themselves.
GameObject managerObject = new GameObject("EOSManager", typeof(EOSManager)); DontDestroyOnLoad(managerObject);

Unity is fun, but thanks for even considering looking at it. I have a fork so I am able to change my copy to OnDestroy so if this doesn't pass review no problems. I suppose I could also call it manually.

[WispyMouse]

Oh wow, thank you for your sharp eyes there! I'll ammend the pull request to include that comment change.

I have a tracked task in our back-end task management for considering DontDestroyOnLoad for the manager. It's probably fine to do, but we'll discuss thoroughly.

Part of the pre-requisite for me merging in my PR is to exhaustively test every platform and development target we support. Going to be a busy day today with builds! Thank you for the warning about Android, I'll keep an eye on that for sure