syncthing / syncthing-android

Wrapper of syncthing for Android.
https://syncthing.net/
Mozilla Public License 2.0
3.61k stars 406 forks source link

Compact persistent notification #1048

Open KopfKrieg opened 6 years ago

KopfKrieg commented 6 years ago

I've seen all the other issues complaining about the persistent notification (#981, #994, #998, #1021), but I understand it's currently necessary (and, honestly, I don't have a problem with it. I like to know what's running in the backround).

Therefore, I suggest to make use of the compact message feature. For example, the app "Smarter Wi-Fi Manager" already supports this (tested on Lineage OS 15.1 / Android 8.1). It will look like this:

compact-notification

It will only show a very minimalistic notification. I think that's a good compromise and would be useful for Syncthing on Android too.

The notification itself can be expanded if necessary. I'm not sure how exactly this is implemented, and I couldn't find an answer in the developer's guide (maybe I just didn't see it), but I hope someone can figure this one out.

wweich commented 6 years ago

You can do this yourself by setting the persistent notification channel to low.

This should (IMHO) be the default thought.

KopfKrieg commented 6 years ago

You can do this yourself by setting the persistent notification channel to low.

This only removes the icon from the top bar, but doesn't compress the notification itself into something smaller. And of course, even if there's the option to do this, having it as default would still be nice :)

Default priority (on my system, but I didn't change anything about notifications) is "Medium". But that might be a bug, because I've found posts of people who can't set their app's notification priority below medium.

AudriusButkevicius commented 6 years ago

@capi did your pr fix this by any chance?

capi commented 6 years ago

@AudriusButkevicius My PR separates the channels, so you can have different priorities for "deactivated" and "active" (I like to see when it's active, but hide it when in background). It is still up to the user to change the channel's priority of the individual channels.

I personally leave it at "off", so that it is not shown at all. When I set the priority to "low", it is compressed. But I noticed that in this case it works worse than with "off", because the service still seems to be killed, since startForeground() requires a medium priority notification.

Also, due to the requirement of startForeground(), I was unable to set the default to "low". But as I said, it seems the user can still reduce the channel's priority manually afterwards.

If I turn it "off" the "Application is using power in the background" annoying notification of Android 8+ is triggered. But I have set the notification priority of this system notification to "low".

I am on LineageOS 15.1, there I can set this priority to "low". If this is due to LineageOS or due to 8.1, I don't know. Also, like I said, if you see the background service being killed with the notification's priority set to "low", you might have a try with "OFF" instead.

Some research on the web also indicates, that there are changes between 8.0 and 8.1 what you can do with the individual channels, because Google itself found that 8.0's behavior is unfortunate in this area. At least in 8.1 it seems the user can hide the "using power in the background" notification without workaround apps that periodically mute exactly this notification for two hours.

capi commented 6 years ago

What I also noticed: if you change the notification's priority, the effect (if it is a collapsed or expanded notification) takes place only after the notification is re-created by Syncthing again, e.g. by changing between "active" and "deactived" (after my PR).

KopfKrieg commented 6 years ago

Edit: Restarting syncthing after setting priority to low allows the notification to be collapsed. I totally missed that previously.

capi commented 6 years ago

@KopfKrieg I don't know of any way to actually improve the current behavior which requires the manual configuration of the priority. The requirement to restart the app in order to have the icon be collapsed is not good, but that seems more a limitation of the Android notification system at the moment and I don't know any way around it.

Does this solve this issue and we can close it?

KopfKrieg commented 6 years ago

I don't know of any way to actually improve the current behavior which requires the manual configuration of the priority.

Creating the foreground service notification with low priority would fix my issue. According to the documentation this should be possible:

To request that your service run in the foreground, call startForeground(). This method takes two parameters: an integer that uniquely identifies the notification and the Notification for the status bar. The notification must have a priority of PRIORITY_LOW or higher.


@capi:

But I noticed that in this case it works worse than with "off", because the service still seems to be killed, since startForeground() requires a medium priority notification.

This shouldn't be the case. Documentation says low priority is allowed.

Also, like I said, if you see the background service being killed with the notification's priority set to "low", you might have a try with "OFF" instead.

Huh? Low should leave the service running, while Off might allow Android to kill it faster if necessary.

I'm on LineageOS 15.1 too, and here I don't have any problems with priority set low.


I found a (for me) confusing part in this function: If I understand this correctly, the default priority is already low?!

This is confusing, especially because after installing the app the priority was set to medium on my device (and I've done a completely clean install [wiped everything] only a few days ago, so there shouldn't be any leftovers from previous installations).

Could someone explain why this is the case? Because, if it's already set to low, I'd just close the issue.

capi commented 6 years ago

@KopfKrieg You are right, the "being killed" on PRIORITY_LOW must have been some artifacts during development. I have tested it with PRIORITY_LOW for the weekend, works as expected, like you say. And I must have mixed up some priorities in my head.

I have not really debugged the existing parts when splitting the channels. I can imagine that the if in line 122 (https://github.com/syncthing/syncthing-android/blob/7194e25a5cacff14b3df85f4cf2e17d415a20038/app/src/main/java/com/nutomic/syncthingandroid/service/NotificationHandler.java#L122-L123) does not trigger. And I don't know what a notification channel with IMPORTANCE_MIN does with a default priority notification.

Unfortunately this method is a mix of old and new behavior and has grown with the Android versions. I'll need to work through it what it does without default settings.

KopfKrieg commented 6 years ago

I can imagine that the if in line 122 does not trigger.

Unfortunately, I don't know either.

I'll need to work through it what it does without default settings.

Sure, no problem.


Edit: Documentation says the hierarchy of priorities is min < low < default < high < max. I don't know, but maybe PRIORITY_MIN falls back to PRIORITY_DEFAULT because foreground services can't be started with a PRIORITY_MIN as PRIORITY_LOW is required as minimum? Just a guess, though.

Ajedi32 commented 6 years ago

Is there a reason why this notification is needed at all? I realize that Oreo doesn't allow background apps to listen for implicit broadcasts anymore, but the proper solution to that is not to just run a foreground service 24/7. Instead what should happen is that Syncthing should use the JobScheduler to tell Android to run a sync the next time Wi-Fi is available (and/or power is connected).

capi commented 6 years ago

@Ajedi32 you are completely right about that, and hopefully in the future this can be realized. It is a little bit complicated with backwards compatibility, since JobScheduler is available only since API level 21, we currently support back to API level 14.

So the short-term workaround was to have the service run with foreground-priority, which on the other hand requires the notification.

Catfriend1 commented 6 years ago

Conclusion of the other issues: JobScheduler should be checked if it could be implemented Android version specific. Faq should be updated

qianbinbin commented 2 years ago

I think it advisable to add an option to run without persistent notifications in root mode

Ajedi32 commented 2 years ago

FYI, it looks like the new recommended way to do this is with WorkManager. It's part of Android Jetpack and should have better comparability with older API levels.

andreapx commented 2 months ago

So the short-term workaround was to have the service run with foreground-priority, which on the other hand requires the notification.

@capi Tasker woked around this problem creating a "useless notification" that can be hidden and all the other notifications still work (if I didn't get it wrong). Can this be implemented in Syncthing too?

capi commented 2 months ago

@andreapx I have not contributed to Syncthing on Android in a long time, sorry. I was under the impression that there was a PR for that at one point in the distant past to introduce various notification channels. But in general, I'm quite sure that this would be possible.