Skip to content
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

Add foreground service with notification for the current sending and receiving state #15

Merged
merged 2 commits into from
Dec 3, 2020

Conversation

BelchingJalapeno
Copy link
Contributor

This implements #12.

This uses the same sender/receiver thread code from the MainActivity with some slight modification. android:launchMode="singleTop" is so when tapping the notification body it opens the current activity instead of creating a new
one.

This doesn't solve #1. I think the receiver thread gets stuck at the native Roc Receiver.close(long receiverPtr) function. I'm not sure what causes this.

@ortex
Copy link
Member

ortex commented Oct 18, 2020

Hi, thanks for PR!
I'll try to review more precisely in the upcoming days. See a couple of preliminary comments.

  • I guess there are should be different buttons for stop receiver and stop sender
  • Maybe also show notification on the locked screen? like Spotify do e.g.?

P.s: You're right, receiver thread stuck on closing

@BelchingJalapeno
Copy link
Contributor Author

I didn't even think about having a separate button for stopping each. I'm not sure what icons to use for each action. They could both use the stop icon for now since I think the icons are only used for Android wear and media style notifications.

It doesn't look like there is a way to get the default notification actions to work on the lock screen. For some reason you can drag the notification down to show the action buttons but it always collapses. The odd part is that if you tap quick enough you can tap the button and it will work as it collapses. It even does this for built in apps like the clock/timer. Kinda odd behavior.

I've got a few ideas for how to get a notification on the lock screen that can stop the sender or receiver.

  • Spotify probably uses a notification with the Notification.MediaStyle which would be easy to do. The main problem with this is it requires actions to use icons instead of text and I'm not sure what kind of icons would make it clear that it stops the receiver and another that stops the sender.

  • A custom layout can be used but would lose the default notification header and might look weird unless it's recreated or a Notification.DecoratedCustomViewStyle is used but then I think you need to fit everything in the size of a non expanded notification and it may get a bit cramped from what I've seen testing it so far.

  • The regular notification could be used and a custom layout could be used just for the lock screen notification but may have the same issues as the previous idea.

I wish there was an easy way to get it to work on the lock screen as it is but that's all I've come up with so far.

@gavv
Copy link
Member

gavv commented Dec 2, 2020

Thanks for PR! Sorry for delay, now looking into it.

Just tested the branch, it works great for me.

For some reason you can drag the notification down to show the action buttons but it always collapses.

I also see this behavior, BUT: if it drag it a bit further to bottom, it finally uncollapses :) I assume this is intended behavior, since it works the same way for all apps. I'm on Android 10, Xiaomi.

Here is how it looks like: https://imgur.com/a/AB28pgI

I've got a few ideas for how to get a notification on the lock screen that can stop the sender or receiver.

I think the current behavior is well enough to merge it (I'll review the implementation soon). As for the provided ideas, I think (1) and (2) make sense, we can try them after merging this.

@gavv
Copy link
Member

gavv commented Dec 2, 2020

Another possible improvement is to allow sender & receiver (and service with its notification) running even when the app is closed.

Comment on lines 82 to -71
fun startStopReceiver(@Suppress("UNUSED_PARAMETER") view: View) {
if (receiverThread?.isAlive == true) {
receiverThread?.interrupt()
if (senderReceiverService.isReceiverAlive()) {
senderReceiverService.stopReceiver()
} else {
startReceiver()
senderReceiverService.startReceiver()
}
setReceiverButtonState()
}
Copy link
Member

@gavv gavv Dec 3, 2020

Choose a reason for hiding this comment

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

Just noticed one potential race in startStopReceiver and startStopSender.

    fun startStopReceiver(@Suppress("UNUSED_PARAMETER") view: View) {
        if (senderReceiverService.isReceiverAlive()) {
            senderReceiverService.stopReceiver()
        } else {
            senderReceiverService.startReceiver()
        }
    }

Assume that the user presses STOP, startStopReceiver() is invoked, and it calls stopReceiver(). Then receiver thread receives interrupt exception and invokes senderChanged(false). UI thread handles it and renames button from STOP to START. Then user presses START button and startStopReceiver() is invoked again.

The code above expects that in this situation isReceiverAlive() will return false, so that we will call startReceiver().

However, it's possible that the receiver thread already called senderChanged(), but didn't finish yet, and isReceiverAlive() will report true. In result, the user presses START, but we call stopReceiver() and don't try to start anything.

As far as I can tell, the race was present before this PR too. I'll create a new issue for it.

@gavv
Copy link
Member

gavv commented Dec 3, 2020

I've created three new issues according to my comments above:

The PR itself looks good to me! @ortex Can we merge it?

@gavv
Copy link
Member

gavv commented Dec 3, 2020

This doesn't solve #1. I think the receiver thread gets stuck at the native Roc Receiver.close(long receiverPtr) function. I'm not sure what causes this.

Added this to #1, I'll take a look.

@gavv gavv self-requested a review December 3, 2020 09:32
@ortex ortex merged commit 17d8238 into roc-streaming:master Dec 3, 2020
@gavv gavv mentioned this pull request Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants