Improving group mode #33
Conversation
|
Hi, thank you for the PR! Unfortunately it breaks the app on API level 15 (creating a grouped notification with 2 notes makes the app crash). This is a nice feature, but I think we should make the code paths for Nougat and pre-Nougat completely separate to avoid triggering functions that are not available on older Android. |
NotificationNotes.iml
Outdated
| external.system.module.version="unspecified" | ||
| type="JAVA_MODULE" | ||
| version="4"> | ||
| <module external.linked.project.id="NotificationNotes" external.linked.project.path="$MODULE_DIR$" external.root.project.path="$MODULE_DIR$" external.system.id="GRADLE" type="JAVA_MODULE" version="4"> |
There was a problem hiding this comment.
Are changes in this file necessary?
| this.notificationBuilder.setOngoing(true); | ||
| this.notificationBuilder.setPriority(NotificationCompat.PRIORITY_LOW); | ||
| this.notificationBuilder.setContentIntent(PendingIntent | ||
| public NotificationCompat.Builder getNotificationBuilder() |
There was a problem hiding this comment.
What's the reason for changing the builder to not be a member anymore?
There was a problem hiding this comment.
I have been unable to get the notification that shows how many notes there (the top notification) to work using the builder as a member variable. In general, having a new builder each time makes the building of notifications a bit easier to reason about.
| } | ||
| } | ||
|
|
||
| clearAllNotifications(); |
There was a problem hiding this comment.
Calling clearAllNotifications() here doesn't seem right. Would it make more sense to only call this when setting the Nougat+ group notifications, if this is needed for them?
There was a problem hiding this comment.
The function clearAllNotifications is now called in displayGroupNotifications.
| @@ -90,43 +85,158 @@ public void setGroupNotification(ArrayList<NotificationNote> notes) | |||
| break; | |||
|
|
|||
| default: | |||
| this.notificationManager.notify(GROUP_NOTIF_ID, createGroupNotification(visibleNotes)); | |||
| if (groupAPI) | |||
| { | |||
There was a problem hiding this comment.
It would be clearer to put the code for creating the grouped notifications to a function so that both if and else branches here could be one-liners.
There was a problem hiding this comment.
I've created the displayGroupNotifications function to do this.
| { | ||
| id = findNotificationID(notes, title, text); | ||
| } | ||
| this.notificationManager.notify("note", id, note); |
There was a problem hiding this comment.
What's the reason for taking the tag parameter in use in the notify() call?
| break; | ||
| } | ||
| } | ||
|
|
||
| private int findNotificationID(ArrayList<NotificationNote> notes, String title, String text) |
There was a problem hiding this comment.
This function doesn't really work right if you happen to have two notes with identical title and text. Would it be possible to restructure code so that function like this wouldn't be needed?
app/src/main/java/com/khuttun/notificationnotes/NotificationMgr.java
Outdated
Show resolved
Hide resolved
| groupedNotes.add(newNote); | ||
| } | ||
|
|
||
| if (groupAPI) |
There was a problem hiding this comment.
Is this check necessary here? Is this function ever called if groupAPI is false?
| } | ||
| } |
There was a problem hiding this comment.
Seems you have removed the NotificationChannel. It's needed for Oreo and newer.
| } | ||
| } | ||
|
|
||
| clearAllNotifications(); |
There was a problem hiding this comment.
clearAllNotifications() is still called here?
| * Creates the group of notifications that will be bundled together into one notification. | ||
| * Used for Android Nougat (7.0) and beyond. | ||
| */ | ||
| private ArrayList<Notification> createGroupNotifications(ArrayList<NotificationNote> notes) |
There was a problem hiding this comment.
If you wouldn't use this function, but rather called createGroupNotification() directly from displayGroupNotifications() while looping through visibleNotes, you wouldn't need the whole "note.id" Bundle, right?
| @@ -202,6 +202,7 @@ private void setNotification(NotificationNote n) | |||
| if (PreferenceManager.getDefaultSharedPreferences(this.context).getBoolean(this.context | |||
| .getString(R.string.group_notif_pref_key), false)) | |||
| { | |||
| this.notificationMgr.checkNotification(n); | |||
There was a problem hiding this comment.
I don't see why this would be needed? Changing the group notes setting should update the notifications to reflect the current state (see SettingsActivity).
There was a problem hiding this comment.
Now multiple notifications with unique ids are created, so when one is unchecked, it does not go away when the notifications are remade.
There was a problem hiding this comment.
I see. Would it work if you rather called clearAllNotifications in the beginning of displayGroupNotifications? That would seem like a more robust approach.
| } | ||
| } | ||
|
|
||
| ArrayList<NotificationNote> visibleNotes = getVisibleNotes(notes); | ||
| if (Globals.LOG) Log.d(Globals.TAG, "Group notification: visible notes " + visibleNotes.size()); | ||
|
|
||
| switch (visibleNotes.size()) |
There was a problem hiding this comment.
The "case 0" and "case 1" don't really work when using the grouped notifications. Would it make sense to leave this switch-case to be used only when groupAPI == false, and always call displayGroupNotifications when it's true?
Also, now you are using GROUP_NOTIF_ID in cases 0 and 1, and SUMMARY_NOTIF_ID in default case. That will not work. Consider should you just remove GROUP_NOTIF_ID?
| @@ -202,6 +202,7 @@ private void setNotification(NotificationNote n) | |||
| if (PreferenceManager.getDefaultSharedPreferences(this.context).getBoolean(this.context | |||
| .getString(R.string.group_notif_pref_key), false)) | |||
| { | |||
| this.notificationMgr.checkNotification(n); | |||
There was a problem hiding this comment.
I see. Would it work if you rather called clearAllNotifications in the beginning of displayGroupNotifications? That would seem like a more robust approach.
As of Android KitKat (7.0) the notes can be bundled together in a group. Each note within this group is expandable, and their full contents can be viewed.