Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving group mode #33

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Improving group mode #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a462995
Adding new style of grouped notifications
noaoh Jan 8, 2019
ac18ce5
Figured out how to remove notifications from group
noaoh Jan 8, 2019
dff13b0
Added notification that tells how many notes to new API
noaoh Jan 9, 2019
52e6eef
Simplified summary notification generation
noaoh Jan 9, 2019
81bd4d2
Cleaned up a bit & added comments
noaoh Jan 9, 2019
46629d7
Seperated code paths for Android Nougat and Pre-Nougat
noaoh Jan 13, 2019
e05343b
Simplified display & creation of notifications
noaoh Jan 25, 2019
fd8a992
Reverted changes to .iml file
noaoh Jan 25, 2019
bc3f7eb
Reverted changes to notificationManager type
noaoh Jan 26, 2019
f59027b
Streamlined creation of group notes & fixed issue with unchecked note…
noaoh Jan 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/src/main/java/com/khuttun/notificationnotes/NotesListAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now multiple notifications with unique ids are created, so when one is unchecked, it does not go away when the notifications are remade.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Would it work if you rather called clearAllNotifications in the beginning of displayGroupNotifications? That would seem like a more robust approach.

this.notificationMgr.setGroupNotification(this.notes);
}
else
Expand Down
112 changes: 93 additions & 19 deletions app/src/main/java/com/khuttun/notificationnotes/NotificationMgr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
import android.content.Intent;
import android.graphics.Typeface;
import android.os.Build;
import android.os.Bundle;
import android.support.v4.app.NotificationCompat;
import android.support.v4.app.NotificationManagerCompat;
import android.text.Spannable;
import android.text.SpannableString;
import android.text.style.StyleSpan;
Expand All @@ -23,17 +25,22 @@ public class NotificationMgr
{
private static final String CHANNEL_ID = "notes";
private static final int GROUP_NOTIF_ID = -1000;

private static final int SUMMARY_NOTIF_ID = -2000;
private static final String GROUP_KEY = "com.khuttun.notificationnotes";
private Context context;
private NotificationManager notificationManager;
private boolean groupAPI = false;
private NotificationCompat.Builder notificationBuilder;

public NotificationMgr(Context context)
{
this.context = context;
// Need the NotificationManager type for a NotificationChannel,
// the official documentation saying to use NotificationManager.Compat is
// wrong
this.notificationManager = (NotificationManager) this.context.getSystemService(Context.NOTIFICATION_SERVICE);

// NotificationChannel is required on Oreo and newer
// A NotificationChannel is needed for Android Oreo and above
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O)
{
this.notificationManager.createNotificationChannel(new NotificationChannel(
Expand All @@ -42,17 +49,35 @@ public NotificationMgr(Context context)
NotificationManager.IMPORTANCE_LOW));
}

this.notificationBuilder = new NotificationCompat.Builder(this.context, CHANNEL_ID);
this.notificationBuilder.setSmallIcon(R.drawable.pen);
this.notificationBuilder.setOngoing(true);
this.notificationBuilder.setPriority(NotificationCompat.PRIORITY_LOW);
this.notificationBuilder.setContentIntent(PendingIntent
// Grouping notifications together so they are expandable is available on Nougat and newer
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N)
{
this.groupAPI = true;
}

notificationBuilder = new NotificationCompat.Builder(this.context, CHANNEL_ID);
notificationBuilder.setSmallIcon(R.drawable.pen);
notificationBuilder.setOngoing(true);
notificationBuilder.setPriority(NotificationCompat.PRIORITY_LOW);
notificationBuilder.setContentIntent(PendingIntent
.getActivity(this.context, 0, new Intent(this.context, MainActivity.class), 0));
}

/**
* Used for group mode to check if a notification that is
* currently being displayed needs to be canceled
*/
public void checkNotification(NotificationNote note)
{
if (!note.isVisible)
{
this.notificationManager.cancel(note.id);
}
}
public void setNotification(NotificationNote note)
{
if (Globals.LOG) Log.d(Globals.TAG, "Notification: ID " + note.id + ", show " + note.isVisible);

if (note.isVisible)
{
this.notificationManager.notify(note.id, createNotification(note.title, note.text));
Expand All @@ -65,16 +90,7 @@ public void setNotification(NotificationNote note)

public void setGroupNotification(ArrayList<NotificationNote> notes)
{
ArrayList<NotificationNote> visibleNotes = new ArrayList<>();
for (int i = 0; i < notes.size(); ++i)
{
NotificationNote n = notes.get(i);
if (n.isVisible)
{
visibleNotes.add(n);
}
}

ArrayList<NotificationNote> visibleNotes = getVisibleNotes(notes);
if (Globals.LOG) Log.d(Globals.TAG, "Group notification: visible notes " + visibleNotes.size());

switch (visibleNotes.size())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Expand All @@ -90,11 +106,33 @@ public void setGroupNotification(ArrayList<NotificationNote> notes)
break;

default:
this.notificationManager.notify(GROUP_NOTIF_ID, createGroupNotification(visibleNotes));
if (groupAPI)
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created the displayGroupNotifications function to do this.

displayGroupNotifications(visibleNotes);
}
else
{
this.notificationManager.notify(SUMMARY_NOTIF_ID,
createSummaryNotification(visibleNotes));
}
break;
}
}

private static ArrayList<NotificationNote> getVisibleNotes(ArrayList<NotificationNote> notes)
{
ArrayList<NotificationNote> visibleNotes = new ArrayList<>();
for (int i = 0; i < notes.size(); i++)
{
NotificationNote n = notes.get(i);
if (n.isVisible)
{
visibleNotes.add(n);
}
}
return visibleNotes;
}

public void clearAllNotifications()
{
if (Globals.LOG) Log.d(Globals.TAG, "Clearing all notifications");
Expand All @@ -109,7 +147,12 @@ private Notification createNotification(String title, String text)
return this.notificationBuilder.build();
}

private Notification createGroupNotification(ArrayList<NotificationNote> notes)
/**
* Creates a notification with the number of notes and the first line of each note.
* This notification is shown for Android Versions below Nougat, and is still necessary
* for Android Nougat, but is not shown.
*/
private Notification createSummaryNotification(ArrayList<NotificationNote> notes)
{
NotificationCompat.InboxStyle style = new NotificationCompat.InboxStyle();
for (int i = 0; i < notes.size(); ++i)
Expand All @@ -120,6 +163,8 @@ private Notification createGroupNotification(ArrayList<NotificationNote> notes)
this.notificationBuilder.setContentTitle(
this.context.getString(R.string.group_notif_title) + ": " + notes.size());
this.notificationBuilder.setContentText(getGroupNotificationLine(notes.get(0)));
this.notificationBuilder.setGroup(GROUP_KEY);
this.notificationBuilder.setGroupSummary(true);
return this.notificationBuilder.build();
}

Expand All @@ -129,4 +174,33 @@ private CharSequence getGroupNotificationLine(NotificationNote note)
sp.setSpan(new StyleSpan(Typeface.BOLD), 0, note.title.length(), Spannable.SPAN_EXCLUSIVE_EXCLUSIVE);
return sp;
}

private void displayGroupNotifications(ArrayList<NotificationNote> visibleNotes)
{
int visibleSize = visibleNotes.size();
for (int i = 0; i < visibleSize; i++)
{
NotificationNote note = visibleNotes.get(i);
if (Globals.LOG)
{
Log.d(Globals.TAG, "Displaying notification for note with id ".concat(Integer.toString(note.id)));
}
this.notificationManager.notify(note.id, createGroupNotification(note));
}
this.notificationManager.notify(SUMMARY_NOTIF_ID, createSummaryNotification(visibleNotes));
}

/**
* Creates a notification that is the member of a group & is not the summary.
*/
private Notification createGroupNotification(NotificationNote note)
{
this.notificationBuilder.setStyle(new NotificationCompat.BigTextStyle().bigText(note.text));
this.notificationBuilder.setContentTitle(note.title);
this.notificationBuilder.setContentText(note.text);
this.notificationBuilder.setGroup(this.GROUP_KEY);
this.notificationBuilder.setGroupSummary(false);
return this.notificationBuilder.build();
}
}