-
Notifications
You must be signed in to change notification settings - Fork 320
Rebuild RemoteViews for MapboxNavigationNotification on each update #1474
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.
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
Conversation
Guardiola31337
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment.
| PendingIntent pendingCloseIntent = createPendingCloseIntent(applicationContext); | ||
| expandedNotificationRemoteViews.setOnClickPendingIntent(R.id.endNavigationBtn, pendingCloseIntent); | ||
| pendingCloseIntent = createPendingCloseIntent(applicationContext); | ||
| buildRemoteViews(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're building the remote views on every notification update. Should this call be removed from here?
02a986a to
ede2671
Compare
|
@Guardiola31337 good catch, this is updated |
Guardiola31337
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's test if this workarounds the TransactionTooLargeException!
Thanks @danesfeder
|
I've tested OP and now I'm seeing when running a long session in a Pixel XL - Android 9 and After some digging 👀 https://stackoverflow.com/questions/11451393/what-to-do-on-transactiontoolargeexception and https://stackoverflow.com/questions/39098590/android-os-transactiontoolargeexception-on-nougat it seems the problem is not in the notification per se and could be caused somewhere else because of the amount of data that we store in I want to note that this exact same behavior was documented in the release notes for Nougat 👀
We need to keep 🕵️ so I'm going to reopen #1188 again 😥 cc @danesfeder |
|
From https://developer.android.com/reference/android/os/DeadSystemException.html
Are we sure that's related to these changes?
@Guardiola31337 for these sessions you're running, are you rotating the screen a lot? |
Yeah, could be definitely related, I've seen weird logs including
Nah, actually the opposite. I've launched a navigation session and left the device stuck at the dock for hours. No rotations, stuck at the same position / location all the time, no config changes. What are you thinking on? |
|
Forgot to mentioned that the device is plugged in, the |
Closes #1188
Unfortunately #1441 and #1455 didn't address the
TransactionTooLargeException.If the data is being built-up in the
RemoteView, then it may make sense to rebuild the views on each update? I'm concerned about performance here, as this could be costly to do every second. Should we be smarter about it? Maybe only do this after a certain amount of time has passed?I guess first we should test that it fixes it at all and then worry about performance implications.
cc @akitchen @taraniduncan