Review Request 121687: Added support to filter notifications according to applications

[Albert Vaca Cintora]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121687/#review72663
-----------------------------------------------------------


Good and necessary patch! I commented some issues below, but it looks good overall.


res/values-w820dp/dimens.xml
<https://git.reviewboard.kde.org/r/121687/#comment50635>

    This file is only for resolutions w820dp! Move it to the regular "values" directory, or just delete it and set the margin directly in the layout (that is how it's done in all the other activities).



src/org/kde/kdeconnect/Plugins/NotificationsPlugin/AppDatabase.java
<https://git.reviewboard.kde.org/r/121687/#comment50639>

    Instead of this, use the license headers that you can find in other files.



src/org/kde/kdeconnect/Plugins/NotificationsPlugin/AppDatabase.java
<https://git.reviewboard.kde.org/r/121687/#comment50641>

    checkentry -> checkEntry



src/org/kde/kdeconnect/Plugins/NotificationsPlugin/NotificationFilterActivity.java
<https://git.reviewboard.kde.org/r/121687/#comment50640>

    License header missing as well.



src/org/kde/kdeconnect/Plugins/NotificationsPlugin/NotificationFilterActivity.java
<https://git.reviewboard.kde.org/r/121687/#comment50636>

    I think we want to invert this: Have all the apps checked by default and manually "uncheck" the ones that we don't want. I think it makes more sense from a UX point of view to "disable" notifications by "unchecking" a box. This should not affect the internal behaviour: if a new app is installed, it should be enabled by default!



src/org/kde/kdeconnect/Plugins/NotificationsPlugin/NotificationFilterActivity.java
<https://git.reviewboard.kde.org/r/121687/#comment50638>

    "isPackageExisted" doesnt's sound right. Rename to "packageExists" or "isPackageInstalled" instead.


- Albert Vaca Cintora


On des. 26, 2014, 9:41 p.m., Vineet Garg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121687/
> -----------------------------------------------------------
> 
> (Updated des. 26, 2014, 9:41 p.m.)
> 
> 
> Review request for kdeconnect.
> 
> 
> Repository: kdeconnect-android
> 
> 
> Description
> -------
> 
> Notification sync settings is added where user can select applications for which he don't want to see notifications.
> 
> 
> Diffs
> -----
> 
>   src/org/kde/kdeconnect/Plugins/NotificationsPlugin/NotificationFilterActivity.java PRE-CREATION 
>   src/org/kde/kdeconnect/Plugins/NotificationsPlugin/NotificationsPlugin.java f54c48f 
>   src/org/kde/kdeconnect/UserInterface/PluginSettingsActivity.java abe1db1 
>   AndroidManifest.xml 6cbe4a7 
>   res/layout/activity_notification_filter.xml PRE-CREATION 
>   res/values-w820dp/dimens.xml PRE-CREATION 
>   res/values/strings.xml 8337783 
>   src/org/kde/kdeconnect/Plugins/NotificationsPlugin/AppDatabase.java PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/121687/diff/
> 
> 
> Testing
> -------
> 
> Testing done on emuulator with Android Lollipop by sending pings and Whatsapp messages to emulator via physical device both for application selected for filter and not selected for filter.
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20141229/fcb5ef44/attachment-0001.html>