Update notification and permission handling #274
Conversation
zsmb13
force-pushed
the
feature/permissions
branch
from
ee7c7d3 to
216d3fa
Compare
zsmb13
changed the base branch from
kotlinx-datetime
to
feature/notifications
zsmb13
force-pushed
the
feature/permissions
branch
2 times, most recently
from
2cdeaa3 to
eeda6bb
Compare
zsmb13
force-pushed
the
feature/permissions
branch
2 times, most recently
from
e5f29ea to
709d0de
Compare
zsmb13
force-pushed
the
feature/permissions
branch
9 times, most recently
from
9d0fec6 to
be9d82f
Compare
| } | ||
| val request = UNNotificationRequest.requestWithIdentifier(notificationId, content, trigger) | ||
| notificationCenter.addNotificationRequest(request) { error: NSError? -> | ||
| // TODO use logger |
zsmb13
changed the title
| if (items.isNotEmpty() && firstLiveIndex != -1) { | ||
| LaunchedEffect(Unit) { | ||
| listState.scrollToItem(firstLiveIndex) | ||
| } | ||
| } |
There was a problem hiding this comment.
Quick random fix: scroll to current time in schedule when first loaded
zsmb13
force-pushed
the
feature/permissions
branch
from
be9d82f to
56453ad
Compare
| message: String | ||
| ): Unit = | ||
| js( | ||
| """{ |
There was a problem hiding this comment.
Could we make external declarations for this instead or check if they are in the browser dependency? I really want to avoid string js code
There was a problem hiding this comment.
I'll see if this can be moved to the JS side easily, but this is something I moved over from the old implementation as-is. I'd also be happy to just leave it and not touch it, since it works :D
|
|
||
| @OptIn(ExperimentalComposeUiApi::class) | ||
| fun main() { | ||
| initCoil() | ||
| CanvasBasedWindow { | ||
| App(ApplicationContext()) | ||
| App(platformModule = module { |
There was a problem hiding this comment.
we may have last block parameter outside of the (), so it can be a bit more readable
There was a problem hiding this comment.
Doesn't work here as we're also calling the module function, but I'd also avoid it either way because it would look confusing in Compose code
| suspend fun requestPermission(): Boolean | ||
|
|
||
| fun post( | ||
| notificationId: String, |
There was a problem hiding this comment.
It would be nice to use value class NotificationId here, similar to other places.
There was a problem hiding this comment.
We need to serialize it into an intent at some point on Android, and pass it to JS for interop, I don't want to complicate its type for now
| private const val EXTRA_MESSAGE = "message" | ||
| private const val EXTRA_NOTIFICATION_ID = "notificationId" | ||
| private const val EXTRA_ICON_ID = "iconId" | ||
| private const val NOTIFICATION_CHANNEL_ID = "channel_all_notifications" |
There was a problem hiding this comment.
Should we introduce separate channels for different notification types (session alerts vs news)?
There was a problem hiding this comment.
Not without putting in a lot more work for managing these channels. The ideal setup for Android users would probably be to use 3 different notification channels and not have in-app settings for notification categories at all, only a button that sends them to the system settings where they can configure each channel. But it's a bunch of extra work just for the one platform, so not critical for the time being.
(We can't read or change the state of notification channels from the app, so we can't sync our own toggles to what the user sets on the channels in settings.)
There was a problem hiding this comment.
On the bright side, we'll bump the minimum API to 26 on the icons PR, which is where notification channels were introduced, so that eliminates a whole problem where on 24-25 we'd have to have the in-app switcher instead anyway.
zsmb13
force-pushed
the
feature/permissions
branch
from
0a80097 to
818db79
Compare
Updates the notification system (and related permission handling) to some new APIs, still implemented with separate native APIs on each platform, as no suitable KMP library was found that covered our use cases (including scheduled notifications).
Makes notification handling depend on
TimeProvideras well, allowing tests of notifications with fake time values and sped up clocks within the app.