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

Enhance mobile suspend MainLoop notifications #85100

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

ztc0611
Copy link
Contributor

@ztc0611 ztc0611 commented Nov 19, 2023

Implements godotengine/godot-proposals#8453

iOS:

Adds NOTIFICATION_APPLICATION_RESUMED and NOTIFICATION_APPLICATION_PAUSED on iOS, as well as making the application being switched from also fire NOTIFICATION_APPLICATION_FOCUS_OUT and NOTIFICATION_APPLICATION_FOCUS_IN upon returning.

Android:

Makes the application being switched from fire NOTIFICATION_APPLICATION_FOCUS_OUT and NOTIFICATION_APPLICATION_FOCUS_IN upon returning.

Be mindful of godotengine/godot-proposals#8453 (comment), there may be complications with DeX mode that need to be ironed out.

doc/classes/MainLoop.xml Outdated Show resolved Hide resolved
doc/classes/MainLoop.xml Outdated Show resolved Hide resolved
@AThousandShips AThousandShips changed the title Add iOS APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop Notifications [iOS] Add APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop Notifications Nov 19, 2023
@akien-mga
Copy link
Member

It would be better to have the Android and iOS implementations in the same PR/commit, so that we don't risk introducing significant platform support differences. If you could only implement one, it's fine to have a PR for it, but since you did both it makes sense to land that change all at once.

@ztc0611
Copy link
Contributor Author

ztc0611 commented Nov 20, 2023

@Calinou thoughts?

I don't feel strongly one way or the other here, I just want to make sure everyone is on the same page to avoid future confusion.

@Calinou
Copy link
Member

Calinou commented Nov 20, 2023

I'd follow Akien's judgement here, so that it's easier to compare the Android and iOS code added in the same PR.

@ztc0611 ztc0611 requested a review from a team as a code owner November 20, 2023 18:17
@AThousandShips AThousandShips changed the title [iOS] Add APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop Notifications [Android, iOS] Add APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop Notifications Nov 20, 2023
@ztc0611 ztc0611 force-pushed the fix-ios-focus-mainloop-notifs branch 2 times, most recently from dd24170 to ac5cdf1 Compare November 20, 2023 18:23
@ztc0611
Copy link
Contributor Author

ztc0611 commented Nov 20, 2023

Merged

@AThousandShips AThousandShips changed the title [Android, iOS] Add APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop Notifications [Mobile] Add APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop Notifications Nov 20, 2023
@akien-mga akien-mga changed the title [Mobile] Add APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop Notifications [Mobile] Add APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop Notifications Nov 20, 2023
@akien-mga akien-mga changed the title [Mobile] Add APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop Notifications [Mobile] Add APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop notifications Nov 20, 2023
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

The Android section looks good.

@AThousandShips
Copy link
Member

Please squash your commits into one, see here

@ztc0611 ztc0611 force-pushed the fix-ios-focus-mainloop-notifs branch from 2622b46 to ec5ed18 Compare November 22, 2023 16:58
@ztc0611
Copy link
Contributor Author

ztc0611 commented Nov 22, 2023

Done 🙂

@AlexBowring
Copy link

Any update on this? I believe this feature is exactly what i'm looking for in my own game, so would be great to see this merged

@YuriSizov YuriSizov changed the title [Mobile] Add APPLICATION_PAUSE and APPLICATION_RESUMED MainLoop notifications Enhance mobile suspend MainLoop notifications Dec 16, 2023
@YuriSizov
Copy link
Contributor

Any update on this?

The iOS part still needs to be reviewed and we don't have a dedicated iOS maintainer, so it takes some time.

@YuriSizov YuriSizov requested a review from bruvzg December 16, 2023 16:10
@ztc0611
Copy link
Contributor Author

ztc0611 commented Dec 16, 2023

Came back from that notification and forgot I should have included the rationale for this.

[b]Note:[/b] On iOS, you only have approximately 5 seconds to finish a task started by this signal. If you go over this allotment, iOS will kill the app instead of pausing it.

The source for that claim added to the docs is this, there might be a better way to word it but that might make review a bit easier. I think this is relevant information because it would probably be hard for someone deep into a Godot project to think to check Apple's docs for applicationDidEnterBackground().

@ztc0611 ztc0611 force-pushed the fix-ios-focus-mainloop-notifs branch from ec5ed18 to fc7a63c Compare February 5, 2024 02:12
@akien-mga akien-mga merged commit 3a2fb42 into godotengine:master Feb 14, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants