-
Notifications
You must be signed in to change notification settings - Fork 174
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
Added extended headset controls (headset button click handling) #1218
base: master
Are you sure you want to change the base?
Conversation
KeyEvent.KEYCODE_MEDIA_NEXT -> { | ||
clickCount += 2 | ||
Log.d(tag, "=== handleCallMediaButton: Media Next, clickCount=$clickCount") | ||
} | ||
|
||
KeyEvent.KEYCODE_MEDIA_PREVIOUS -> { | ||
clickCount += 3 | ||
Log.d(tag, "=== handleCallMediaButton: Media Previous, clickCount=$clickCount") | ||
} |
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.
I'm not understanding the logic here. How do we know that KEYCODE_MEDIA_NEXT/PREVIOUS are clicks and not a separate next/prev button press?
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.
@advplyr
Android is handling specific shortcuts not emitting exact events, but overriding them by combined events:
- One click: Play/Pause
- Double click: Next
- Triple click: Previous
- Long press: Voice over - not a media event, but an extra permission AND handler on Andr. > 11, see here or here
This cannot be prevented, because it is a system / kernel behaviour.
So double-clicking does NOT emit 2 raw click events, but 1 single (automatically rewritten) event KeyEvent.KEYCODE_MEDIA_NEXT
.
I handle KeyEvent.KEYCODE_MEDIA_NEXT
as if 2 single clicks had happened and increase the click count by 2. Same for the triple-click, which happens to be emitted as KeyEvent.KEYCODE_MEDIA_PREVIOUS
by Android instead of 3 single keydown/keyup event-combos.
This results in a more flexible API, since you can configure behaviour based on click count now and are not required to work around pre-defined behaviour. You can also see this in the how does it work
comment, the mentioned problems there are kind of fixed / unfixable - I tuned the timings as good as possible, but I need feedback from other device owners.
Best way would be to try it out (by using a headset) monitoring the logs. If you don't own the required hardware to test it yourself, let me know, maybe we can arrange something.
- Press down:
KEY_DOWN
- Wait:
// wait 1248ms
- Release:
KEY_UP
on Android 7.0 devices is
- Press down:
// nothing
- Wait:
// wait 1248ms
- Release:
KEY_DOWN
,KEY_UP
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.
My understanding is that these events are emitted by the bluetooth device and so depending on the bluetooth device it might emit a KEYCODE_MEDIA_NEXT on 2 clicks if it is setup for that.
Or you might have a bluetooth device that has separate buttons next to play/pause that is for KEYCODE_MEDIA_NEXT.
Did you find any documentation on these KeyEvents that say when/how they are triggered?
I think this needs to be tested on a bluetooth device that has NEXT/PREV media buttons. I'll see if I have one. I think many bluetooth in vehicles have this.
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.
My understanding is that these events are emitted by the bluetooth device and so depending on the bluetooth device it might emit a KEYCODE_MEDIA_NEXT on 2 clicks if it is setup for that.
@advplyr
Ah, now I understand. You mean a device like this - a car bluetooth receiver with multiple buttons (e.g. for play/pause
, next
and prev
).
Well, first things first: The PR is NON-DESTRUCTIVE, so that there are no behaviour changes, as long as the according setting (extended headset controls
) is disabled, which it is by default. So no fear of breaking anything, that works fine at the moment.
I own such a car bluetooth recevier with buttons for play/pause
, next
and previous
and tested it. It shows the following behaviour (which I would have expected):
play/pause
- single click = play/pause
- double click = next
- triple click = prev
- 4 clicks or more (fast execution) = nothing
- multiple clicks (slow execution) = multiple play / pause events
next
- single click = next
- 2 clicks or more (fast execution) = nothing (translated to 4 clicks or more)
- multiple clicks (slow execution) = multiple next events
prev
- single click = prev
- 2 clicks or more (fast execution) = nothing (translated to 6 clicks or more)
- multiple clicks (slow execution) = multiple prev events
So I think it works like expected. Even better, on bluetooth devices with A SINGLE button, you can now use it for next
and prev
.
The only unexpected behaviour might be that multiple fast next
clicks won't do anything, as long as the timer is not hit - you have to click slower, but since this feature is disabled by default, I don't see this as a huge problem.
android/app/src/main/java/com/audiobookshelf/app/player/MediaSessionCallback.kt
Show resolved
Hide resolved
…essionCallback.kt Remove CoroutineContext depenency Co-authored-by: advplyr <[email protected]>
@advplyr I accepted the change suggestion, but how would we deal with the conflicts? I'm not that experienced in doing code reviews with conflicts on GH. |
I can handle the merge conflicts. That change I suggested is just so that you can remove all the coroutine stuff you added in the PlayerNotificationService. I also see code commented out so I'm not sure if this is still in progress? |
Not really in progress but hoping to get corrected by a professional :-) I'm not a kotlin expert and the coroutine stuff was new to me, I was just trying to get this working without breaking anything. So feel free to merge and improve the code whereever you think it's worth. Once the merge is done I'm going to test this carefully and give feedback. Future steps I plan to implement to improve this feature:
For now I just hope this PR gets accepted somehow ;-) |
@advplyr Any news? If there is anything I can do to support you, let me know... |
Fixes #847
In addition I should say:
Feedback welcome.