Fix secondary user SMS notification hiding sender and message#200
Conversation
| return | ||
| } | ||
|
|
||
| if (intent.action == Sms.Intents.SMS_RECEIVED_ACTION) { |
There was a problem hiding this comment.
android.provider.Telephony.MMS_DOWNLOADED is not handled there, so user will not receive a notification
There was a problem hiding this comment.
I checked and couldn't find this action broadcast anywhere. Messaging sends its own SendStatusReceiver.MMS_DOWNLOADED for MMS downloads, and incoming MMS comes through WAP_PUSH_DELIVER, so this branch never fired. @inthewaves, what do you think?
| ) { | ||
| // The contact lookup performs a synchronous query, so resolve off the main thread. | ||
| val pendingResult = goAsync() | ||
| entryPoint.applicationScope().launch(entryPoint.ioDispatcher()) { |
There was a problem hiding this comment.
Concurrent broadcasts can display an older SMS as the latest notification.
Every broadcast starts an independent Dispatchers.IO coroutine, so:
- SMS A starts a slow contacts lookup
- SMS B arrives later and resolves quickly
- B posts its notification
- A completes and overwrites B using the shared notification ID
| } | ||
|
|
||
| if (intent.action == Sms.Intents.SMS_RECEIVED_ACTION) { | ||
| handleSecondaryUserSmsReceived(context, intent, entryPoint(context)) |
There was a problem hiding this comment.
Would it be more reliable to handle it in a Service instead of short-living Broadcast, given that we're querying db with unpredictable delays?
There was a problem hiding this comment.
As far as I know the system gives a receiver ~10s, and a single contact lookup should finish well within that. Service/WorkManager feels like overkill here, and this worked fine before.
| ) | ||
| } | ||
|
|
||
| private fun entryPoint(context: Context): IncomingSmsEntryPoint { |
There was a problem hiding this comment.
Why not regular injection?
There was a problem hiding this comment.
The receiver is created by the system, not by Hilt, so constructor injection isn't possible. The usual alternative is @AndroidEntryPoint field injection, but that requires calling super.onReceive(), and it doesn't compile because BroadcastReceiver.onReceive is abstract. So the remaining option is EntryPointAccessors to reach the Hilt graph.
There was a problem hiding this comment.
AndroidEntryPoint
Yes, that's what I mean, not injecting the receiver itself, of course :)
but that requires calling super.onReceive()
Ah, okay
| import org.junit.Assert.assertNull | ||
| import org.junit.Test | ||
|
|
||
| internal class SecondaryUserMessageResolverTest { |
There was a problem hiding this comment.
Test coverage should be improved, currently tests are skipping lots of cases
7f1120c to
b1a4825
Compare
b1a4825 to
f9f3d8c
Compare
2 participants
Closes #75
Fixed: secondary users saw a generic "New message" instead of the sender and text. The notification now shows them directly.
Also: receivers moved to Kotlin, receive/parse/notify logic split into DI collaborators, phone number formatting consolidated.
How to check on device: