Add playback speed option to song menu

[lymnyx]

Description

This PR adds a very basic playback speed option to the new song menu in the redesign and thereby fixes #412.

Currently, pressing the newly added button computes the new playback speed with speed % 3.5 + 0.5, so in steps of 50%.
The speed is remembered while the app is open.
The speed is saved to the database and with that remembered even after restarts.

As I am very inexperienced with Flutter and Dart, my approach here might not be ideal. If that is the case, just let me know.

Screenshots

[Chaphasilor]

Hey, thanks for the PR! Really nice addition!

I have a few comments that I would like to discuss:

• Can we show the speed at a different location? Maybe right below the icon or something? It won't be easy without messing up the alignment, but I don't love that the value is shown in parentheses.
  • Maybe use a Stack to position the text right below the icon without messing up the rest of the layout?
  • If we decide that it should stay the way it is right now, then the translation token should be updated to accept a value (the numeric speed) and use that value within the translations, as there might be languages where the speed value would come before the text
• Given that the option is only really useful for audio books, and Finamp is primarily a music player, I'd like to add a setting for hiding this button. For now it could be a boolean switch, and down the road there might be a third option ("automatic") that shows the button when an audio book is being played (or something similar)
  • Any ideas where the setting should be located? Where would you look for something like this?
  • What do you think should the default be? Should the button be shown or not shown in the menu?
• The quick toggle is nice, but we could also add a long-press option that shows a dialog with more fine-grained options. That could include buttons for increasing and decreasing the speed freely (allowing the user to slow down playback speed below 1.0 if they desire), an input field allowing arbitrary speeds (within a range), and a "reset to default" button. If you don't want to implement that, I can do it myself, I just want to make sure that there's no issue with adding such a dialog?

Again, thanks for the PR, I'm sure we can get this merged quickly. I just want to make it even better, while we're at it already :D

[lymnyx]

I am not that comfortable with the parentheses situation as well ;D, I will change that.

Regarding the settings, I like the idea of adding an "automatic" setting, it could be based on genre ("Speech", "audiobook", ...) and/or maybe album duration (>5h or so)!? That "automatic" setting could then be the default.

I think the setting could be placed into "Layout & Theme" (that's where I would look), "Audio Service" is an option as well, but to me it sounds too technical and backend for a UI setting.

Adding a dialog for that makes sense of course, the codebase (just like the environment) is just so new to me that I wasn't that comfortable immediately trying to implement that, but I think, I would like to try it now.

[Chaphasilor]

I was thinking of using more definitive clues for the automatic detection, like the type of the BaseItemDto (is it just Audio for audio books, like for songs, or is there a separate type?) or the type of the parent collection (there we definitely have the needed type, but I'm not sure how hard it is to get to it, e.g. if the item is in a playlist).
This would enable adding support for AudioBook libraries to the library selector, and I'm not sure right now if there's any difference in how such a library needs to be handled. We could also use the active library as a clue.

[lymnyx]

While adding audiobook libraries could be an idea, I don't know if doing automatic audiobook detection based on the library type is ideal. Personally, when I started using Jellyfin and tried the Audiobook support under the Books library type (which is, if I understand your proposal correctly, what you mean), it was lacking in different ways and I ended up using a normal Music type library instead. Maybe that's just me though...

Also not sure what the situation is with the Bookshelf plugin and how that is then supposed to be handled.

[lymnyx]

Just checked by curling a bit.

The audio in a Books library has in fact a type of AudioBook, so if we do want to, we could differentiate those audiobooks and the music from one another.

We could also just do both (checking type and genre) to account for different "setups"?

[Chaphasilor]

Yeah that sounds good. You're right, I always confuse audio books with music videos, that's why I thought there is a dedicated collection. I'm surprised that there's an actual AudioBook type, but I guess otherwise the web frontend wouldn't know how to show/play the items.
Did you try selecting a book library in Finamp and seeing if any sensible data is being fetched? My guess is the data types are too different for it to work, so we'll probably have to go with the genre and length detection for now. Although I'm not sure if there is a sensible length threshold that can handle audiobooks with short chapters (are separate chapters a thing for audio books?) and long songs (e.g. classical music pieces).

I'll go ahead and add a new sub-settings page (like tabs) for customization options and move the new setting there, otherwise the layout settings get pretty messy. I'll probably also reorder the settings a bit to move it towards the top. Do you also want me to take a look at adding the details dialog on long press, or do you want to try it yourself first? :)

[lymnyx]

I haven't yet checked the returned data for the book library in comparison to a music library, but I will do that.

I would try to fetch the album's duration and use that as a basis, if that is a thing with the current architecture I would prefer that over track duration.
What do you think is an appropriate minimum duration for an album to be considered an audiobook?

A new settings page is nice!
I would like to try the long press myself, thank you for being so considerate by the way <3.

I am not so sure how to implement your idea with the positioning of the playbackSpeed variable as I don't quite know where to add the Stack. As a child of PlaybackAction?

[lymnyx]

Apart from having to traverse down into the actual folders (Home/Books/Audiobooks/[Artist/Album || Album]) with the files to get any sensible metadata, at least on first look the BaseItemDto of the audio files seems to be fairly similar, much of the typical stuff one would need, is there, I think.

So maybe adding support for the official Jellyfin Audiobook implementation is really something that could be fairly easily done down the line.

[Chaphasilor]

Okay. Could you check if the duration is properly set for the audiobook album? If that's there, we can use it for the detection. I'd say it should be at least 2h to the automatic detection, do you think that would that work?

So maybe adding support for the official Jellyfin Audiobook implementation is really something that could be fairly easily done down the line.

That sounds promising, would you be willing to do that in a follow-up PR? With the redesign we can make some breaking changes, so it would be easy to fit it in. I just don't have the resources (nor the interest) to look into audio books right now, but I'm definitely willing to help out a bit!

[lymnyx]

Oh, sorry, I just noticed the folder itself (in contrast to the files) seems to somewhat lack metadata actually, it has no RunTimeTicks. The metadata (like RunTimeTicks) would have to be mainly grabbed from the audio items, I guess.

After this PR, I will still see what I can do, there seems to be an issue for this already (#231).

[Chaphasilor]

I am not so sure how to implement your idea with the positioning of the playbackSpeed variable as I don't quite know where to add the Stack. As a child of PlaybackAction?

Sorry, seems like I didn't see this message 😅
Yes, there is a Column with an Icon, a SizedBox (for the spacing) and another SizedBox for giving the labels of the PlaybackActions a consistent height. You should be able to wrap the Icon in a Stack, and then put a Positioned widget into the Stack that wraps a Text containing the current value.
You could make the value a nullable parameter of the PlaybackAction constructor, and not show any text if the value is null.
Does that make it clearer? :)

[lymnyx]

Yes, it does. Thank you!

[Chaphasilor]

I improve upon it a bit, making the icons all have the same size. Do you think the speed text is too small now? 😅

I'm also planning to leave a review tomorrow or on the weekend, there are a few things that I'd like you to tweak :D

[lymnyx]

I thought about keeping the size the same, but I didn't particularly like the reduced vertical spacing and thought even with the changed size it looked quite nice. (Edit: And yes, I think the speed text is a bit small.) Matter of taste, I guess ;).

I'm also planning to leave a review tomorrow or on the weekend, there are a few things that I'd like you to tweak :D

Sure, looking forward to it ;). Just added the album length detection, right now the seemsLikeAudiobook function is started when opening the song menu (and therefore needs a moment for connecting to the API when album length checking is required), we might want to already determine whether the button should be displayed when opening the player screen or so.

I was also trying to add the long-press dialog by wrapping the IconButton with a GestureDetector, setting its onLongPress parameter. That doesn't work for some reason (even though wrapping the Column inside instead works without problems). Any idea why? Or is there a better way of detecting the long-presses?

[Chaphasilor]

Okay, I need to think about the PlaybackAction design some more. I looked at the other actions, and those also contain some sort of state/value for the most part, so they might have to be changed as well. Or we integrate the playback speed into the label more nicely, like "Playing at x1.5 speed" or something.

For the seemsLikeAudiobook function, the name doesn't fit because it also handles showing it if the playback speed has been changed, even if the item is not an audio book. Maybe call it shouldShowSpeedSelector or something (maybe you can think of a better name).
As for when to call the function, ideally we'd replicate something like current_album_image_provider.dart, that is then able to pre-fetch the data for the upcoming items. That should be flexible to also support the FeatureChips that show metadata about the current track. The "provider" stuff is implemented with Riverpod, in case you want to give it a try.
I wouldn't want to leave the function within the menu if it's asynchronous, because that could cause layout shifts after opening the menu. And make sure to test everything in offline mode, making sure that no requests are sent to the server in that case.

Regarding the IconButton and GestureDetector, it probably doesn't work because the IconButton also recognizes gestures. Using just a GestureDetector isn't great for accessibility, because that is not recognized as a button. There's also no easy way to long-press when using a screen reader.
I see two good solutions: 1) always showing the dialog, even when just tapping the button (so no quick toggle), or 2) adding some kind of indicator that can be pressed for the "extended options". But the first solution gets rid of some nice functionality and the second solution is a challenging UI and layout issue.

For now, go with just the GestureDetector. It still works and can be used with a screen reader, I just need to add some additional semantics info down the road.

[jmshrv]

As for design, I quite like how Apple Podcasts does it (clicking shows a dropdown)

[lymnyx]

@Chaphasilor

I like that, it would fit right into the design of the sleep timer.

I wouldn't want to leave the function within the menu if it's asynchronous, because that could cause layout shifts after opening the menu.

Yes, that was my concern.

For now, go with just the GestureDetector.

Yeah, I just don't know how I'm supposed to fix the IconButton stealing the mouse events from the GestureDetector.
I could use an IgnorePointer or something like that, but then I would interfere with the onPressed, the focus styling and the long-press tooltip.

[lymnyx]

As for design, I quite like how Apple Podcasts does it (clicking shows a dropdown)

@jmshrv I like it too, but in my humble opinion a design similar to AntennaPod's might be a bit more fitting. (Considering Finamp's new queue design, etc.)

[Chaphasilor]

Okay, all good points.
Firstly, if we go with the long-press stuff, just replace the IconButton with a GestureDetector wrapping an Icon :)

But I don't think we'll do the long-press stuff, the drop-down / picker list is a nice idea. The way it's implemented in AntennaPod is actually similar to what I had in mind for the long-press dialog, just with presets instead of increment/decrement buttons.
But I guess presets are more useful and faster to interact with, and could also be user-customizable. I'm not a fan of the slider for selecting the speed though, a number input should be enough if we have presets already.

I could try to whip up a mockup tomorrow :)

[lymnyx]

Like this, maybe?

[Chaphasilor]

In the end, it depends on the width of the device. But we can probably add a bit of margin again, you're right.
I can do it later.

So the only thing missing now is the pre-fetching of metadata. I can take a look at it, since something similar will be needed for getting lyrics as well.

[Chaphasilor]

Like this, maybe?

I think that's a bit too much, but I'll have to check on my phone later. Maybe it's still possible to select a certain step, in that case all is well.

[lymnyx]

At least on my phone it wasn't a problem.

[lymnyx]

An alternative would be to have the slider go to 2.5x, but still provide "3x" and "3.5x" presets, for example. Would you like that better?

[Chaphasilor]

At least on my phone it wasn't a problem.

Which one do you have?

An alternative would be to have the slider go to 2.5x, but still provide "3x" and "3.5x" presets

So the exact opposite of what we have right now? ^^
Possible. The slider has no value indicators anyway, so it probably would be fine.
I think we can try it out and gather some initial feedback with that.

[lymnyx]

Which one do you have?

Google Pixel 6a, so, to be fair, quite big. Not sure about the Apple SEs ;).

So the exact opposite of what we have right now? ^^

Yep :D, sorry.

I think we can try it out and gather some initial feedback with that.

Okay!

[Chaphasilor]

I have an Asus Zenfone 10, so rather small and tall

[Chaphasilor]

Well, selecting the x3.5 results in an exception, a simple clamp should fix that.

[lymnyx]

Good point, what a stupid mistake, should have tested for that.

I've also added a small indicator for when the user selects a preset value above the maximum the slider can display.
Let me know what you think.

[Chaphasilor]

Yeah that looks good and works well!

[Chaphasilor]

@lymnyx just to give you a quick update, I've now implemented a metadata provider that give us info for the upcoming tracks in #693, which I should be able to re-use for deciding if the button should be shown or not. That means I can get this PR finished up once the lyrics PR is merged.

I noticed that the current check might also return true for playlists (depending on where the parentId is coming from, I'll have to check). Do you have any other ideas to make the detection more reliable (even if it would be a bit more involved)?

[lymnyx]

Great, thanks for the update!

I noticed that the current check might also return true for playlists (depending on where the parentId is coming from, I'll have to check).

In case the parentId of the playlist items is your concern, that, after a quick test with a playlist of mine, seems to be, like expected, the album id. The menu doesn't show the speed options for me when starting tracks from huge playlists. Not sure if this is what you meant.

Do you have any other ideas to make the detection more reliable (even if it would be a bit more involved)?

I mean, theoretically we could check the pre-fetched audio for (repeated/prevalent) near-silence and detect typical audiobooks that way, maybe skipping the first few seconds of audio to account for music having to start. Not sure if you would call that reliable or worth it though ;).

As we are already checking for the album length (provided that it works as expected which I've assumed) and the item genres, I am not sure what would be left for us to easily and reliably analyze as we obviously have to differentiate normal music tracks and audiobook tracks that might be, as far as the backend is concerned, categorized in precisely the same way.

[Chaphasilor]

yeah that's what I meant, good to know. thanks for testing.

Analyzing the audio itself wouldn't be a great idea I think, and much too complex either way.
At some point we might have a way to mark a whole library as containing audio books, either in Finamp or in Jellyfin itself, that would work better for this.
We should also try to support podcasts, so maybe checking the duration of the current track isn't a bad idea either.

I guess if the speed menu is "accidentally" shown, it's not a big deal...

[Chaphasilor]

Okay, the other PR has now been merged, and I've brought this one up to speed with the latest changes. Hopefully I haven't made mistakes during the merge.

I'll look into moving the shouldShowSpeedControls() method into the metadata provider. I'll try to remember to only request the parent item if the visibility is set to "automatic".
Depending on how quickly I can get that done, it might land with the next update today or tomorrow!

[Chaphasilor]

@lymnyx could you give the new changes some testing? Everything should be working now, and if you give the go, I'll include it in the update :D

[Chaphasilor]

@lymnyx many thanks for your efforts with this PR!

[lymnyx]

@Chaphasilor Thank you so much for your not-so-minimal assistence and patience <3, really helped a lot. Great to have this in the beta now!