Add Privacy Options for Notifications Forwarded to Desktop.
Details
Details
- Reviewers
sredman - Group Reviewers
KDE Connect - Maniphest Tasks
- T9850: Privacy Options for Notifications Forwarded to Desktop
- Commits
- R225:6ba6842fc75e: Privacy Options for Notifications Forwarded to Desktop
- Open the android app.
- Go to Notifications Plugin Filter List (?)
- Long click any application.
- Press "Privacy options".
- Set your options.
- Close the menu and get a notification.
Diff Detail
Diff Detail
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 5320 Build 5338: arc lint + arc unit
Comment Actions
I haven't completely figured out the problem yet, but one thing I have noticed is that the enable/disable checkbox for an app is no longer saved. This suggests to me that something about the way the database is set up is causing it to either not be read-able or not be write-able. You could check the return value from ourDatabase.insert. If it is -1, it means there was an error.
My suggestion would be to make two database tables. Leave the old one for KEY_IS_ENABLED like it was, then make a second table with KEY_PRIVACY_OPTIONS. This makes more sense to me from a database standpoint anyway, because there is no relationship between being enabled and having privacy options set
Comment Actions
This was the problem. If you like having everything in one table better, then feel free to keep working in this direction. Otherwise, I think multiple tables is the way to go.
Also, once that is working, a change I would suggest is to block the reply-able button on a content-private notification, since it doesn't really make sense to try to reply if there is nothing to reply to. Also, I think having the app name, instead of "New Notification" is a bit nicer.
If you want to work on this yourself, go for it! Otherwise, I have uploaded the patch with my changes here: https://phabricator.kde.org/differential/diff/46219/ . Be sure you understand what is going on, since GCi is about teaching, not just me providing the answer :)
(The complete diff is here: https://phabricator.kde.org/differential/diff/46218/)
Comment Actions
Thanks Simon! Your code is really easy to understand. Initiallly I wanted to work on my own. It turned out, that I did the same things as you, but it didn't work. Then I realised, that I had to delete KDE Connect and reinstall it, because it had an old table, which was in the way. Now everything works just right with my code and your code, but yours is more ... elegant. If you don't mind, I will add your diff to this revision, so we can close it.
Comment Actions
Go ahead and use my version. There are a few small things I noticed which I will point out after uploading (because the other form doesn't accept comments!)
Comment Actions
Diff by sredman, fix of bug with tables in AppDatabase.java and some beautification of NotificationsPlugin.java
| src/org/kde/kdeconnect/Plugins/NotificationsPlugin/AppDatabase.java | ||
|---|---|---|
| 166 ↗ | (On Diff #46218) | I forgot to clean up this comment |
| 177–191 ↗ | (On Diff #46218) | These sections could do with some comments explaining how you are using the encoded bits to store the two privacy options. |
| src/org/kde/kdeconnect/Plugins/NotificationsPlugin/NotificationsPlugin.java | ||
| 260–262 ↗ | (On Diff #46090) | I was messing around with this (thus the commented-out line). Do any of these need to be here? I don't think so, but we should test. If not, then the "else" block which matches this "if" can probably be combined into the "if (!blockContents)" block starting at line 250 |
Comment Actions
- Privacy Options for Notifications Forwarded to Desktop v5
Cosmetic changes, added dev comments, added normal comments, xml values tweaked.
Comment Actions
I think, I've fixed everythng.
| src/org/kde/kdeconnect/Plugins/NotificationsPlugin/AppDatabase.java | ||
|---|---|---|
| 177–191 ↗ | (On Diff #46218) | I added comments. I don't thing, that idea with two booleans are good, because back when I wrote this code, I focused, so if someone wants to add a new option, for example, to ignore all notification of the app ( for example if it's Facebook messenger xD ). With just an integer the code isn't that easy, but all you have to do is to add a new enum value and that's it. With two booleans it wouldn't be much harder actually... but where's the fun in that? (I guess...) |
| src/org/kde/kdeconnect/Plugins/NotificationsPlugin/NotificationsPlugin.java | ||
| 260–262 ↗ | (On Diff #46090) | I guess, I did what you said... |
Comment Actions
This is looking really good! I noticed that there seems to be a problem with the privacy options not being per-connection. I hid the contents for Gmail on my computer at home, but now Gmail contents are blocked on my work computer as well! Do you have some way to test with two desktop devices? I can look in to this as well, but I will not have time for at least a week, probably longer
| src/org/kde/kdeconnect/Plugins/NotificationsPlugin/AppDatabase.java | ||
|---|---|---|
| 177–191 ↗ | (On Diff #46218) | Okay, sounds fair to me |
| src/org/kde/kdeconnect/Plugins/NotificationsPlugin/NotificationsPlugin.java | ||
| 260–262 ↗ | (On Diff #46090) | Sorry, I was unclear. Does the packet need any of these fields set? Does it work to just leave off "ticker" and "title", rather than assign them to empty strings? |
Comment Actions
Thanks Simon. This behaviour is logical as AppDatabase is unified and used for every device with NotificationsPligin. And NotificationsPlugin's settings doesn't have any information of connected devices, so there isn't any elegant way of doing things. I see two ways of implementing this:
- Creating an AppDatabase property for every Device, add a spinner with every remembered device. When selected, the corresponding device's AppDatabase should be updated. For this I will have to somehow provide NotificationsPluginFilter with a list of all devices.
- Copying a Notifications Filter List (without any checkboxes) to a menu for every device (like I did at the beginning).
Also, I have noticed, that the task I wanted to claim (write about developing your own plugin), was already taken, I thought it was not 'single-claim' task, I already had an idea for a plugin, but I guess I'll have to work on it after the gci ends.
Comment Actions
Sorry this took so long
I finally had a chance to check, and you are correct. The 'bug' where the notification plugin settings are not per-device is there on the master build too. In that case, it's a problem that should be solved, but not as part of this patch :)
I think this is good work. There is one part that's missing, though. If you notice, when you build from master or you use the Play Store build, then you upgrade to a build with this patch, the app will crash with some error about missing a table. This is because we don't currently have an onUpgrade which handles putting in the new tables, so someone upgrading would have to wipe their app data! This is obviously not great.
There is a solution. If you want to learn about it, look here is the relevant tutorial: https://thebhwgroup.com/blog/how-android-sqlite-onupgrade
I have made a revision which depends on this one: D17521. We can merge that revision right after merging this one, then everything should be working! :)
Comment Actions
Hi, Simon! Thanks for your help. I've also run into this issue with updating tables at the time, I managed to solve it by deleting the app before debugging on my phone with android studio. Somewhy I didn't think, that I can fix it, but now it's done. I also fixed that if block which was a bit 'overcoded'.
| src/org/kde/kdeconnect/Plugins/NotificationsPlugin/NotificationsPlugin.java | ||
|---|---|---|
| 260–262 ↗ | (On Diff #46090) | Now it's done, I think. |