etesync / android

EteSync - Secure, end-to-end encrypted, and privacy respecting sync for your contacts, calendars and tasks.
https://www.etesync.com
GNU General Public License v3.0
296 stars 33 forks source link

Add `POST_NOTIFICATIONS` permission to app's manifest #237

Closed 0xedward closed 1 year ago

0xedward commented 1 year ago

Since we updated the target SDK version to 33 in 3f05b7fc1fcc7fea06d5dd2d863188a784ad27e9, we should request notifications permissions to continue allowing the user to receive journal changes and certificate notifications. In some cases, the lack of notifications may block Android 13 users from accepting certificates, which will prevent them from syncing with the Etebase server

Current state of EteSync on a device running Android 13: image

0xedward commented 1 year ago

There may be more work necessary for a nice user experience, like prompting the user to allow notifications during the first time a user launches EteSync. But for now, hopefully this PR is sufficient to allow existing users on Android 13 to get notifications by allowing them through the application information screen

barathrm commented 1 year ago

fyi @tasn

tasn commented 1 year ago

I'm really annoyed with myself over 2.4.2. I submitted to pressure by people to change the targetSDK and god how many things Android broke there without sufficient linting errors. :(

thgoebel commented 1 year ago

I just noticed this change when reading the changelog.

There may be more work necessary for a nice user experience, like prompting the user to allow notifications [...].

You are right there is more work necessary. But not just for a nice user experience, but for notifications to work at all. POST_NOTIFICATIONS is a runtime permission, which means declaring it is NOT enough. You need to request it at runtime (like READ_CALENDAR or WRITE_CONTACTS). Otherwise it will do nothing (unless the user manually navigates to the settings and grants it from there).

Granted, Google's docs don't really make this obvious if you skim them and only look at the code snippets. But they do say "[...] and complete a similar process compared to requesting other runtime permissions [...]". EteSync is already the second app (after FindMyDevice) that fell into this...

tasn commented 1 year ago

Oh god, @thgoebel, thanks for the info!

I wonder if it makes sense to just reduce the target API level to 32 and be done with this. Any thoughts?

0xedward commented 1 year ago

You are right there is more work necessary. But not just for a nice user experience, but for notifications to work at all.

Otherwise it will do nothing (unless the user manually navigates to the settings and grants it from there).

Sorry I'm not sure I understand - I'm reading both that notifications work and do not work.

Notifications work for me on Android 13. You have to manually allow them in EteSync's application info screen. Prompting/requesting the notifications permission at runtime is not necessary for notifications to work, however, in my opinion, is necessary for a good UX.

If you allow the notification permission for EteSync through the application info screen, do you receive any notifications, @thgoebel?

My intention with this PR and my earlier comment was to submit a hotfix to just make notifications work again. To make notifications work and to have a nice user experience with them, more work will need to be done (e.g. checking if EteSync has notifications allowed and prompting the user to allow notifications when the user enables anything that would send notifications in the app settings, like "Show change notifications" and "Distrust system certificates").

thgoebel commented 1 year ago

If you allow the notification permission for EteSync through the application info screen, do you receive any notifications, @thgoebel?

Yes.

But you as a dev know that you need to manually go into settings. None of the normal users will know. Notifications are not a core feature, users won't miss anything, so they have no reason to go to settings. Even I only noticed that notifications were off when I read your changelog.

So for the average user it is not about nice UX. It is about whether or not this notifications will work ~at all~ in practice.

but for notifications to work at all

Yes, considering your motivation with this PR ("sufficient to allow existing users on Android 13 to get notifications by allowing them through the application information screen") this statement of mine is too strong. Without this PR, this switch in the settings is blocked. So this PR is progress.

But in my opinion, if a camera app just shows a black screen and expects the user to think "oh I should go to settings and grant CAMERA" then this is not a matter of nice UX. It's a matter of "the app is not doing its job".

Yes, this PR hotfixes it for you. But for EteSync as a product notifications are still incomplete (nearly noone will manually grant notifications).

I wonder if it makes sense to just reduce the target API level to 32 and be done with this. Any thoughts?

Might work short term. I don't know what happens to users on Android 13 who had upgraded to 2.4.3 and the upgrade to "2.4.4" (with targetSdk 32). Would need to test this flow and that notifications work for them.

But on the other hand EteSync will have to jump through Google's hoops eventually anyway :/

0xedward commented 1 year ago

Yes.

Okay, good to know that you receive notifications if you allow the permission in the application info screen.

So for the average user it is not about nice UX. It is about whether or not this notifications will work at all in practice.

But in my opinion, if a camera app just shows a black screen and expects the user to think "oh I should go to settings and grant CAMERA" then this is not a matter of nice UX. It's a matter of "the app is not doing its job".

Yes, this PR hotfixes it for you. But for EteSync as a product notifications are still incomplete (nearly noone will manually grant notifications).

I intended to follow up with another PR when I had some extra time. You're welcome to submit a PR help EteSync reach the state you have in mind :). I'd imagine @tasn would be happy to review/accept it.

I wonder if it makes sense to just reduce the target API level to 32 and be done with this. Any thoughts?

I haven't done any testing so this is a guess, but I think dropping down to API level 32 shouldn't cause any problems for another year. At least, API level 32 wouldn't prevent future updates for users that download the app from Google Play Store for hopefully 1 more year:

Existing apps must target API level 31 or above to remain available to users on devices running Android OS higher than your app's target API level. Apps that target API level 30 or below (target API level 29 or below for Wear OS), will only be available on devices running Android OS the same or lower than your apps’ target API level.