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

Merged
merged 12 commits into from
Nov 10, 2024
Merged

Conversation

kabel2
Copy link
Contributor

@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
Contributor 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.

@kabel2
Copy link
Contributor Author

kabel2 commented Oct 8, 2024

Sorry for the late reply.
Without the rewrite of the player it is not possible to distinguish between forward and backwards.
So should we use the mean value of forward and backwards or how should two settings be used for one value?

@nielsvanvelzen
Copy link
Member

We can use the "forward" preference for now and update it later when we redo the player design.

@kabel2
Copy link
Contributor Author

kabel2 commented Oct 12, 2024

Okay, I'll take a look at the necessary changes tomorrow :)

@kabel2 kabel2 reopened this Oct 19, 2024
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Oct 19, 2024
@kabel2
Copy link
Contributor Author

kabel2 commented Oct 19, 2024

Is now adjusted.
But there is still a problem, the minimum skip length is just 0, although I have set it to min = 5_000.

@nielsvanvelzen
Copy link
Member

The tests are currently failing

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Nov 5, 2024
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Nov 6, 2024
Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

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

I've made 2 final changes:

  1. Fix the issue where you can change the value below the minimum - this wasn't your mistake but a bug in our UI code
  2. Use the long type in CustomSeekProvider and do the casting from LeanbackOverlayFragment (where we initialize the provider)

Thanks for the pull request!

@nielsvanvelzen nielsvanvelzen added this to the v0.18.0 milestone Nov 10, 2024
@nielsvanvelzen nielsvanvelzen added the enhancement New feature or request label Nov 10, 2024
@nielsvanvelzen nielsvanvelzen enabled auto-merge (squash) November 10, 2024 13:46
@nielsvanvelzen nielsvanvelzen merged commit 15554bd into jellyfin:master Nov 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants