Allow disabling persistent notification in Android 8.0 and up
ClosedPublic

Authored by brute4s99 on Fri, Jan 11, 11:33 AM.

Details

Summary

Show persistent notification toggle is disabled from Android O and up
This patch aims to enable the toggle with link to Notifications Settings, so that user can itself disable the persistent notification if desired

Test Plan

Apply patch, use on Android O or up device and use Show persistent notification toggle. It should take you to Notifications settings page of KDE Connect

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.
brute4s99 created this revision.Fri, Jan 11, 11:33 AM
Restricted Application added a project: KDE Connect. · View Herald TranscriptFri, Jan 11, 11:33 AM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
brute4s99 requested review of this revision.Fri, Jan 11, 11:33 AM
nicolasfella added inline comments.
src/org/kde/kdeconnect/UserInterface/SettingsFragment.java
77

Build.VERSION.SDK_INT >= Build.VERSION_CODES.O should be enough

brute4s99 updated this revision to Diff 49231.Fri, Jan 11, 12:36 PM

did the changes, please review

brute4s99 marked an inline comment as done.Fri, Jan 11, 12:40 PM
eduisters requested changes to this revision.Fri, Jan 11, 1:07 PM
eduisters added a subscriber: eduisters.

The "Show persistent notifications" switch should represent reality. If it's on the notification should be displayed and if it's off the notification should not be visible.

src/org/kde/kdeconnect/UserInterface/SettingsFragment.java
79

Why this if/else? You already tested for O above

This revision now requires changes to proceed.Fri, Jan 11, 1:07 PM
brute4s99 updated this revision to Diff 49246.Fri, Jan 11, 3:37 PM
This comment was removed by brute4s99.
brute4s99 added inline comments.Fri, Jan 11, 5:23 PM
src/org/kde/kdeconnect/UserInterface/SettingsFragment.java
79

you're right, I shall remove it

Single touch preference if Android >=Oreo
twoStatepref with previous functionality otherwise
My phone is using UK locales from strings.xml, so I haven't touched the UK locales.

brute4s99 updated this revision to Diff 49280.Fri, Jan 11, 6:42 PM

removed unreachable code

eduisters requested changes to this revision.Sat, Jan 12, 11:40 AM

I think changing the title to "Persistent Notification" and summary to "Click to enable/disable in Notification settings" would be better for Oreo and newer

src/org/kde/kdeconnect/UserInterface/SettingsFragment.java
82

indentation

84

indentation

86

Can be replaced by a lambda (hover on Preference.OnPreferenceClickListener() and press Alt+enter)

This revision now requires changes to proceed.Sat, Jan 12, 11:40 AM
brute4s99 updated this revision to Diff 49458.EditedMon, Jan 14, 4:28 PM
  1. Removed custom description for Oreo in Persistent Notification preference.
  2. Removed excess code according to above change.
  3. Added custom title for Persistent Notification preference if Android >= Oreo
  4. Renamed custom description for Pie in Persistent Notification preference to reflect new value , for Android >= Oreo
brute4s99 updated this revision to Diff 49491.Mon, Jan 14, 9:57 PM

reverted changes from other strings.xmls.

eduisters resigned from this revision.Tue, Jan 15, 2:37 PM
This comment was removed by eduisters.

+1 Look good to me

albertvaka accepted this revision.Wed, Jan 16, 1:14 PM
albertvaka added a subscriber: albertvaka.
albertvaka added inline comments.
res/values/strings.xml
317–318

Use lowercase "n" in notification.

src/org/kde/kdeconnect/UserInterface/SettingsFragment.java
35

Try not to include unrelated changes in patches, like this added space.

This revision is now accepted and ready to land.Wed, Jan 16, 1:14 PM
This revision was automatically updated to reflect the committed changes.