This way, the user always knows that KDE Connect is running (and what devices are connected). This is a requirement in Android Oreo.
Implements task T6218.
Details
Details
Diff Detail
Diff Detail
- Repository
- R225 KDE Connect - Android application
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
And here's how it looks:
Because of the set priority (minimum), it appears at the bottom of the panel and is hidden when the notification panel is closed.
Comment Actions
FYI there is already some similar work here: https://cgit.kde.org/kdeconnect-android.git/log/?h=update/android-o
I don't say either one is better, just leaving this here for some inspiration. I can't say much about Oreo-related stuff because I don't have a Oreo phone (yet)
Comment Actions
Ah, good to know. The update/android-o branch seems to contain more general work, although its notification is more basic compared to the one in this diff.
Is there any reason why the branch is not merged yet? And should I rebase this diff to the update/android-o branch?
Comment Actions
The only OTHER (edit) commit I see that is not in the master is the one with the Appcompatactivity. Since it does not affect your patch there is no rebasing necessary. I don't know why it's not merged, I would guess Albert has no Oreo phone to test it.
Comment Actions
The latest commit I see (https://cgit.kde.org/kdeconnect-android.git/commit/?h=update/android-o&id=28fc8708b90b43af7c1ada0178c0c82c7cad8c41) also adds a foreground notification tough, which is why I asked.
I don't have Android Oreo either (and cannot connect to anything using the emulator), but this change works for Android 7.1 Nougat.
| src/org/kde/kdeconnect/BackgroundService.java | ||
|---|---|---|
| 302 | Why set a explicit color? I would go with the default and remove the line | |
| 322 | Also I expect a method called createForegroundNotification to do nothing but creating the notification. This has nothing to do with creating but actually using the the notification, thus is a side-effect which should be avoided. According to the docs this must be called after the service is started. Not sure if this is the case here (especially when createForegroundNotification is called from oncreate(). Maybe it should be moved to onStartCommand() | |
Comment Actions
Changed createForegroundNotification to only create the notification, and use it at the call sites.
Also moved the startForeground call to onStartCommand.
| src/org/kde/kdeconnect/BackgroundService.java | ||
|---|---|---|
| 302 | This way the notification color ties in with the app color (orange), see https://developer.android.com/reference/android/app/Notification.html#color. More apps do this (and personally, I like it 😉). If you disagree, I can change it. | |
Comment Actions
I've been comparing your changes to those in my branch update/android-o. The main difference is that I'm using startForegroundService instead of startForeground, as the docs recommend in the Android O migration guide.
Have you tested it works on Android O with startForeground? If it does, it's not what they have documented... However, I would prefer not having to use startForegroundService because it depends on the version 26 of the support library, which forces us to raise the minSdkVersion to Android 4.0. I know most people don't use Android<4 anymore, but I personally would like to keep compatibility for as long as possible to support those who decide to stick with their old phones instead of buying a new one.
Any ideas? I would like to add support for Android O, but I don't know if I should go with my solution (recommended by Android, but drops support for old APIs) or yours (backwards compatible, but I'm not sure if it will cause some problems on Android O).
Comment Actions
I think startForegroundService is only a promise that you'll call startForeground from the service. So startForeground is needed in any case. Without it, it might not even start the service (because of background limitations).
Regarding the support library problem: I think you can decide to call startService or startForegroundService manually, depending on the API version, even without the newer support library, so it should work on all android versions. I cannot actually test this as I don't have an Android Oreo device.
Comment Actions
I've been running the version you gave me that keeps the notification on all the time. I see 2 problems:
- The notification is annoying. It's more Android saying KDE Connect is doing dark stuff than KDE Connect stating it's alive.
- I'm pretty sure the phone doesn't do power management properly when it's on (I can't confirm because I had FireChat installed and it's craziness)
Or did I misunderstand this patch altogether?
Comment Actions
Android O introduced new restrictions to background processes making it impossible for us to run in the background without this notification.
https://developer.android.com/about/versions/oreo/background.html
We can still disable it on Android <Oreo.
The only way to completely avoid it is to get rid of the BackgroundService which seems very difficult (if not impossible) to me.
Comment Actions
This isn't needed until we target Oreo, but eventually we will have to. See https://www.xda-developers.com/play-store-updated-requirements-api-level-64-bit/
Comment Actions
I would like to see it disables (at least by default) when not really needed, i.e. below Oreo
Comment Actions
I'll can do that, but those version checks (and the call to startForegroundService) require target API 26, which in turn has other requirements. An example is notification channels. Since I can't test Oreo-only things, I'll leave this diff open for now.
Comment Actions
The version checks are not really the issue (only from the not-a-magical-number angle). But as discussed above we need startForegroundService too, and that's not available without targeting Android Oreo.
I can implement only the version check now, if preferred?
Comment Actions
I can implement only the version check now, if preferred?
I think the whole patch should wait until we can target Oreo
Comment Actions
Apps that target Oreo/26 SHOULD also use version 26 of the support library. Eventually we have to target Oreo. Since only 0.4% of all devices use Android <15 I vote for dropping support instead of dealing with impossible-to-debug bugs that may occur when using old support libraries with new target api.
I just updated my phone to Oreo and can help testing now.