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 Lifecycle convergence #7989

Merged
merged 12 commits into from Oct 1, 2022
Merged

Android Lifecycle convergence #7989

merged 12 commits into from Oct 1, 2022

Conversation

RobertFlatt
Copy link
Contributor

@RobertFlatt RobertFlatt commented Aug 30, 2022

Maintainer merge checklist

  • Title is descriptive/clear for inclusion in release notes.
  • Applied a Component: xxx label.
  • Applied the api-deprecation or api-break label.
  • Applied the release-highlight label to be highlighted in release notes.
  • Added to the milestone version it was merged into.
  • Unittests are included in PR.
  • Properly documented, including versionadded, versionchanged as needed.

Replaces previous divergence between Android and Kivy state machines.
Corrects previous behavior where Kivy app stops, but Android does not stop.
Let Kivy app state follow Android state.
It is unclear it "default case" refers to existence of a return statement, or existence of a method.
@RobertFlatt
Copy link
Contributor Author

Executive Summary

On a mobile device (unlike a desktop) the Kivy Lifecycle is an abstraction of a lifecycle implemented in the OS. The Kivy and OS state machines muct remain synchronized to avoid ambigious states.

This PR addresses differences between these state machines on Android, it may be a template for extending these observations to iOS.

Issues

On Android the cases where the Kivy state machine diverges from the Android state machine are:

    1. Back Gesture/Button | App: stop | Android : pause |
    1. on_pause False | App : stop | Android : no change |
    1. app.stop() | App: stop | Android : no change |
    1. app.pause() | App : N/A | Android : N/A |
    1. Android sleep->destroy | App : sleep | Android : destroyed |
    1. Android destroy app | App: service quit | Android : service no change |

In this PR we address issues i thru iv. Issues v and vi are not Kivy issues.

Discussion and proposed changes

  • Cases i-iii) In general the Kivy state machine follows the Android state machine via events from SDL2. However in these three cases Kivy sets its own state without informing Android. The cases are addressed by replacing Kivy state changes with calls to Android telling it to change state. The Android state change will in turn tell Kivy to change state via SDL2.

Both Android api calls used in this PR are available on all current p4a supported Android versions.
https://developer.android.com/reference/android/app/Activity#moveTaskToBack(boolean)
https://developer.android.com/reference/android/app/Activity#finishAndRemoveTask()

  • Case iv) is currently this is not implemented in Kivy, it is implemented in this PR (in the same manner as the first 3 cases) for completeness.

  • Case v) Is not implemented in SDL2 SDL app gets closed in the background? libsdl-org/SDL#3297 . Hence Kivy fails to issue an 'on_stop' - this is documented. There are no other known side effects. If it is an issue, it is an SDL2 issue.

  • Case vi) The maximum lifetime of a Kivy Android service is the lifetime of the Kivy App (that is until the app is killed, not until the app is paused). This behavior is not changed by this PR. However this is not the default behavior of a service in a Java Android app. If it is an issue, it is a p4a issue.

  • Case ii Revisited) The case is addressed in this PR, but I really think the 'on_pause return' feature should be removed. The utility is unclear to me (self.stop() exists and is more intuative) and there is an unfortunate side effect. As designed, if a user omits the return statement in on_pause there will be no effect on desktop execution (on_pause is not called) but on mobile the app will correctly dissappear from the screen and the task list. The connection between a simple coding error and the app behavior is not intuative. This PR logs this event, and corrects an ambiguity in the comment/documentation. If there is interest in removing the functionality I can create a PR for this.

from android import mActivity
mActivity.moveTaskToBack(True)
return True
elif WindowBase.on_keyboard.exit_on_escape:
if key == 27 or all([is_osx, key in [113, 119], modifier == 1024]):
Copy link
Contributor

Choose a reason for hiding this comment

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

just question:
shouldnt we do some key mapper to constants for keys such "27" (android) and 113 like here etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because it is a change that would be mission creep for this PR. This PR uses the existing conventions for the express purpose of being clear about the functional changes.

If this kind of thing is important to you, please submit a PR.

Copy link
Contributor

@NomadDemon NomadDemon Sep 8, 2022

Choose a reason for hiding this comment

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

I was NOT asking to make it in this PR :) but just asked question globally, in future [separate PR], whatever :)

But you are maybe right. I was thinking in same way pygame was handling keys, they were mapped like K_BACKSPACE, K_ESCAPE, K_SLASH etc.

I though it might improve readability of code somehow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I should have been clearer, I get why you would like this.
And, really, you could write that PR.

@misl6 misl6 self-assigned this Sep 19, 2022
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

  • Tested on runtime (on API 31) with all the possible combinations ✅
  • Code LGTM. (we can do something to avoid some minor DRY violations, but we can target it later) ✅
  • Left some minor suggestions 🧐

Thank you!

Regarding:

Case ii Revisited) The case is addressed in this PR, but I really think the 'on_pause return' feature should be removed. The utility is unclear to me (self.stop() exists and is more intuative) and there is an unfortunate side effect. As designed, if a user omits the return statement in on_pause there will be no effect on desktop execution (on_pause is not called) but on mobile the app will correctly dissappear from the screen and the task list. The connection between a simple coding error and the app behavior is not intuative. This PR logs this event, and corrects an ambiguity in the comment/documentation. If there is interest in removing the functionality I can create a PR for this.

I guess we can discuss it on a separate issue, and if we decide to remove that feature, it will need to be deprecated at first, and then removed in 2/3 releases.

kivy/app.py Outdated Show resolved Hide resolved
'''
if platform == 'android':
from android import mActivity
mActivity.moveTaskToBack(True)
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Maybe we can add a log that warns the developer about the non-availability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@misl6 misl6 added the Component: core-app app, clock, config, inspector, logger, resources, modules, clock, base.py label Sep 20, 2022
@misl6 misl6 added this to the 2.2.0 milestone Sep 20, 2022
RobertFlatt and others added 2 commits September 20, 2022 11:42
Co-authored-by: Mirko Galimberti <me@mirkogalimberti.com>
@RobertFlatt
Copy link
Contributor Author

but I really think the 'on_pause return' feature should be removed.

it will need to be deprecated at first, and then removed in 2/3 releases

Faced with inertia I'm less enthusiastic!

Its really about cleaning up some fuzzy design, not a bug. So not something I'd fight hard for.

A quick search finds 3 cases:

kivy-master>grep -R \'on_pause\'
kivy/core/window/window_sdl2.py:            elif not app.dispatch('on_pause'):
kivy/core/window/window_sdl2.py:        elif not app.dispatch('on_pause'):
kivy/support.py:        if app.dispatch('on_pause'):

I could add a Logger.warning message in the return False case. The return True case is a NOP and of course very common, a warning in that case would generate a storm of confusion.

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@misl6 misl6 merged commit fd12906 into kivy:master Oct 1, 2022
@misl6
Copy link
Member

misl6 commented Oct 1, 2022

but I really think the 'on_pause return' feature should be removed.

it will need to be deprecated at first, and then removed in 2/3 releases

Faced with inertia I'm less enthusiastic!

Its really about cleaning up some fuzzy design, not a bug. So not something I'd fight hard for.

A quick search finds 3 cases:

kivy-master>grep -R \'on_pause\'
kivy/core/window/window_sdl2.py:            elif not app.dispatch('on_pause'):
kivy/core/window/window_sdl2.py:        elif not app.dispatch('on_pause'):
kivy/support.py:        if app.dispatch('on_pause'):

I could add a Logger.warning message in the return False case. The return True case is a NOP and of course very common, a warning in that case would generate a storm of confusion.

Let's open an issue to keep it tracked! 😃

@RobertFlatt
Copy link
Contributor Author

For anybody reading this in my future, it turns out issue iv) 'service lifetime' has previously been fixed kivy/python-for-android#2401

@misl6 misl6 mentioned this pull request Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: core-app app, clock, config, inspector, logger, resources, modules, clock, base.py
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants