D29335: Implement notification grouping on Android

2020-05-19 Thread Volker Krause 
This revision was automatically updated to reflect the committed changes.
Closed by commit R289:942bddded289: Implement notification grouping on Android 
(authored by vkrause).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D29335?vs=81731=83063#toc

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29335?vs=81731=83063

REVISION DETAIL
  https://phabricator.kde.org/D29335

AFFECTED FILES
  src/android/org/kde/knotifications/KNotification.java
  src/android/org/kde/knotifications/NotifyByAndroid.java
  src/notifybyandroid.cpp

To: vkrause, nicolasfella
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29335: Implement notification grouping on Android

2020-05-02 Thread Volker Krause 
vkrause updated this revision to Diff 81731.
vkrause added a comment.


  Replace the simple ref count with a full child id tracking.
  
  The ref count got out of sync when an existing notification is updated, using 
a set fixes that.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29335?vs=81725=81731

BRANCH
  grouping

REVISION DETAIL
  https://phabricator.kde.org/D29335

AFFECTED FILES
  src/android/org/kde/knotifications/KNotification.java
  src/android/org/kde/knotifications/NotifyByAndroid.java
  src/notifybyandroid.cpp

To: vkrause, nicolasfella
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29335: Implement notification grouping on Android

2020-05-02 Thread Volker Krause 
vkrause added a comment.


  Still not good enough, updating existing notfication messes up the 
refcounter, resulting still in leftover group elements.

REPOSITORY
  R289 KNotifications

BRANCH
  grouping

REVISION DETAIL
  https://phabricator.kde.org/D29335

To: vkrause, nicolasfella
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29335: Implement notification grouping on Android

2020-05-02 Thread Volker Krause 
vkrause updated this revision to Diff 81725.
vkrause added a comment.


  Explicitly track if notification groups are still in use.
  
  This fixes group summaries staying active when we explicitly
  close a notification, rather then having the user or system
  dismiss it.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29335?vs=81678=81725

BRANCH
  grouping

REVISION DETAIL
  https://phabricator.kde.org/D29335

AFFECTED FILES
  src/android/org/kde/knotifications/KNotification.java
  src/android/org/kde/knotifications/NotifyByAndroid.java
  src/notifybyandroid.cpp

To: vkrause, nicolasfella
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29335: Implement notification grouping on Android

2020-05-02 Thread Volker Krause 
vkrause added a comment.


  This isn't good to go yet, there are corner cases where the group summary 
item stays around after closing the last notification, working on fixing this.

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29335

To: vkrause, nicolasfella
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29335: Implement notification grouping on Android

2020-05-01 Thread Nicolas Fella 
nicolasfella added inline comments.

INLINE COMMENTS

> vkrause wrote in NotifyByAndroid.java:171
> That seems counter-productive to me, as the Android API documentation always 
> speaks about API level numbers, so you'd need to do an additional translation 
> to/from the letters in your head. So for that I find "26" more useful here 
> than "O". It's also consistent with the rest of the code in this file.

You have a point

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29335

To: vkrause, nicolasfella
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29335: Implement notification grouping on Android

2020-05-01 Thread Volker Krause 
vkrause added inline comments.

INLINE COMMENTS

> nicolasfella wrote in NotifyByAndroid.java:171
> Please use 
> https://developer.android.com/reference/android/os/Build.VERSION_CODES 
> instead of hardcoding numbers

That seems counter-productive to me, as the Android API documentation always 
speaks about API level numbers, so you'd need to do an additional translation 
to/from the letters in your head. So for that I find "26" more useful here than 
"O". It's also consistent with the rest of the code in this file.

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29335

To: vkrause, nicolasfella
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29335: Implement notification grouping on Android

2020-05-01 Thread Nicolas Fella 
nicolasfella accepted this revision.
nicolasfella added a comment.
This revision is now accepted and ready to land.


  Haven't tested it, but the code looks sensible

INLINE COMMENTS

> NotifyByAndroid.java:171
> +Notification.Builder builder;
> +if (Build.VERSION.SDK_INT >= 26) {
> +builder = new Notification.Builder(m_ctx, 
> notification.channelId);

Please use 
https://developer.android.com/reference/android/os/Build.VERSION_CODES instead 
of hardcoding numbers

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29335

To: vkrause, nicolasfella
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29335: Implement notification grouping on Android

2020-05-01 Thread Volker Krause 
vkrause added a comment.


  Collapsed: F8276057: Screenshot_20200501_161952.PNG 

  Expanded: F8276059: Screenshot_20200501_162043.PNG 


REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D29335

To: vkrause
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29335: Implement notification grouping on Android

2020-05-01 Thread Volker Krause 
vkrause created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
vkrause requested review of this revision.

REVISION SUMMARY
  This is available starting at API level 20, which is below our minimal
  requirement. Grouping can be disabled by the already existing SkipGrouping
  flag.
  
  When enabled this puts all notifications from the same eventId in a group,
  which makes the display more compact by removing the otherwise duplicated
  app name.

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29335

AFFECTED FILES
  src/android/org/kde/knotifications/KNotification.java
  src/android/org/kde/knotifications/NotifyByAndroid.java
  src/notifybyandroid.cpp

To: vkrause
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns