Notification.setLatestEventInfo() is removed in API Level 23

[barbeau]

master branch

If we update compileSdkVersion to 23, it no longer builds:

Error:(157, 21) error: cannot find symbol method setLatestEventInfo(Context,String,String,PendingIntent)

Notification.setLatestEventInfo() has been deprecated for a while, and has caused problems in the past (see #104).

We should finally remove it.

[erick03]

Did you find a solution for that?

[barbeau]

@erick03 If you mean a solution for being able to update compileSdkVersion to 23, we need to create and register a new notification instead of calling Notification.setLatestEventInfo().

From my conversation here with Google on the best practice for updating notifications:

Me:

To clarify the above, for people fixing old code - the current official best practice for Notifications is NOT to hold a reference and update them with new information, correct? Instead, to show new info to the user, you should always cancel the existing notification and build a new Notification via Notification.Builder?

The app I was working on had periodic notifications delivered to the user, and it used persisted notification IDs along with setLatestEventInfo() to update the information shown to the user within the same Notification. Based on my understanding of the above, the proper behavior would be to cancel and re-create the Notification using the Builder.

Google:

There is no need to cancel. Simply notify again with the same id and tag and the existing notification will be updated. The actual notification object is parceled when it is sent to the service, so it's not important to hold a reference to the local Notification object.

See packages/experimental/NotificationShowcase and look at the upload notification for an example.

So the solution is to implement the above, and test to make sure reminders still work correctly. It's on my TODO list, but I've been trying to focus on getting the UI redesign to the point of release. PR's accepted! :)

[barbeau]

We should also be using NotificationCompat.Builder:
http://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder.html

[erick03]

Great!

[barbeau]

Here's how notifications are currently used in OBA Android:

1. Pull up arrival times
2. Tap on an arrival time
3. Tap "Set a reminder", and save it

You should get a notification then based on the reminder you set up - for example, if you selected to be reminded 5 minutes before the Route 5 North arrives, when the Route 5 North prediction reaches 5 min you should get a notification.

[barbeau]

As far as unit-testing Notifications on Android - I've never set this up before, so I'm not sure the best way to approach it. Ideally it would involve actually registering a Notification and then retrieving it from the Android framework to confirm it was set up correctly, or somehow otherwise detecting it.

The following seems promising - http://stackoverflow.com/a/34467347/937715.

[charles-b-stb]

Ok, as far as my testing goes, the code I wrote using new notifications framework produces notifications properly. I just have to generate the test for it as required (I believe?) by the developer agreement.

Should I also bump the compile sdk version as part of this fix, or will that be handled elsewhere?

[barbeau]

Cool! Tests aren't required, but very much appreciated. They help us keep the code quality high.

Let's wait to bump compileSdkVersion, we can do that elsewhere.

[barbeau]

The closest existing test would probably be the provider test for the reminders (trip alerts) - https://github.com/OneBusAway/onebusaway-android/blob/master/onebusaway-android/src/androidTest/java/org/onebusaway/android/provider/test/TripAlertsTest.java

It just tests if the proper records are inserted in the provider, though, it doesn't touch the actual notifications.

@charles-b-stb For notification unit tests you might want to check out https://developer.android.com/reference/android/service/notification/NotificationListenerService.html - it seems like you might be able to set up a listener to confirm that the notifications are properly registered with the platform.

[charles-b-stb]

Sorry for being late on updating this. I realized while working on a test case that my first implementation was wrong as it left the service trying to manage references to notification objects. I will fix this before continuing to build a test case.

[barbeau]

@charles-b-stb No rush! I'd prefer a correct solution to a fast one, so take your time.

[charles-b-stb]

@barbeau I am still processing the needed changes, but this is beginning to look like a fairly big deal. From what I am seeing in the code, it looks like most if not all of the code in TripService.java is obsolete given the new architecture from Google. We should not be saving notifications anywhere at all.

I would just rewrite this whole process to rely on NotificationManager to handle notifications, but it appears there is some sort of contract with the service and external apps. Is this correct? If so I will need to implement while maintaining these action flags.

[barbeau]

We should not be saving notifications anywhere at all.

Yes, that's the best solution - TripService needs to be updated to the current best practices for Notifications. I haven't looked at that code in a while, so a fix may be more invasive than I recall.

I would just rewrite this whole process to rely on NotificationManager to handle notifications, but it appears there is some sort of contract with the service and external apps. Is this correct?

Yes, OBA was initially set up to be able to expose info to other apps (before my time), but to my knowledge this was never actually implemented by any other application. I started looking at this in #104 (comment) as part of troubleshooting #104, and found some potential problems with the implementation - see #104 (comment) and CUTR-at-USF@54b3e4c. I ended up backing away from some of these changes, as I found out these issues weren't the cause of #104 (the main cause was a bug in Android). So the above commit never made it into the master branch. You may be able to use that commit, though, in your current changes.

I'd like to keep the current contract with the service and external apps if at all possible, as long as it works with the update Notification best practices and we resolve any known issues.

[charles-b-stb]

I believe I have a viable fix here: https://github.com/charles-b-stb/onebusaway-android.git on the branch "issue290-NotificationManager". As far as my manual testing goes, this seems to be working, but I am still unclear on how to write a test case.

The sample you shared earlier relies on an existing test framework set up specifically for provider objects, and its not clear to me that this will work with Notifications. At least, I do not see a mechanism for mocking notifications in the android test toolkit.

Is it possible to write a test that directly interfaces with the notification manager, or does that need to be mocked somehow to make a test case work?

[barbeau]

Cool! As far as using native Android to implement the notifications test, I'd check out the NotificationListenerService:
https://developer.android.com/reference/android/service/notification/NotificationListenerService.html

In theory I think in a unit test you could set up the listener, register the notification, and make sure that your listener gets the same thing back that you registered.

Main catches, from a quick read:

• It's only API Level 18 or higher, so we'd only be able to run test on newer devices, but as long as you guard it with a check of Build.VERSION.SDK_INT I think this is ok.
• It requires additional permissions/entry in the manifest that we wouldn't want in production - I believe you should be able to solve this via Gradle build flavors, and specifying a debug specific version of the manifest containing only this service, which would get merged with the production manifest ONLY for tests/debug builds - see http://stackoverflow.com/a/25207176/937715 for details

[charles-b-stb]

Conceptually, I understand what needs to be done to see if this will work. I'm a bit more fuzzy on the details for modifying the gradle scripts for allowing a 2nd (test only) manifest and how the service will interact with tests.

Just to make sure I know what I'm doing here, are you implying that all debug versions of the build in onebusaway-android-onebusaway-android.iml should have their MANIFEST_FILE_RELATIVE_PATH changed to something like "/src/debug/AndroidManifest.xml" wherein a copy of the manifest referencing the notificaction service would reside?

I presume that subsequently I would create a subclass of the NotificationListenerService which would listen for a specific notification I would create in a separate AndroidTestCase object. At that point (since these are not directly communicating) how would I verify the test completed? Would it be sufficient for the listener to report somewhere how many notifications it witnessed, or is some sort of timeout required to present a fail condition?

[barbeau]

It's been a little while since I worked on the Gradle build variants, but IIRC you should be able to drop a new file AndroidManifest.xml into the src/androidTest folder that has only the following:

<manifest xmlns:android="http://schemas.android.com/apk/res/android"
          package="org.onebusaway.android">
    <application>
    <!-- Or whatever you end up calling it -->
    <service android:name=".NotificationListener"
          android:label="@string/service_name"
          android:permission="android.permission.BIND_NOTIFICATION_LISTENER_SERVICE">
         <intent-filter>
             <action android:name="android.service.notification.NotificationListenerService" />
         </intent-filter>
     </service>
    </application>
</manifest>

I believe this should automatically merge the above attributes with the main AndroidManifest.xml, but only for tests - this service wouldn't be part of the manifest if you launch a normal debug or release build.

You definitely shouldn't need to modify the .iml files, those are generated by Android Studio.

I presume that subsequently I would create a subclass of the NotificationListenerService which would listen for a specific notification I would create in a separate AndroidTestCase object. At that point (since these are not directly communicating) how would I verify the test completed? Would it be sufficient for the listener to report somewhere how many notifications it witnessed, or is some sort of timeout required to present a fail condition?

I'm not completely sure about this - it would require some testing. I'm not sure we have any other asynchronous tests in OBA Android. I think you're heading down the right path - you'd likely need to register the notification and then sleep for a second, and then check a class member variable to see if the subclass of NotificationListenerService received a callback indicating that the notification was correctly created. In the subclass of NotificationListenerService you'd set this class member variable to true, for example, when the listener fired with the correct notification content.

[charles-b-stb]

I am having issues getting the manifest version to cooperate. Apparently we need to be as high as version 23 in the manifest to use the NotificationListener. I've tried changing the version for the local manifest in the test folder, but it still apparently keeps referring to the version in the core manifest.

Do you know of any workarounds to get past this?

[barbeau]

@charles-b-stb Did you try bumping compileSdkVersion to 23 in build.gradle?

You'll need to sync Android Studio with build.gradle after this change. If you've gotten rid of Notification.setLatestEventInfo() I believe the project should build.

When you use NotificationListener, if you need API level 23 to execute the test, you'll need to surround it with:

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
    // Test here
}

[charles-b-stb]

The only way I could find to get rid of the compiler error was to set the test notification listener implementation (which has to be referenced from the test manifest) to have an annotated version of 23 (@TargetApi(23)) unfortunately I don't know of a way to certify that lower versions of android cannot access this class since its will be instantiated from the manifest directly. I suspect that this method of test generation may cause crashes.

If I am reading this correctly, since the class itself is instantiated from the manifest, it seems that surrounding the test code itself (which will only exercise our notification code) with a version check if statement will not prevent crashes.

Am I reading this correctly, or is there something I am missing?

I am beginning to wonder if this particular test is really feasible given android's current architecture.

[barbeau]

Yes, adding @TargetApi(23) to method is fine. This along with IF statement should be sufficient to run test only on M devices/emulators.

[barbeau]

@charles-b-stb If the test is proving difficult to implement, I'd be ok with you submitting a PR for the fix without it. For this particular case, I don't want perfect to be the enemy of good. This issue is blocking us bumping compileSdkVersion to 23, which needs to happen.