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.

Sign up for GitHub

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

Fix playback continuation on external controller command involving search (through the onPlayFromSearch) #2280

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix playback continuation on external controller command involving search (through the onPlayFromSearch) #2280

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a1ab882
Relax the controller check on onConnect
ASGusev Dec 10, 2023
bc6ef86
Add new session creation if the is released
ASGusev Dec 10, 2023
42ea4ff
Merge branch 'main' into external-controller-start
ASGusev Jan 27, 2024
bbecb23
Move the position loading
ASGusev Feb 4, 2024
fbe0efa
Return the current book for empty query searches
ASGusev Feb 5, 2024
fb28218
Switch the search query check to isNullOrBlank
ASGusev Feb 10, 2024
26c5e88
Merge branch 'main' into external-controller-start
ASGusev Feb 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions playback/src/main/kotlin/voice/playback/session/PlaybackService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,13 @@ class PlaybackService : MediaLibraryService() {
release()
}

override fun onGetSession(controllerInfo: MediaSession.ControllerInfo): MediaLibrarySession? {
return session.takeUnless { session ->
session.invokeIsReleased
}.also {
if (it == null) {
Logger.e("onGetSession returns null because the session is already released")
}
}
override fun onGetSession(controllerInfo: MediaSession.ControllerInfo): MediaLibrarySession {
if (session.invokeIsReleased)
rootComponentAs<PlaybackComponent.Provider>()
Copy link
Owner

Choose a reason for hiding this comment

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

And are you sure this is necessary? I'm hesitant to merge this because this will literally create new instances of the whole player stuff - while not releasing the old things.
I fear that this might lead to several players playing at the same time under some circumstances

Copy link
Author

Choose a reason for hiding this comment

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

A new session is only created in a situation when the service is not destroyed after a release call. I face this situation when I run Voice from SimpleWear and then close it. Now in such a situation playback cannot be started even by the play button in the player activity (or the play button in the books list). I think this behaviour is not more desirable.
By the way, in the comment in onTaskRemoved you have written that releasing the session is required to prevent showing a dead notification. Later, in a comment to my previous PR, you've mentioned having fixed the notification being unresponsive. So, is the dead notification still an issue, and is the release call in onTaskRemoved still required? It seems to me that only calling release from onDestroy would obviate the sole need to check if the session state.

Copy link
Author

Choose a reason for hiding this comment

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

And which conditions do you mean? Is that onGetSession being called from another thread than onCreate that does not see the session created there?

Copy link
Author

Choose a reason for hiding this comment

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

Also, I don't quite understand what you mean by 'not releasing the old stuff' - a new session is only created if the old one has already been released.

Copy link
Author

Choose a reason for hiding this comment

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

Any updates?:)

.playbackComponentFactory
.create(this)
.inject(this)
return session
}
}

Expand Down
2 changes: 1 addition & 1 deletion playback/src/main/kotlin/voice/playback/session/search/BookSearchHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class BookSearchHandler

// Look for anything that might match the query
private suspend fun searchUnstructured(query: String?): Book? {
if (query != null) {
if (query != null && query != "") {
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick: you can use isNullOrBlank

Copy link
Author

Choose a reason for hiding this comment

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

Switched

val foundMatch = findBook {
val bookNameMatches = it.content.name.contains(query, ignoreCase = true)
val authorMatches = it.content.author?.contains(query, ignoreCase = true) == true
Expand Down