-
Notifications
You must be signed in to change notification settings - Fork 441
notif: On Android handle notification taps via Pigeon API #2043
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
Changes from all commits
86f3f98
3b399c1
18ec4ae
cb9d8ed
5a5db3c
44042f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package com.zulip.flutter.notifications | ||
|
|
||
| import android.content.Intent | ||
| import android.net.Uri | ||
|
|
||
| class NotificationTapEventListener : NotificationTapEventsStreamHandler() { | ||
| private var eventSink: PigeonEventSink<NotificationTapEvent>? = null | ||
| private val buffer = mutableListOf<NotificationTapEvent>() | ||
|
|
||
| override fun onListen(p0: Any?, sink: PigeonEventSink<NotificationTapEvent>) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it important here (and elsewhere in this file) that we use the platform-agnostic base class
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, since these are overriden functions we can't change their signatures.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably it'd be good to fully implement the protocol, even though we don't currently intend to use this feature: override That way there isn't a latent bug waiting for us in case we someday do start using that feature of the protocol. |
||
| eventSink = sink | ||
| if (buffer.isNotEmpty()) { | ||
| buffer.forEach { sink.success(it) } | ||
| buffer.clear() | ||
| } | ||
| } | ||
|
|
||
| override fun onCancel(p0: Any?) { | ||
| if (eventSink != null) { | ||
| eventSink!!.endOfStream() | ||
| eventSink = null | ||
| } | ||
| } | ||
|
|
||
| private fun onNotificationTapEvent(dataUrl: Uri) { | ||
| val event = AndroidNotificationTapEvent(dataUrl.toString()) | ||
| if (eventSink != null) { | ||
| eventSink!!.success(event) | ||
| } else { | ||
| buffer.add(event) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Recognize if the ACTION_VIEW intent came from tapping a notification; handle it if so. | ||
| * | ||
| * If the intent is recognized, sends a notification tap event via | ||
| * the Pigeon event stream to the Dart layer and returns true. | ||
| * Else does nothing and returns false. | ||
| * | ||
| * Do not call if `intent.action` is not ACTION_VIEW. | ||
| */ | ||
| fun maybeHandleViewIntent(intent: Intent): Boolean { | ||
| assert(intent.action == Intent.ACTION_VIEW) | ||
|
|
||
| val url = intent.data | ||
| if (url?.scheme == "zulip" && url.authority == "notification") { | ||
| onNotificationTapEvent(url) | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } | ||
| } | ||
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.
The notification taps come to us as Android Intents, right, so I'm curious why our existing class named
AndroidIntentEventListenerisn't involved at all in this work. Is it helpful or necessary to create this whole separate class? (Maybe it has something to do with the Pigeon limitations you mentioned at #2043 (comment) ?) Or would the new intent-handling code be better organized if we just put it onAndroidIntentEventListener?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.
IIRC the problem was that the implementation of
AndroidIntentEventListenerwould need to accommodate for multiple listeners (handling multiple incomingonListencalls), becausestream.listen()would be called at two places.The implementation would then need to handle buffered events specially, basically keep filling the buffer until there are two incoming
onListencalls, replay the events to the listener on each call, and then clear the buffer.AndroidIntentEventListenerwould also need to ensure that there can be only twostream.listen()on the Dart side.All this seemed like unnecessary complexity, and also a nice self-contained
notifications.dartPigeon API with types like cross-platform variants ofNotificationTapEventseemed like a better approach.