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

Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API #1528

Merged
merged 1 commit into from Jan 26, 2019
Merged

Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API #1528

merged 1 commit into from Jan 26, 2019

Conversation

ghost
Copy link

@ghost ghost commented Dec 13, 2018

Bump to SDL2 to 2.0.9.

State: ready for merge

  • Expand build.py to apply SDLActivity.java patch (required so this pull request will work for others)
  • Add option to enable/disable immersive mode to build.py and AndroidManifest.tmpl.xml, and make it disabled per default to keep old behavior - might conflict with Unify configChanges manifest entry and add missing values #1522 upstream SDL2 bug: https://bugzilla.libsdl.org/show_bug.cgi?id=4424 (makes navigation annoyingly slide away in windowed when it shouldn't, workaround is to set window fullscreen & back to windowed at runtime)
  • Find a new approach to trigger keepActive for PythonActivity.java to fix loading screen not disappearing on its own (can be worked around by user by tabbing in & out of the app until it works)
  • Get in some way of triggering sd card runtime permission dialog, because SDL2 requires target API >= 26 where sd card access isn't otherwise possible, and document it - also see p4a doesn't handle runtime permissions #1183
  • Fix kivy not dealing properly with runtime sd card permission - also see Remove WRITE_EXTERNAL_STORAGE default permission #1081 and Sd card runtime permission dialog support required for p4a SDL2 update kivy#6081
  • Fix kivy not properly obeying fullscreen/windowed mode on launch by making it use env vars
  • Document new API requirement >= 26, and possibly add an instant abort for lower API levels when using the sdl2 bootstrap added clear note in docs that other target API levels than suggested 27 are less likely to work (such that I didn't need to specify an exact minimum that we'll forget to update later)
  • Back button might be broken. Investigate... --> works fine when new SDL_ANDROID_TRAP_BACK_BUTTON hint is set to 1

Example use for new runtime permissions API added by this pull request:

from android.permissions import Permission, request_permission,\
    check_permission
request_permission(Permission.WRITE_EXTERNAL_STORAGE)

Gains:

  • CTRL key finally works properly on physical keyboards
  • Side-by-side works properly (with Unify configChanges manifest entry and add missing values #1522 merged), including contents resizing properly to half the screen
  • All other bugfixes that happened between 2.0.4(?) and 2.0.9
  • API >=23 runtime permission API added to p4a's android module

@ghost
Copy link
Author

ghost commented Dec 23, 2018

Edit: no longer relevant

@ghost ghost changed the title [WIP] Bump SDL2 to 2.0.9 [WIP] Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API Dec 24, 2018
@ghost ghost changed the title [WIP] Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API Jan 2, 2019
@ghost
Copy link
Author

ghost commented Jan 2, 2019

I was so annoyed with the upstream bug that I spent some hours reading & debugging SDL2 code, and finally got to the culprit: https://bugzilla.libsdl.org/show_bug.cgi?id=4424#c8 (Fix is already merged in SDL2 upstream, but not in latest stable yet - so we need to patch this manually and remove that with the next version bump - see code comment at the fixed line)

Apart from kivy needing to be adapted, this is ready for merge! It has some conflict potential, so if you want working CTRL keys, working space bar on hardware keyboards and all the other improvements of a new SDL2 and the runtime permissions API, I recommend to merge this soon.

@OptimusGREEN
Copy link
Contributor

Nice!

I already have my permissions up and running, the only thing I couldn't get around was the onRequestPermissionsResult issue as this can only be done from the activity.

Is this something that will be implemented into PythonActivity with a callback accessible in python? The reason this is required is if someone has ticked the 'don't ask again box or clicked the deny either by accident or deliberately, then it allows the dev to show a dialog or something to explain that the permission is denied.

I'd do it myself if I understood java enough but to be honest it hurts my head hence why Kivy is the man! :)

@ghost
Copy link
Author

ghost commented Jan 5, 2019

@OptimusGREEN I just haven't added it yet because I wanted to get the basics in first 😄 but yes it absolutely makes sense to add it later, and it shouldn't be particularly hard to do. (and you didn't miss anything, right now that information / callback is not accessible)

@OptimusGREEN
Copy link
Contributor

@Jonast Superb, looking forward to it.

@Zen-CODE
Copy link
Member

Zen-CODE commented Jan 7, 2019

@Jonast. Thanks for this 👍 I think this is a going to be a great step for Android support. Once it's merged, I plan to help document the changes and put together a simple example showing how to use the runtime permission API. I think we need to have a run-able example to illustrate exactly this stuff works :-)

@ghost ghost changed the title Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API [WIP] Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API Jan 9, 2019
@ghost
Copy link
Author

ghost commented Jan 9, 2019

Edit: done

@misl6
Copy link
Member

misl6 commented Jan 12, 2019

I've tested it on my real-world app with a huge set of recipes.

I've encountered 2 bugs.

Bug 1)
Every even compilation fails during java build. I suppose that you're patching the SDLActivity.java also if it is already patched.

Bug 2)
During the startup process, app automatically rotates to landscape.

This is my current buildozer setup for orientation:
# (str) Supported orientation (one of landscape, portrait or all)
orientation = portrait

If, during runtime, I'm changing the rotation programmatically, the app set-up to requested orientation without any problem.

@ghost
Copy link
Author

ghost commented Jan 13, 2019

Regarding 1) should be fixed (although if you tried the previous version before I fixed this just now, you might need to delete your distribution folder)

Regarding 2): I tested windowed mode portrait in different configurations (windowed manifest + SDL_Window created windowed, fullscreen manifest + SDL_Window created windowed) and it worked fine, so this needs to be something connected to fullscreen mode. I'll test that next

inclement
inclement previously approved these changes Jan 13, 2019
Copy link
Member

@inclement inclement left a comment

Choose a reason for hiding this comment

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

This looks great and can be merged when you think it's ready. I'm just testing it, although I probably won't do anything as extensive as has already been mentioned.

Edit: I do get the issue with the app automatically rotating to landscape.

@inclement
Copy link
Member

It turns out the issue with the app automatically rotating is because SDL-2.0.9 ignores the app's normal orientation setting and always tries to set a new value based on the contents of the "SDL_IOS_ORIENTATIONS" environment variable (despite the name, this is documented as applying to both Android an iOS). By default it uses "LandscapeLeft LandscapeRight, although I'm not sure where it gets that from.

If the environment variable doesn't have a valid value, SDL2 still sets the orientation to landscape or portrait based on the aspect ratio of the display, so there seems to be no simple way to skip this.

Also, when I did set the env var to have value "" or "Portrait", the window is fullscreen despite using --window (maybe a different and similar SDL hint issue?), and the drawing surface is in the wrong place until I tap the screen.

@ghost
Copy link
Author

ghost commented Jan 13, 2019 via email

@inclement
Copy link
Member

Well, SDL2 wasn't doing this before, right? I guessed this is more just a new hint they introduced is a simpler cross-platform api, its behaviour is okay except that it's mildly annoying to have no 'do nothing' option that will let the app use the AndroidManifest.xml orientation.

Do you know why the app isn't windowed any more when --window is used? Is this the issue you explained above?

@ghost
Copy link
Author

ghost commented Jan 13, 2019 via email

@inclement
Copy link
Member

inclement commented Jan 13, 2019

For reference (as discussed on discord), the problem is that SDL-2.0.9 uses certain SDL2 settings to control the Android window, whereas the older SDL2 ignored them on Android. Kivy passes some of these settings, which causes problems.

The following code should make windowed Kivy apps work, place it before any other imports.

import os
os.environ['SDL_IOS_ORIENTATIONS'] = ""
from kivy.config import Config
Config.set('graphics', 'fullscreen', '0')

Alternatively setting fullscreen to '1' should work too to give a full screen app, if you don't pass the --window argument.

You can also set the KIVY_ORIENTATIONS env var instead of SDL_IOS_ORIENTATIONS.

We'll need to either retire the --window and --orientation options of the SDL2 bootstrap, or make sure these are stored and used when the code runs on the device. In principle it might be best to remove the options and leave it to applications to do what they want when they create the SDL2 window, but that would be a relatively big api change (since these are pretty core options) and also would need documenting.

Alternatively we could add some more code to pass these options into the device.

@ghost
Copy link
Author

ghost commented Jan 13, 2019 via email

@Fak3
Copy link
Contributor

Fak3 commented Jan 14, 2019

Could someone please tell if it will be possible to build app that supports Android 4.4 api level 19 after this is merged?

@ghost
Copy link
Author

ghost commented Jan 14, 2019

@Fak3 I think current minimum API level is actually 21, so I don't think so. This is however unrelated to this pull request, that only changes the target api level's minimum, which doesn't change compatibility with older devices

@ghost ghost changed the title [WIP] Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API Jan 15, 2019
@ghost
Copy link
Author

ghost commented Jan 15, 2019

I added environment options now, P4A_IS_WINDOWED (which can be "True" or "False") and another one being P4A_ORIENTATION (which can be e.g. "user", "portrait", ... just whatever that setting is set to).

They're set in build.py and it shouldn't be too hard to expand to more in the future if we ever should want to add any

@ghost
Copy link
Author

ghost commented Jan 15, 2019

@AndreMiras @inclement I just did a larger docs rework as part of this pull request!

Important changes:

1. Moved Working on Android further up in the overall navigation - I think it's something that is way more central and what a beginner would want to know first than advanced things like custom recipes or bootstraps

2. Added Runtime Permissions to "Working on Android" section - such that people actually know how to use it!

2. Restructured the APIs/"Working on Android" section with the following details:

  1. Reordered in degree of escalation: common examples, general android module use,
    plyer use (since that's still highlevel), then pyjnius use

  2. Removed deprecation notice for android! Since we added new things now
    and @inclement and me concluded that it might be nice to just add commonly
    needed pyjnius wrapping code here (and therefore gradually fade out the Cython
    mess) it might be worth keeping around. This probably needs discussion

  3. Removed example on how to open web browser without android, since it seemed
    really distracting (huge junk of pyjnius code in android module section, an entire
    module focused around having people not need to do that),
    and it seems somewhat unnecessary if we actually keep the android
    module around

@guysoft
Copy link

guysoft commented Jan 16, 2019

ok, I have python for android built with your newsdl branch, marged with master, running on android lollypop.

I run print(request_permission(Permission.WRITE_EXTERNAL_STORAGE)) , and it prints back None

But I still get on os.makedirs PermissionError: [Errno 13] Permission denied: '/storage/external_SD/test_yay'

Folder exists and I can print its contents with os.listdir("/storage/external_SD")

Reproduction code avilable here: https://github.com/guysoft/kivy-external-storage-permission

Reproduction steps:

mkdir reproduce
cd reproduce
git clone https://github.com/guysoft/kivy-external-storage-permission.git
git clone https://github.com/guysoft/python-for-android.git
cd kivy-external-storage-permission
sudo docker-compose up -d 
# Waiting for merge of https://github.com/JonasT/p4a-build-spaces/pull/8
sudo docker exec -it -u 0 buildozer-test pip3 install https://github.com/kivy/buildozer/archive/master.zip
./docker_build
./docker_debug

Note: build might fail to download the Android API, you wiil get the lines

[INFO]:    Will compile for the following archs: armeabi-v7a
[INFO]:    Found Android API target in $ANDROIDAPI: 28
[INFO]:    Available Android APIs are ()
[ERROR]:   Build failed: Requested API target 28 is not available, install it with the SDK android tool.

Just do:

sudo rm -rf ./.buildozer

and run again, might take more than one try.

@inclement
Copy link
Member

I opened kivy/kivy#6107 to fix Kivy to work with these env vars. It works fine except for a back button issue mentioned there, which might have a simple fix.

@ghost
Copy link
Author

ghost commented Jan 20, 2019

For what it's worth, we discussed the issue @guysoft had in chat, and from what I can read in the official android docs this is somewhat expected. (The permission doesn't appear to allow writing anywhere, just to a few common folders like Documents, ... To pick other locations, the graphical dialog API probably needs to be used, but this is really beyond the scope of this pull request) So as far as I'm concerned this is still good & ready for merging!

@guysoft
Copy link

guysoft commented Jan 20, 2019

I can confirm that here.
Path that does work is /storage/external_SD/Android/data/com.your.app.name/files on my lollipop device.
Where external_SD changes in different vendors.
Also it seem that latter Nugat threre was an API added for this but I don't have a device to test.

@ghost ghost changed the title Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API [WIP] Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API Jan 24, 2019
@ghost ghost changed the title [WIP] Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API Bump SDL2 to 2.0.9 & Add API >=23 runtime permissions API Jan 26, 2019
@ghost
Copy link
Author

ghost commented Jan 26, 2019

Tested this with the fixed keyboard app, and I can back out with the return button just fine. Portrait orientation also worked fine. @inclement is there more that needs to be tested to verify it? Otherwise, this is ready for merge as far as I'm concerned

@inclement inclement merged commit e8fcc32 into kivy:master Jan 26, 2019
@inclement
Copy link
Member

Works super, thanks again!

@ghost ghost deleted the newsdl branch January 26, 2019 21:22
@OptimusGREEN
Copy link
Contributor

OptimusGREEN commented Feb 10, 2019

any word on a getPermissionsRequestResult callback for python? That way we could get the result instantly if the "don't ask again" box has previously been ticked?

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

Successfully merging this pull request may close these issues.

7 participants