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

android: Forward suspended() and resumed() events and patch up platform-specific documentation #3786

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Jul 10, 2024

Addresses #3779

Key them off of onStop() and onStart() which seems to match the other backends most closely. These Android Activity lifecycle events denote when the application is visible on-screen, and recommend that any heavy lifting for startup and shutdown happens here, as the application may be demoted to the background and later shut down entirely unless the user navigates back to it.

EDIT: Also addresses some of the late review comments that were posted on #3765.

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@MarijnS95 MarijnS95 added DS - android C - waiting on author Waiting for a response or another PR S - platform parity Unintended platform differences labels Jul 10, 2024
@MarijnS95 MarijnS95 added C - needs discussion Direction must be ironed out C - nominated Nominated for discussion in the next meeting and removed C - waiting on author Waiting for a response or another PR labels Aug 11, 2024
@daxpedda daxpedda removed the C - nominated Nominated for discussion in the next meeting label Aug 23, 2024
@MarijnS95 MarijnS95 removed the C - needs discussion Direction must be ironed out label Aug 23, 2024
@MarijnS95 MarijnS95 force-pushed the marijn/android/suspend-resume branch from d30b5fb to 8eaaf98 Compare September 2, 2024 08:28
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Nice, feel free to merge once you think it's ready!

@MarijnS95 MarijnS95 marked this pull request as ready for review September 2, 2024 09:36
@MarijnS95 MarijnS95 force-pushed the marijn/android/suspend-resume branch from 8eaaf98 to 41c9c71 Compare September 2, 2024 09:41
@MarijnS95 MarijnS95 requested a review from daxpedda September 2, 2024 09:42
@MarijnS95 MarijnS95 force-pushed the marijn/android/suspend-resume branch from 41c9c71 to 0a2a176 Compare September 11, 2024 08:27
@daxpedda
Copy link
Member

daxpedda commented Oct 8, 2024

(I changed OP to not fix #3779, it just addresses it)

@MarijnS95 MarijnS95 force-pushed the marijn/android/suspend-resume branch from 0a2a176 to b9bcfd7 Compare October 8, 2024 09:08
@MarijnS95
Copy link
Member Author

MarijnS95 commented Oct 8, 2024

(I changed OP to not fix #3779, it just addresses it)

I thought this was fixing the issue entirely but it seems there's still more to address for Web/iOS, and maybe together with more Android changes like #3897?


@daxpedda let me know if you can review this, it should address all except one (duplicated) comment from #3765 (review).

@daxpedda
Copy link
Member

daxpedda commented Oct 8, 2024

@daxpedda let me know if you can review this, it should address all except one (duplicated) comment from #3765 (review).

I took the whole day today to do some catch up on my open source responsibilities, so if I don't manage to do it today feel free to go ahead.

@MarijnS95 MarijnS95 force-pushed the marijn/android/suspend-resume branch from b9bcfd7 to 9a488fb Compare October 8, 2024 11:32
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Great stuff!

src/application.rs Outdated Show resolved Hide resolved
src/application.rs Show resolved Hide resolved
src/application.rs Outdated Show resolved Hide resolved
src/application.rs Outdated Show resolved Hide resolved
src/application.rs Outdated Show resolved Hide resolved
src/application.rs Outdated Show resolved Hide resolved
…atform-specific documentation

Key them off of `onStop()` and `onStart()` which seems to match the
other backends most closely.  These [Android Activity lifecycle] events
denote when the application is visible on-screen, and recommend that any
heavy lifting for startup and shutdown happens here, as the application
may be demoted to the background and later shut down entirely unless the
user navigates back to it.

[Android Activity lifecycle]: https://developer.android.com/guide/components/activities/activity-lifecycle
@MarijnS95 MarijnS95 requested a review from daxpedda October 14, 2024 12:16
@MarijnS95 MarijnS95 force-pushed the marijn/android/suspend-resume branch from 9a488fb to c44ddc4 Compare October 14, 2024 12:16
@MarijnS95
Copy link
Member Author

Thanks for all the reviews!

@MarijnS95 MarijnS95 merged commit 7d77ccf into master Oct 16, 2024
58 checks passed
@MarijnS95 MarijnS95 deleted the marijn/android/suspend-resume branch October 16, 2024 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DS - android S - platform parity Unintended platform differences
Development

Successfully merging this pull request may close these issues.

3 participants