Replace notifications library with using platform APIs directly, on Android

[kunall17]

Fixes #2877

The last part of refreshing the ID's is still going on

Thanks for the instructions in the issues @gnprice

[gnprice]

Thanks @kunall17 ! Glad the instructions were helpful, too.

Initial comments:

• In "android notif: Simplify logic for passing clear action", you're also editing this ProxyService logic:

     protected void onHandleIntent(Intent intent) {
         Log.d(TAG, "New intent: " + intent);
-        final Bundle notificationData = NotificationIntentAdapter.extractPendingNotificationDataFromIntent(intent);
+        Bundle notificationData = new Bundle();
+        notificationData.putString(NOTIFICATIONS_ACTION_KEY, intent.getAction());
         final IPushNotification pushNotification = PushNotification.get(this, notificationData);

That logic has a very different job -- when the user presses a notification (for the main/"content" action, not our extra "clear" action), this is what receives that intent so we can pass it on to the JS code. That's what the job of ProxyService is in the library.

As a result, if you run this commit and try receiving a notification and pressing it, I expect it won't work.

I want to cut out that logic too (that's step 3 in the issue), but it should be in a separate commit, and of course each commit should leave things in a working state.

• I think we actually don't have any need to modify the library's proxy, until we just replace it completely in step 3. For step 1, we just need to stop using it for this unrelated job (the dismissIntent) that isn't its intended job.

• Commit message style: the first line is a single line, <75ch, that stands on its own as a summary (like an email subject line). Then a blank line, then paragraphs of text.

• Hmm, did the idea of doing one tools:node="replace" on the manifest element not work?

• If we do have to do each piece separately: are these everything in the library, or is there something left over? If it's everything, should assert that in the commit message as that's helpful for the reader. If something left, definitely mention/explain. :-)

• What's this diff about? I don't see it explained in the commit message (and it seems like an independent change anyway.)

-                <category android:name="com.zulipmobile" />
+                <category android:name="${applicationId}" />

• android notif: Implement GcmMessageHandlerService -- This looks like it's copied from the library. I'd much rather copy from upstream docs; easier to convince ourselves those are good.

  Just follow upstream GCM docs; this for the manifest blob, and this for the service implementation.

[gnprice]

Before getting into the ID refresh, let's focus on steps 1-4. The first two bullets, and the last bullet, in my comment above are the major ones.

[kunall17]

As a result, if you run this commit and try receiving a notification and pressing it, I expect it won't work.

Yeah, I've changed upon that.

Commit message style: the first line is a single line, <75ch, that stands on its own as a summary (like an email subject line). Then a blank line, then paragraphs of text.

Done that

Hmm, did the idea of doing one tools:node="replace" on the manifest element not work?

No the tools:selector don't work in manifest and application tag, I tried it out.

For this one

-                <category android:name="com.zulipmobile" />
+                <category android:name="${applicationId}" />

I have read for the GcmReceive, the category name of the intent filter should be same as the package name. But I tested it out for both of the package name's for the category name and it worked.

I'll make this commit seperate and add links for this.

If we do have to do each piece separately: are these everything in the library, or is there something left over? If it's everything, should assert that in the commit message as that's helpful for the reader. If something left, definitely mention/explain. :-)

The CLEAR action works in all the commits, and I'll test the other narrowing thing as well.

[kunall17]

Updated the branch @gnprice , tested both the clear action button and the narrowing to a message on dead and running state, both are working.

bc6eb98 Replaces the hardcoded value of category name in the filter, but it isn't required as this was only for the projects having minSdkVersion lower than 16. So this commit can be omitted.

[gnprice]

Thanks again @kunall17 !

• About the first couple of commits: rereading what I wrote above, I see I didn't say this clearly -- I'd rather skip copy-pasting the library's ProxyService. Instead,
  • First, write our own tiny service with a bit of code that just handles the CLEAR case. Send CLEAR actions there, with the nice new cleaner code like the second commit of this branch.
  • Then later (in step 3), add our own bit of code to construct a PendingIntent for setContentIntent, and add to our service our own code to handle those intents.
  • At each step, this means we get to follow the upstream documentation and our own sense of good design, rather than follow whatever the library did.
• Specifically, I think it'd be helpful to try to not copy-paste any library code at any step of the branch. Not because we absolutely can't copy code (it has a fine license), but I think that'll help direct the approach.
  • One exception is the manifest, where it may be convenient to copy stuff just to point explicitly to the library code before removing those items one by one.
  • If there are other places you think it'd really be much better to copy the library code, go ahead but explain why :-)
• I like how much simpler the code gets with the new CLEAR logic in the second commit! 🎉
• Why does conversations become static where it wasn't before? That sounds like a subtle point, and I don't really know which is right or why -- it'd be good to explain.
  • Would that one-word change actually be a good fix to make right at the beginning of the branch, and merge now even before other changes are ready?

(cont'd)

[gnprice]

As stated in google docs and Stackoverflow the category name of the
GCM reciever intent-filter should match with the package name
Link: https://stackoverflow.com/a/13445847/5026681

Hmm, interesting. Good find!

• A general point: if there are docs from upstream that say something, that's the best source to link to. StackOverflow is a great place to get ideas and to locate the right API or documentation, but then once you've found the thing, you go read its own documentation (if it has good documentation -- which Android generally does, fortunately) to get more reliable answers.

• That SO answer's quoted text includes this:

  and the category tag is not required for applications targeted to minSdkVersion 16 and higher

  That's us! So I guess we should just drop this tag completely, if that's right.

  Check the actual documentation to be sure, though. 😉

This is also an example of the kind of change I'd love to merge right up front, while the rest of the PR might still be in progress.

(still reading)

[gnprice]

• For "Implement GCM service to process the GCM messages" -- this is another spot where I'd like to try this approach I mentioned just above:

  it'd be helpful to try to not copy-paste any library code at any step of the branch. [...] I think that'll help direct the approach.

  • For a first commit, maybe have a very simple ZulipGcmListenerService that inlines the body of getPushNotification, and delete that method. That makes the first commit (which is copying upstream's examples) short, just like your first commit for this in the current version of the branch.
  • Then refactor further, like you do here.

• Also, do we still need an element with tools:node="remove" or something to avoid getting the library's manifest fragment merged in?

• In refactoring the old GCMPushNotifications class: this commit moves a lot of code from one file to another, which makes a big diff, but then it also has some non-mechanical / interesting changes.

  • It's hard to carefully read the interesting changes when they're camouflaged by big mechanical changes.
  • When you want to move a bunch of code, or do another big mechanical change that git diff / git log --stat -p don't understand how to summarize, isolate that one change with the bare minimum of other stuff to make things keep working (like editing call sites, import lines, etc.). Then make the more interesting changes in separate commits before and after.

• Good thought re: NotificationCompat! Using the compat libraries is usually a good idea on Android in general, for things where there is one.

  • git grep -w Notification finds a couple more places left. Are there compat equivalents for those?
  • In this instance, I'm a bit confused that the setSound seems to have less in the API than the newer platform API has. Do you know what's up there?

[kunall17]

@gnprice Thanks for the comments above, I've updated the PR, and this has become a huge PR now :)
I've done till removing the notifications library and I think let's not implement any more features to this one.

[kunall17]

About the first couple of commits: rereading what I wrote above, I see I didn't say this clearly -- I'd rather skip copy-pasting the library's ProxyService. Instead,

I've re written the proxy service as ClearNotificationHandlerService which handles only the clear action in the first commit, the second commit clears the logic which is implemented in the MainApplication and moves everything to IntentService.

Specifically, I think it'd be helpful to try to not copy-paste any library code at any step of the branch. Not because we absolutely can't copy code (it has a fine license), but I think that'll help direct the approach.

I've re written most of the parts, but someplaces it can be the same,

Why does conversations become static where it wasn't before? That sounds like a subtle point, and I don't really know which is right or why -- it'd be good to explain.
Would that one-word change actually be a good fix to make right at the beginning of the branch, and merge now even before other changes are ready?

Static because this should be a singleton object and there should be only 1 of this in the whole application.
Also its easy to manipulate with static objects as they can be accessed by different classes.

Would that one-word change actually be a good fix to make right at the beginning of the branch, and merge now even before other changes are ready?

Yeah, that could be another commit.

[kunall17]

In refactoring the old GCMPushNotifications class: this commit moves a lot of code from one file to another, which makes a big diff, but then it also has some non-mechanical / interesting changes.

While moving the code I did like very negligible changes and in the further commits there are changes like the sound and the notification compat changes

In this instance, I'm a bit confused that the setSound seems to have less in the API than the newer platform API has. Do you know what's up there?

Yes there were some deprecated functions before so I had to use the if condition, in the compat library there is a function which supports all the library and I suspect it's doing the same thing.
I think we should change the notification sound, as it's more like in app sound of message recieved.

[gnprice]

Thanks @kunall17 for your work on this!

Looking at the latest version of this branch, and comparing with the thread above: I think this isn't converging toward a PR we'll be able to merge. We'll certainly want to do #2877; we'll approach it at some point with a fresh start.

[kunall17]

Thanks for the review @gnprice, if you're free some time, could we discuss how can it be improved as I'm interested in taking a fresh start on this PR?