Skip to content

Conversation

@Zayankovsky
Copy link
Contributor

@Zayankovsky Zayankovsky commented Nov 24, 2021

Description

With Android 12 we can no longer customize the whole notification area: https://developer.android.com/about/versions/12/behavior-changes-12#custom-notifications. But we can use setColorized to apply background color to the rest on the notification.
Also with Android 12 it is no longer possible to set only those attributes to RemoteViews that changed since the last notification. Since we are creating a new RemoteViews instance every time, we have to set all attributes.
I would appreciate if someone tested this in long trips, as I'm worried about possible performance degradation.

Changelog

<changelog>Fixed notification appearance on Android 12</changelog>

Screenshots or Gifs

3 4

@Zayankovsky Zayankovsky added the bug Defect to be fixed. label Nov 24, 2021
@Zayankovsky Zayankovsky self-assigned this Nov 24, 2021
@Zayankovsky Zayankovsky requested a review from a team as a code owner November 24, 2021 21:42
@Zayankovsky Zayankovsky force-pushed the vz-notification branch 3 times, most recently from 4032b68 to 4de858f Compare November 25, 2021 12:27
Copy link

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

Since we are creating a new RemoteViews instance every time, we have to set all attributes.

Do we need to do that? is there possibly a better way to recycle the bitmaps/actions instead of rebuilding the whole view?

@Zayankovsky
Copy link
Contributor Author

Zayankovsky commented Nov 30, 2021

Since we are creating a new RemoteViews instance every time, we have to set all attributes.

Do we need to do that? is there possibly a better way to recycle the bitmaps/actions instead of rebuilding the whole view?

We actually still cache bitmaps and other stuff, e.g. there is currentManeuverImage in MapboxTripNotification. So we don't redraw the same bitmap over and over.
To be honest I don't understand why this trick of setting only the data that changed works on older Android versions. Since we create a new RemoteViews instance every time, I would expect Android to forget attributes we set for previous RemoteViews. And I haven't seen anywhere in documentation that those attributes are retained.

@Guardiola31337
Copy link
Contributor

To be honest I don't understand why this trick of setting only the data that changed works on older Android versions. Since we create a new RemoteViews instance every time, I would expect Android to forget attributes we set for previous RemoteViews. And I haven't seen anywhere in documentation that those attributes are retained.

Noting that this implementation was added in an attempt to workaround #1188 #1188 (comment) Apparently it's a downstream issue in the RemoteViews, maybe fixed by the latest versions? 🤔

@Guardiola31337
Copy link
Contributor

Guardiola31337 commented Nov 30, 2021

Took this for a quick spin and so far it's looking good.

I'm currently testing 👇

I would appreciate if someone tested this in long trips, as I'm worried about possible performance degradation.

With Android 12 we can no longer customize the whole notification area: https://developer.android.com/about/versions/12/behavior-changes-12#custom-notifications.

What can we do exactly? I'd love to hear from @avi-c @d-prukop as to how we can improve it from a design standpoint. I personally see the roundabout icons too small now 👀

Also wondering if we can move everything to the left and make it a big bigger (it feels we're not taking advantage of the whole notification area) 👀

In any case, this shouldn't block the PR. We can follow up later.

cc @abhishek1508

@Guardiola31337
Copy link
Contributor

Guardiola31337 commented Nov 30, 2021

Also noting that I had to bump LeakCanary to 2.7 (and set JDK version to 11+) to workaround the following install error:

11/30 12:57:48: Launching 'examples' on Google Pixel 3.
Installation did not succeed.
The application could not be installed: INSTALL_PARSE_FAILED_MANIFEST_MALFORMED

List of apks:
[0] '/Users/pabloguardiola/mapbox/mapbox-navigation-android/examples/build/outputs/apk/debug/examples-debug.apk'
Installation failed due to: 'Failed to commit install session 656700 with command cmd package install-commit 656700. Error: INSTALL_PARSE_FAILED_MANIFEST_MALFORMED: Failed parse during installPackageLI: /data/app/vmdl656700.tmp/base.apk (at Binary XML file line #241): leakcanary.internal.activity.LeakLauncherActivity: Targeting S+ (version 31 and above) requires that an explicit value for android:exported be defined when intent filters are present'
Retry

@Zayankovsky
Copy link
Contributor Author

Also wondering if we can move everything to the left and make it a big bigger (it feels we're not taking advantage of the whole notification area) 👀

I reduced paddings for Android 12, since the system already offsets our content.

@Zayankovsky
Copy link
Contributor Author

Also noting that I had to bump LeakCanary to 2.7 (and set JDK version to 11+) to workaround the following install error:

11/30 12:57:48: Launching 'examples' on Google Pixel 3.
Installation did not succeed.
The application could not be installed: INSTALL_PARSE_FAILED_MANIFEST_MALFORMED

List of apks:
[0] '/Users/pabloguardiola/mapbox/mapbox-navigation-android/examples/build/outputs/apk/debug/examples-debug.apk'
Installation failed due to: 'Failed to commit install session 656700 with command cmd package install-commit 656700. Error: INSTALL_PARSE_FAILED_MANIFEST_MALFORMED: Failed parse during installPackageLI: /data/app/vmdl656700.tmp/base.apk (at Binary XML file line #241): leakcanary.internal.activity.LeakLauncherActivity: Targeting S+ (version 31 and above) requires that an explicit value for android:exported be defined when intent filters are present'
Retry

I removed targetSdkVersion bump for now. You can bump it locally, if you want to test the notification on Android 12.

Copy link

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

I tested on Android 11 and the performance looks acceptable to me.

I replayed a very long route with 10x speed and memory didn't seem to grow and the update time of the notification was taking on average ~4-5ms on my device and is on par with main. Admittedly, this is still a lot of time, especially if we consider 10Hz location signal and effectively a notification update every 100ms. In this case we're looking at ~5% of time of the main thread spent just at updating the notification. However, most of that is spent in the Android SDK's notify which we have no control over.

This is where efforts like #5172 might be very important to limit the updates to the notification that are not essential.

@Zayankovsky Zayankovsky merged commit 23c487e into main Dec 2, 2021
@Zayankovsky Zayankovsky deleted the vz-notification branch December 2, 2021 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Defect to be fixed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants