Add Notification Channels configuration for Android O #713

iernie · 2017-08-19T11:50:02Z

In response to my own issue #711 I tried to add the support myself.

I added the new meta-tag com.parse.push.notification_channel which defaults to "parse_push".
If the Android version SDK is 26 or above, the channelId is set to the notification. The project is also compiled against SDK 26 in order to get the notification api.

I have no idea how to test this, so any help or pointers are appreciated.

natario1 · 2017-08-19T17:37:11Z

build.gradle

@@ -22,11 +22,11 @@ allprojects {
 }

 ext {
-    compileSdkVersion = 25
+    compileSdkVersion = 26
    buildToolsVersion = "25.0.3"


Should this be changed? Also in travis. I'm not sure, just asking.

iernie · 2017-08-19T18:19:00Z

What I forgot to mention here is that I only set the channel id for the notification, and then it's up to the user to actually make a channel with that id in their app.

natario1 · 2017-08-20T12:01:16Z

So I'll leave you my own view on this... Currently I'm not comfortable with the facts that:

developer must also call notificationManager.createNotificationChannel(id) to have this work
the point of channels is that you can have multiple ones, but with manifest we practically only support a single channel

I think that:

Manifest support should be removed. That makes sense only in a single-channel setup
The broadcast receiver should just have a protected NotificationChannel getNotificationChannel(Context, Intent), defaulting to something like new NotificationChannel("parse_push", "Push notifications", IMPORTANCE_DEFAULT)
After we call getNotificationChannel(), we should also care about creating the channel if it does not exist

This way developer can create different channels based on different pushes inside the Intent, with a reasonable default, and also it works out of the box because we create the channel ourselves. Developer also has the chance to customize the NotificationChannel with lights, vibration etc.

What do you guys think?

It would be nice to merge this soon since O is rolling out

iernie · 2017-08-20T12:55:54Z

I agree with you @natario1. I've tried to modify my code based on your feedback.

natario1 · 2017-08-21T07:35:58Z

Thanks @iernie !

I like the design, but currently I think this would break for example here. When API < O, you are passing around a null NotificationChannel, but the NotificationChannel does not exist in these platforms. Right?

I think you should ensure that any reference to NotificationChannel happens in the if (version >= O) blocks... For example you could do

    NotificationCompat.Builder parseBuilder = new NotificationCompat.Builder(context);
    parseBuilder.setContentTitle(title)
        .setContentText(alert)
        .set ...;
    if (androidVersion >= O) {
        NotificationChannel channel = getNotificationChannel(); // Not called before O
        parseBuilder.setNotificationChannel(channel); // This will use the channel id
    }

Also this call to getNotificationChannel() is redundant, we already call it from getNotification()

natario1 · 2017-08-21T07:38:24Z

Then in ParseNotificationManager you could just pass the notification as it was and, if O, get the channel with nm.getNotificationChannel(notification.getChannelId()) and create it

iernie · 2017-08-21T07:49:09Z

nm.getNotificationChannel(notification.getChannelId()) won't work because the NotificationManager hasn't created the notification channel yet. If I don't pass the channel to ParseNotificationManager the channel object is lost once the notification is built.

iernie · 2017-08-21T07:51:31Z

What could be done is get NotificationManager and create the channel when you call getNotificationChannel()

iernie · 2017-08-21T07:58:30Z

@natario1 I've made some changes. getNotificationChannel() is only called on SDK >= 26 and null is never returned. It also adds it to the NotificationManager immediately. So there is minimal change to the existing code.

natario1 · 2017-08-21T08:23:13Z

Parse/src/main/java/com/parse/ParsePushBroadcastReceiver.java

+    NotificationChannel notificationChannel = new NotificationChannel("parse_push", "Push notifications", NotificationManager.IMPORTANCE_DEFAULT);
+
+    // Android doesn't create a new channel if the properties of the channel hasn't changed
+    nm.createNotificationChannel(notificationChannel);


Could you move the channel creation outside, maybe in getNotification(), right after we call getNotificationChannel() ? If someone overrides this method, the creation won't happen

…otification

iernie · 2017-08-21T08:50:24Z

I made the createNotificationChannel() method overrideable as well in case somebody wants to add multiple channels. Of course, they could just do that from within their app as well, but It's nice to have the option I guess.

natario1 · 2017-08-21T09:52:00Z

Parse/src/main/java/com/parse/NotificationCompat.java

@@ -124,6 +128,7 @@ public Notification build(Builder b) {
    Bitmap mLargeIcon;
    int mPriority;
    Style mStyle;
+    NotificationChannel mNotificationChannel;


Sorry man, I just noticed this... I am not sure if this might throw at runtime on older APIs, even if not used. Could we just store String mNotificationChannelId here?

You are right again, this will crash. Sending just the Id is better.

natario1 · 2017-08-21T09:55:27Z

build.gradle

@@ -22,11 +22,11 @@ allprojects {
 }

 ext {
-    compileSdkVersion = 25
+    compileSdkVersion = 26
    buildToolsVersion = "25.0.3"


As before, shouldn't we build with something starting with 26? Here and in travis file... No?

I didn't want to change the build tools since I don't know what the routines are here on that, but I can change to 26.0.1

I could also upgrade the support library to 26.0.1, but then I'd have to raise the minSDK to 14, since they removed support for <14

natario1

Looks good to me!

natario1 · 2017-08-21T10:59:48Z

Any insight as to why tests are failing?

iernie · 2017-08-21T11:01:56Z

Ahh! Robolectric does not support API level 26.

iernie · 2017-08-21T11:39:05Z

The last 4 failing tests are because metaData is null for some reason in those tests after upgrading to robolectric 3.4.2. Any pointers on how to solve this are welcome as I am running out of ideas.

iernie · 2017-08-21T20:44:03Z

I've tried everything I can think of to get the tests to work with robolectric 3.4.2 and API 26, but I don't feel like I have enough experience with the testing framework or robolectric to get this to work, so any help is appreciated here.

Now that Android Oreo is launched, it would be nice to get this merged :)

Jawnnypoo · 2017-08-21T20:47:00Z

No tests were written to test out the Android O based functionality, so I would opt to just keep Robolectric on the version it is on for now.

iernie · 2017-08-21T21:14:44Z

The problem is that if the compileSDK isn't 26 then the code fails to compile as NotificationChannel doesn't exist. When compileSDK is 26 then robolectric 3.3 fails because it doesn't support API 26. And with robolectric 3.4.2 I haven't been able to get the last 4 tests running.

natario1 · 2017-08-22T10:32:25Z

This is not going to fix anything, but you should also update the ROBOLECTRIC_SDK constant in the TestHelper class to 26. Not every test class is referencing it by the way.

codecov · 2017-08-22T19:38:20Z

Codecov Report

Merging #713 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##             master    #713      +/-   ##
===========================================
- Coverage     53.06%     53%   -0.07%     
  Complexity     1732    1732              
===========================================
  Files           132     132              
  Lines         10254   10266      +12     
  Branches       1432    1434       +2     
===========================================
  Hits           5441    5441              
- Misses         4364    4376      +12     
  Partials        449     449

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/com/parse/ParseNotificationManager.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...se/src/main/java/com/parse/NotificationCompat.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ain/java/com/parse/ParsePushBroadcastReceiver.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e637667...e808ab9. Read the comment docs.

iernie · 2017-08-22T19:39:56Z

What... I swear after testing all kinds of combinations of configs and versions, I can't believe that this finally went through. Tests still run on API 25 and robolectric 3.3.2 which is the last 3.3.x version that supports API 26.

natario1 · 2017-08-22T19:42:51Z

Nice catch. Going to merge, thanks @iernie for bringing up this issue!

iernie · 2017-08-22T19:44:16Z

@natario1 Happy to help! :)

Jawnnypoo · 2017-08-22T20:06:09Z

Eventually I think it would be great to move away from testing with Robolectric and just running tests on the emulator. Robolectric causes lots of headaches with how it breaks things. But thanks for suffering through it, @iernie !

iernie · 2017-08-22T20:10:38Z

Haha, yes I think I was on the brink of replacing the whole test framework with something else to get it to work. I found the documentation of Robolectric lacking. There has to be a better way like you said to just test on the emulator.

iernie · 2017-08-24T16:23:46Z

@natario1 Just a small thing, but I think you chose the wrong PR in the release notes :)

natario1 · 2017-08-24T16:28:13Z

You are right, will fix :-)

iernie · 2017-08-24T16:31:11Z

Thanks 😄

iernie added 3 commits August 19, 2017 13:40

Add Notification Channels configuration for Android O

de45313

Revert build tools version

9b3a7cf

Update travis.yml to android-26

876d5d9

natario1 reviewed Aug 19, 2017

View reviewed changes

Switch to making a notification channel instead based on feedback

20aec3f

iernie added 2 commits August 20, 2017 14:57

Merge branch 'master' into notificationchannels

ca9b340

Add comment on recreating notification channels

8f5a349

Only call getNotificationChannel on SDK >= 26

b33d776

natario1 reviewed Aug 21, 2017

View reviewed changes

Extract createNotificationChannel to own method and call it from getN…

e151f30

…otification

natario1 reviewed Aug 21, 2017

View reviewed changes

iernie added 2 commits August 21, 2017 12:36

Update to build-tools 26.0.1

49e3003

Save channelId instead of whole channel in NotificationCompat

d62981c

natario1 approved these changes Aug 21, 2017

View reviewed changes

Update robolectric to support API 26

4aa9252

Downgrade to robolectric 3.3.2

e808ab9

natario1 merged commit 4803953 into parse-community:master Aug 22, 2017

iernie deleted the notificationchannels branch August 22, 2017 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Notification Channels configuration for Android O #713

Add Notification Channels configuration for Android O #713

iernie commented Aug 19, 2017

natario1 Aug 19, 2017 •

edited

Loading

iernie commented Aug 19, 2017

natario1 commented Aug 20, 2017

iernie commented Aug 20, 2017

natario1 commented Aug 21, 2017 •

edited

Loading

natario1 commented Aug 21, 2017 •

edited

Loading

iernie commented Aug 21, 2017

iernie commented Aug 21, 2017

iernie commented Aug 21, 2017

natario1 Aug 21, 2017

iernie Aug 21, 2017

iernie commented Aug 21, 2017

natario1 Aug 21, 2017

iernie Aug 21, 2017

natario1 Aug 21, 2017

iernie Aug 21, 2017

natario1 left a comment

natario1 commented Aug 21, 2017

iernie commented Aug 21, 2017

iernie commented Aug 21, 2017

iernie commented Aug 21, 2017

Jawnnypoo commented Aug 21, 2017

iernie commented Aug 21, 2017

natario1 commented Aug 22, 2017

codecov bot commented Aug 22, 2017

iernie commented Aug 22, 2017

natario1 commented Aug 22, 2017

iernie commented Aug 22, 2017

Jawnnypoo commented Aug 22, 2017

iernie commented Aug 22, 2017

iernie commented Aug 24, 2017

natario1 commented Aug 24, 2017

iernie commented Aug 24, 2017

Add Notification Channels configuration for Android O #713

Add Notification Channels configuration for Android O #713

Conversation

iernie commented Aug 19, 2017

natario1 Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

iernie commented Aug 19, 2017

natario1 commented Aug 20, 2017

iernie commented Aug 20, 2017

natario1 commented Aug 21, 2017 • edited Loading

natario1 commented Aug 21, 2017 • edited Loading

iernie commented Aug 21, 2017

iernie commented Aug 21, 2017

iernie commented Aug 21, 2017

natario1 Aug 21, 2017

Choose a reason for hiding this comment

iernie Aug 21, 2017

Choose a reason for hiding this comment

iernie commented Aug 21, 2017

natario1 Aug 21, 2017

Choose a reason for hiding this comment

iernie Aug 21, 2017

Choose a reason for hiding this comment

natario1 Aug 21, 2017

Choose a reason for hiding this comment

iernie Aug 21, 2017

Choose a reason for hiding this comment

natario1 left a comment

Choose a reason for hiding this comment

natario1 commented Aug 21, 2017

iernie commented Aug 21, 2017

iernie commented Aug 21, 2017

iernie commented Aug 21, 2017

Jawnnypoo commented Aug 21, 2017

iernie commented Aug 21, 2017

natario1 commented Aug 22, 2017

codecov bot commented Aug 22, 2017

Codecov Report

iernie commented Aug 22, 2017

natario1 commented Aug 22, 2017

iernie commented Aug 22, 2017

Jawnnypoo commented Aug 22, 2017

iernie commented Aug 22, 2017

iernie commented Aug 24, 2017

natario1 commented Aug 24, 2017

iernie commented Aug 24, 2017

natario1 Aug 19, 2017 •

edited

Loading

natario1 commented Aug 21, 2017 •

edited

Loading

natario1 commented Aug 21, 2017 •

edited

Loading