Differential D18181

Allow disabling persistent notification in Android 8.0 and up
ClosedPublic
Actions

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

Details

Reviewers
albertvaka
Group Reviewers
KDE Connect
Commits
R225:a6ae97afb127: Allow disabling persistent notification in Android 8.0 and up
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.Jan 11 2019, 11:33 AM
Restricted Application added a project: KDE Connect. · View Herald TranscriptJan 11 2019, 11:33 AM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
brute4s99 requested review of this revision.Jan 11 2019, 11:33 AM
nicolasfella added a subscriber: nicolasfella.Jan 11 2019, 12:19 PM
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.Jan 11 2019, 12:36 PM

did the changes, please review

brute4s99 marked an inline comment as done.Jan 11 2019, 12:40 PM
eduisters requested changes to this revision.Jan 11 2019, 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.Jan 11 2019, 1:07 PM
brute4s99 updated this revision to Diff 49246.Jan 11 2019, 3:37 PM
This comment was removed by brute4s99.
brute4s99 added inline comments.Jan 11 2019, 5:23 PM
src/org/kde/kdeconnect/UserInterface/SettingsFragment.java
79

you're right, I shall remove it

brute4s99 added a comment.Jan 11 2019, 5:23 PM

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.Jan 11 2019, 6:42 PM

removed unreachable code

eduisters requested changes to this revision.Jan 12 2019, 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.Jan 12 2019, 11:40 AM
brute4s99 updated this revision to Diff 49458.EditedJan 14 2019, 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.Jan 14 2019, 9:57 PM

reverted changes from other strings.xmls.

eduisters resigned from this revision.Jan 15 2019, 2:37 PM
This comment was removed by eduisters.
eduisters removed a reviewer: eduisters.Jan 15 2019, 2:51 PM

+1 Look good to me

albertvaka accepted this revision.Jan 16 2019, 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.Jan 16 2019, 1:14 PM
Closed by commit R225:a6ae97afb127: Allow disabling persistent notification in Android 8.0 and up (authored by brute4s99, committed by aacid). · Explain WhyJan 16 2019, 10:49 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 49675

View Options

res/values/strings.xml

Loading...
View Options

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

Loading...