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 6 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
17 changes: 4 additions & 13 deletions NotificationNotes.iml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<module external.linked.project.id="NotificationNotes"
external.linked.project.path="$MODULE_DIR$"
external.root.project.path="$MODULE_DIR$"
external.system.id="GRADLE"
external.system.module.group=""
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">
Copy link
Owner

Choose a reason for hiding this comment

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

Are changes in this file necessary?

<component name="FacetManager">
<facet type="java-gradle" name="Java-Gradle">
<configuration>
Expand All @@ -15,14 +8,12 @@
</configuration>
</facet>
</component>
<component name="NewModuleRootManager"
LANGUAGE_LEVEL="JDK_1_7"
inherit-compiler-output="true">
<component name="NewModuleRootManager" LANGUAGE_LEVEL="JDK_1_7" inherit-compiler-output="true">
<exclude-output />
<content url="file://$MODULE_DIR$">
<excludeFolder url="file://$MODULE_DIR$/.gradle" />
</content>
<orderEntry type="inheritedJdk" />
<orderEntry type="jdk" jdkName="1.8" jdkType="JavaSDK" />
<orderEntry type="sourceFolder" forTests="false" />
</component>
</module>
</module>
184 changes: 147 additions & 37 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
@@ -1,14 +1,13 @@
package com.khuttun.notificationnotes;

import android.app.Notification;
import android.app.NotificationChannel;
import android.app.NotificationManager;
import android.app.PendingIntent;
import android.content.Context;
import android.content.Intent;
import android.graphics.Typeface;
import android.os.Build;
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,36 +22,39 @@ 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 NotificationCompat.Builder notificationBuilder;
private NotificationManagerCompat notificationManager;
private boolean groupAPI = false;

public NotificationMgr(Context context)
{
this.context = context;
this.notificationManager = (NotificationManager) this.context.getSystemService(Context.NOTIFICATION_SERVICE);
this.notificationManager = NotificationManagerCompat.from(this.context);

// NotificationChannel is required on Oreo and newer
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O)
// Grouping notifications together so they are expandable is available on Nougat and newer
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N)
{
this.notificationManager.createNotificationChannel(new NotificationChannel(
CHANNEL_ID,
this.context.getString(R.string.channel_name),
NotificationManager.IMPORTANCE_LOW));
this.groupAPI = true;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Seems you have removed the NotificationChannel. It's needed for Oreo and newer.


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
public NotificationCompat.Builder getNotificationBuilder()
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason for changing the builder to not be a member anymore?

Copy link
Author

Choose a reason for hiding this comment

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

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.

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

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,18 +67,11 @@ 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);
}
}

clearAllNotifications();
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

The function clearAllNotifications is now called in displayGroupNotifications.

Copy link
Owner

Choose a reason for hiding this comment

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

clearAllNotifications() is still called here?

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?

{
case 0:
Expand All @@ -90,43 +85,158 @@ 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.

ArrayList<Notification> groupedNotes = createGroupNotifications(visibleNotes);
for (int i = 0; i < groupedNotes.size(); i++)
{
Notification note = groupedNotes.get(i);
String title = note.extras.getString("android.title");
String text = note.extras.getString("android.text");
int id = -1;
if (i == groupedNotes.size() - 1)
{
id = SUMMARY_NOTIF_ID;
}
else if (i == 0)
{
id = GROUP_NOTIF_ID;
}
else
{
id = findNotificationID(notes, title, text);
}
this.notificationManager.notify("note", id, note);
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason for taking the tag parameter in use in the notify() call?

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

private int findNotificationID(ArrayList<NotificationNote> notes, String title, String text)
Copy link
Owner

Choose a reason for hiding this comment

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

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?

{
for (int j = 0; j < notes.size(); j++)
{
NotificationNote note = notes.get(j);
if (note.text == text && note.title == title)
{
return note.id;
}
}
return -1;
}

private ArrayList<NotificationNote> getVisibleNotes(ArrayList<NotificationNote> notes)
noaoh marked this conversation as resolved.
Show resolved Hide resolved
{
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");
this.notificationManager.cancelAll();
}


private Notification createNotification(String title, String text)
{
this.notificationBuilder.setStyle(new NotificationCompat.BigTextStyle().bigText(text));
this.notificationBuilder.setContentTitle(title);
this.notificationBuilder.setContentText(text);
return this.notificationBuilder.build();
NotificationCompat.Builder notificationBuilder = getNotificationBuilder();
notificationBuilder.setStyle(new NotificationCompat.BigTextStyle().bigText(text));
notificationBuilder.setContentTitle(title);
notificationBuilder.setContentText(text);
return notificationBuilder.build();
}

private Notification createGroupNotification(ArrayList<NotificationNote> notes)

/**
* Creates a notification with the number of notes and the first line of each note.
* Visible for Android versions below Nougat (7.0).
* Is required for Android versions Nougat and beyond, but does not show.
*/
private Notification createSummaryNotification(ArrayList<NotificationNote> notes)
{
NotificationCompat.Builder notificationBuilder = getNotificationBuilder();
NotificationCompat.InboxStyle style = new NotificationCompat.InboxStyle();
for (int i = 0; i < notes.size(); ++i)
{
style.addLine(getGroupNotificationLine(notes.get(i)));
}
this.notificationBuilder.setStyle(style);
this.notificationBuilder.setContentTitle(
notificationBuilder.setStyle(style);
notificationBuilder.setContentTitle(
this.context.getString(R.string.group_notif_title) + ": " + notes.size());
this.notificationBuilder.setContentText(getGroupNotificationLine(notes.get(0)));
return this.notificationBuilder.build();
notificationBuilder.setContentText(getGroupNotificationLine(notes.get(0)));
notificationBuilder.setGroup(GROUP_KEY);
notificationBuilder.setGroupSummary(true);
return notificationBuilder.build();
}


private CharSequence getGroupNotificationLine(NotificationNote note)
{
Spannable sp = new SpannableString(note.title + " " + note.text);
sp.setSpan(new StyleSpan(Typeface.BOLD), 0, note.title.length(), Spannable.SPAN_EXCLUSIVE_EXCLUSIVE);
return sp;
}

private ArrayList<Notification> createGroupNotifications(ArrayList<NotificationNote> notes)
Copy link
Owner

Choose a reason for hiding this comment

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

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?

{
ArrayList<Notification> groupedNotes = new ArrayList<Notification>();
Notification summary = createSummaryNotification(notes);
groupedNotes.add(summary);
for (int i = 0; i < notes.size(); ++i)
{
NotificationNote note = notes.get(i);
Notification newNote = createGroupNotification(note.title, note.text);
groupedNotes.add(newNote);
}

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.

Is this check necessary here? Is this function ever called if groupAPI is false?

{
Notification top = createTopNotification(notes.size());
groupedNotes.add(top);
}
return groupedNotes;
}

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

/**
* Creates a notification that shows how many notes there are.
*/
private Notification createTopNotification(int size)
{
String message = this.context.getString(R.string.group_notif_title) + ": " + size;
NotificationCompat.Builder notificationBuilder = getNotificationBuilder();
notificationBuilder.setContentTitle(message);
notificationBuilder.setGroup(this.GROUP_KEY);
notificationBuilder.setGroupSummary(false);
return notificationBuilder.build();
}
}