-
-
Notifications
You must be signed in to change notification settings - Fork 147
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
set targetSdkVersion to 33 #2649
Conversation
To test the changes in this pull request, install this apk: |
I am trying this, for me I had notifications disabled for $reasons (to avoid annoying notifications from my dev/test DC installation) after upgrading I could not enable notifications, in system settings the toggle to enable/disable notifications was disabled, after reverting to target 32 when opening the app I see the popup dialog requesting notifications permissions, maybe something changed there and the way the app is requesting notifications permissions is not compatible with target 33 |
To test the changes in this pull request, install this apk: |
To test the changes in this pull request, install this apk: |
To test the changes in this pull request, install this apk: |
95e7e2f
to
b76fd37
Compare
To test the changes in this pull request, install this apk: |
not sure, if this affects getSystemService(Context.WIFI_SERVICE), however, we anyway do not ask for permissions here but just try/catch - if things do not work, we just cannot get the wifi name (we're not asking for permission as this would require LOCATION in the past - and that would scare users that just installed Delta Chat and select "Add Second Devive". this is the exact reason why NEARBY_WIFI_DEVICES was introduced, btw. so in case getting the Wifi name does not work on api33, we can consider asking for a permission - would still be on startup, however)
… notifications in the system settings
To test the changes in this pull request, install this apk: |
To test the changes in this pull request, install this apk: |
To test the changes in this pull request, install this apk: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
this PR updates targetSdkVersion to 33, compiling and building works, however, this kind if updates usually also comes with behaviour changes (see https://developer.android.com/about/versions/13/behavior-changes-all ) that need to be checked carefully:
concrete things that do not work at a first glance:
attach audio or gallery (instead of permission request, the fallback "Permission required" info is shown)
the "quick image attach bar" is
not shownempty as the permissions are granted, but reading photos failscheck remaining occurrences of READ_EXTERNAL_STORAGE and WRITE_EXTERNAL_STORAGE: EDIT: done, we keep the permissions in the code, however bypass the check on api33
check if writing files (backups, keys, export images, ...) work EDIT: log: OK , key: OK , backup: OK , export attachment (imasge): OK
"Privacy": "Runtime permission for notifications": "Android 13 (API level 33) introduces a runtime notification permission: POST_NOTIFICATIONS. This change helps users focus on the notifications that are most important to them."
also see below
add a device message if the user taps "deny notifications"
handle "denied notifications" in the system settings
general aspects (unticked items need to be checked, please add a summary-IMPACT next to the items then):
"Performance and battery": "Task Manager": Apps must be able to handle this user-initiated stopping." - IMPACT: things seem to work as expected, when delta is restarted, the permanent notification reappears (users can disable it in the settings). system looks like the following:
"Performance and battery": "Improve prefetch job handling using JobScheduler": IMPACT: JobScheduler seems to be unused by us
"Performance and battery": "High Priority Firebase Cloud Message (FCM) Quotas": IMPACT we do not use FCM
"Performance and battery": "Battery Resource Utilization": there is a new battery mode "restricted" where "foreground services" cannot be launched (so, our permantent notification) - we should make sure, that we do not crash in this case - IMPACT: looks good at a first glance, a beta test with different, real devices will tell more
"Privacy": "Hide sensitive content from clipboard" - IMPACT: should not affect us
"Security": "Intent filters block non-matching intents" - IMPACT: i cannot really make sense of the link, however, in practise, things work on first tries :)
"Security": "Migrate away from shared user ID": IMPACT: we're not using android:sharedUserId
"User experience": "Dismissible foreground service notifications" - not sure if this is the same as "Performance and battery": "Task Manager" above - IMACT: assuming, the foreground service is still active if the notification is dismissed, this is a nice feature. iirc, hiding/dismissing foreground notifications was also possible before. however, if it turns out, the dismissed notification affects the service, we should call setOngoing()
"Core functionality": "Legacy copy of speech service implementation removed" - IMPACT: we're not using RecognitionService or SpeechService
closes #2599