Fix foreground notification being mandatory and more (attempt 2) #1888
Conversation
ghost
mentioned this pull request
ghost
changed the title
|
@AndreMiras you seem to be using to be services a lot, could you possibly test if this pull request breaks any of your service things or works fine? |
ghost
requested a review
from 
There was a problem hiding this comment.
From looking at the code it looks fine to me, but I don't use services much so I don't have a good idea of what could break. @AndreMiras any thoughts?
One issue we've had with Services in the past is from calls to android apis that get deprecated or which are too new for some devices - I don't think it's really because of the services so much as simply that it's one of the few places where we routinely make use of more dynamic android apis. Do you think there's any chance of that kind of issue here? Maybe the CI is already doing a good job here since we test the vital minapi=21 targetapi>=26 cases.
|
Thanks @inclement for reviewing and thank you @Jonast for pinging me. I'll give it a try hopefully over the weekend so you can merge it. |
|
@inclement good point! for what it's worth I'm pretty sure I didn't touch any of the SDK/NDK/Android-facing APIs but only all the p4a handling before, but one can never be safe enough so I wouldn't mind more testing. I tested with Android 8.1 / API level 27 on a real device (the app being built with target api 27 and min-api 21) and with a side service to an SDL2 bootstrap - so I didn't test service_only because I have no app that uses that |
|
I'm getting build errors. See full build.log and buildozer spec attached |
pythonforandroid/bootstraps/service_only/build/templates/Service.tmpl.java
Outdated
Show resolved
Hide resolved
| notification = builder.build(); | ||
| } | ||
| startForeground({{ service_id }}, notification); | ||
| @override |
There was a problem hiding this comment.
Should be @Override with uppercase O it seems
There was a problem hiding this comment.
oh no that's no good 😮 that is why I want to get some of these merged 😱 I must have done something weird rebasing, I'll do a manual comparison to my rebased combined strange branch I actually used for build and make sure I didn't miss out anything important since apparently at least one rebase conflict ended up weird 😬
There was a problem hiding this comment.
Well that probably mean we should introduce service compilation to our CI. But at the same time it's time/resources every time we add something new to it
There was a problem hiding this comment.
Wouldn't it be enough to add service/main.py to an existing test app? I think this particular bug should also have been triggered if there had been a service used in addition rather than a standalone service service_only test app
There was a problem hiding this comment.
Oh, fascinating! This is not actually a rebase error, it seems when using service/main.py (which is what I tested) this indeed isn't rendered out. So that's the reason, I guess it needed a service_only test to show up - I assume that's what you tested with?
There was a problem hiding this comment.
Ah, it's probably the named extra services with the --service parameter. We have three ways to do services, I forgot (service/main.py with no name which is what I tested, --service with named services, and service_only)
There was a problem hiding this comment.
Yes indeed, I'm using the --service service:service/main.py, see from the the logs.
Complete command was:
/usr/bin/python3 -m pythonforandroid.toolchain apk --debug --bootstrap=sdl2 --dist_name etheroll --name Etheroll --version 2019.0624 --package com.github.andremiras.etheroll --android_api 27 --minsdk 21 --ndk-api 21 --private /home/andre/workspace/EtherollApp/.buildozer/android/app --permission INTERNET --permission WRITE_EXTERNAL_STORAGE --presplash /home/andre/workspace/EtherollApp/docs/images/etheroll-logo.png --icon /home/andre/workspace/EtherollApp/docs/images/icon.png --orientation portrait --window --service service:service/main.py --copy-libs --local-recipes /home/andre/workspace/EtherollApp/src/python-for-android/recipes --blacklist /home/andre/workspace/EtherollApp/blacklist.txt --arch armeabi-v7a --color=always --storage-dir="/home/andre/workspace/EtherollApp/.buildozer/android/platform/build" --ndk-api=21
pythonforandroid/bootstraps/common/build/templates/Service.tmpl.java
Outdated
Show resolved
Hide resolved
pythonforandroid/bootstraps/service_only/build/templates/Service.tmpl.java
Outdated
Show resolved
Hide resolved
| def start_service(title=None, description=None, arg=None, | ||
| as_foreground=True): | ||
| from jnius import autoclass | ||
| mActivity = autoclass('org.kivy.android.PythonActivity').mActivity |
There was a problem hiding this comment.
Also already available at module level.
https://github.com/kivy/python-for-android/blob/v2019.07.08/pythonforandroid/recipes/android/src/android/_android.pyx#L164
That probably requires some refactor though 😄
There was a problem hiding this comment.
yeah I've kept the mActivity for now, seems a bit ugly to have that outside the function and rely on that being available
|
@AndreMiras When making many comments, could you use 'start a review' to submit them all at once? When they're made as individual reviews they cause an email notification for every one! |
|
Will try to next time, thanks for the heads up 😄 |
There was a problem hiding this comment.
This is looking good 👏
Do you mind rebasing to upstream/develop before we can merge?
This is just to make sure tests introduced in #1933 are passing
- Adds start_service_not_as_foreground() to avoid always displaying an associated notification without this being configurable at runtime. It only is configurable right now if setting the foreground property via --service, which cannot be done when using the simpler service/main.py entrypoint - Fixes service_only service code template not rendering .foreground correctly since it added an outdated, useless function name override - Fixes notification channel name which is visible to end user hardcoding "python" in its name which is not ideal for end user naming (since the average user might not even know what python is) - Fixes override of doStartForeground() leading to quite some error-prone code duplication
|
@AndreMiras I just rebased, and I actually ran into the same problem as with the SDL2 pull request: |
Adds start_service_not_as_foreground() to avoid always displaying
an associated notification without this being configurable at runtime.
It only is configurable right now if setting the foreground property
via --service, which cannot be done when using the simpler
service/main.py entrypoint
Fixes service_only service code template not rendering .foreground
correctly since it added an outdated, useless function name override
Fixes notification channel name which is visible to end user
hardcoding "python" in its name which is not ideal for end user
naming (since the average user might not even know what python is)
Fixes override of doStartForeground() leading to quite some
error-prone code duplication