Skip to content

Implement playback inactivity timeout #6686

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

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

Conversation

simonerlic
Copy link

Implements an "Are you still there?" popup after user-defined criterion are met.
User may change the behaviour in playback settings. Users can set the prompt to appear after a set number of episodes with no ineraction (e.g. mouse movement), or after a set amount of time with no interaction.

Changes

  • Created a playTimeout plugin
  • Added user settings for adjusting feature parameters
    • Added controls to playbackSettings.js, playbackSettings.template.html, and userSettings.js
  • Added a bind to playmanager.js
  • Added localization for en-US and en-GB

Issues

Work based off of conversations on feature request 814
Planned work discussed on GitHub discussions

simonerlic and others added 25 commits March 17, 2025 18:01
- Updated `saveUser()` to persist both `stillWatchingTimeout` and `askAfterNumEpisodes`, regardless of mode, preserving values across toggles.
- Added new helper `displayStillWatchingMode(context, userSettings)` to `loadForm()` to correctly display the relevant input (time or episode) based on saved mode.
Refactor still watching mode: save both values, display only active mode
@simonerlic simonerlic requested a review from a team as a code owner March 31, 2025 19:48
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Mar 31, 2025

Cloudflare Pages deployment

Latest commit a8dde66
Status ✅ Deployed!
Preview URL https://6f84a49e.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Mar 31, 2025
@jellyfin-bot
Copy link
Collaborator

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@jellyfin-bot jellyfin-bot added the stale No activity for an extended length of time label Apr 1, 2025
@jhparker88
Copy link

I think you should squash your commit as it contains an unrelated commit history.

Added localization for en-US and en-GB

Translations except for en-US are provided by weblate. Also something happened with the updated en-US json which now contains unrelated format changes which makes the whole commit look huge (+4,537 −3,899 ).

Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

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

It's also a good idea to make changes in a separate branch rather than using master.

Comment on lines +147 to +151
if (stillWatchingEnabled) {
optionsContainer.classList.remove('hide');
} else {
optionsContainer.classList.add('hide');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (stillWatchingEnabled) {
optionsContainer.classList.remove('hide');
} else {
optionsContainer.classList.add('hide');
}
optionsContainer.classList.toggle('hide', !stillWatchingEnabled);

Comment on lines 297 to +298
loading.hide();
initStillWatchingControls(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's hide the loading indicator at the end.

Suggested change
loading.hide();
initStillWatchingControls(context);
initStillWatchingControls(context);
loading.hide();

@@ -347,9 +388,13 @@ function onSubmit(e) {

function embed(options, self) {
options.element.innerHTML = globalize.translateHtml(template, 'core');

Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert unnecessary changes (blank line removal).

@@ -361,6 +406,35 @@ function embed(options, self) {
}
}

function displayStillWatchingMode(context, userSettings) {
// Determine saved mode
const isTimeMode = userSettings.timeBasedStillWatching?.() === true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need optional chain for userSettings.timeBasedStillWatching?

// Determine saved mode
const isTimeMode = userSettings.timeBasedStillWatching?.() === true;
const mode = isTimeMode ? 'time' : 'episodes';

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to make time and episodes consts.

timeInput.value = userSettings.stillWatchingTimeout() || 300;
} else {
episodeInput.value = userSettings.askAfterNumEpisodes() || 5;
}
Copy link
Contributor

@dmitrylyzo dmitrylyzo Mar 31, 2025

Choose a reason for hiding this comment

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

You already have default values.

return parseInt(this.get('stillWatchingTimeout') || '60', 10);

return parseInt(this.get('askAfterNumEpisodes') || '0', 10);

Normally we put the default value in the setting getter.

@@ -1,11 +1,13 @@
<form style="margin: 0 auto;">
<form style="margin: 0 auto">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert unnecessary changes.

* @returns {number} Returns the time in milliseconds after which a timeout should occur
*/
private getTimeoutTime(): number {
return userSettings.stillWatchingTimeout(undefined) * 6 * (10 ** 4); // convert from minutes to milliseconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just 60_000?

Also, there is a const, but it's too long.

export const MILLISECONDS_PER_SECOND = 1_000;

@jellyfin-bot jellyfin-bot added stale No activity for an extended length of time and removed stale No activity for an extended length of time labels Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Conflicts prevent merging stale No activity for an extended length of time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants