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 option to customize seek time #3794

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

kabel2
Copy link

@kabel2 kabel2 commented Jul 23, 2024

Changes
Adds a slider in the playback settings to set the fast-forward/rewind times. Minimum 1 second, maximum 60 seconds. Default 15 seconds.
TODO:
I think the naming of the setting needs to be changed. It may also be necessary to change the slider to two dropdowns (as in the Android smartphone app).

Issues
Fixes my own frustration of not being able to rewind a short dialog.

@fice-t
Copy link
Contributor

fice-t commented Jul 24, 2024

This should ideally take into account the user-specific settings made on the Jellyfin server, i.e. the Skip {forward,back} length values set via the web client's menu->User->Settings.

These settings are already partially used in the Android TV client, as seen here and in this issue.

Having an override specifically for the TV client can still make sense (a user may want different seeking behaviour between clients), but I think that it should still default to the server-side settings.

Also, perhaps the seek time should be cached somewhere so the preferences aren't being hit for each getSeekPositions? Could the seek time itself just be fed into CustomSeekProvider?

@kabel2
Copy link
Author

kabel2 commented Jul 24, 2024

I have already seen the settings for forward and back. However, the SeekProvider does not allow a distinction between forward and back, so there is only one value.

The setting is therefore different from the Android app/web app. I think using only the value of “back” would not be much better than creating a new (local) value (seek time). Maybe this would be a temporary solution until there is a rewrite of the playback.

The seektime can of course be given directly to the CustomSeekProvider, would be no problem to change that.

@fice-t
Copy link
Contributor

fice-t commented Jul 26, 2024

However, the SeekProvider does not allow a distinction between forward and back, so there is only one value.

Right, that does complicate things. Luckily, the ATV playback rewrite allows such a distinction.

Since there is the rewrite coming up, I would propose the following:

Legacy code

  • Use one local setting (UserPreferences)
    • Do not fall back to either server setting (UserSettingPreferences) to avoid possible confusion
    • Always enabled in the UI

Rewrite code

  • Use two local settings, one each for back/forward
    • Fall back to the respective server setting if not changed locally
    • Only enable each local setting in the UI if requested (perhaps gated by a checkbox?)
      • Either way, the UI should present each server-side value if the local setting is not enabled.

This PR already accomplishes the legacy part, and could be considered self-contained. I wrote some thoughts about how this PR could consider the transition between legacy/rewrite, but came to the conclusion that it doesn't matter much on the legacy side. Perhaps the names could be changed from *seek_time* -> *seek_time_legacy*, since in the above scheme the keys/descriptions would need to be different between legacy/rewrite. Also, maybe duration is better than time for the names?

So perhaps the rewrite side could be punted off for later. FWIW, I was previously planning on working on custom seek times once the rewrite was completed if no one else got to it first.

@nielsvanvelzen Thoughts on all this?

@nielsvanvelzen
Copy link
Member

We should re-use the skip forward/backward preferences already available in the user settings. Not add a new one.

@nielsvanvelzen
Copy link
Member

Hey @kabel2, are you still interested in finishing this pull request? it's currently waiting for you to address my comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Conflicts prevent merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants