-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Implement Theme Song Volume Control #6674
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
base: master
Are you sure you want to change the base?
Conversation
Cloudflare Pages deployment
|
|
The media element is not destroyed. If this is not on purpose, we should destroy it completely, as in the video player. But we can leave it as is for now.
I think we can introduce a "volume mode" (primary and secondary) and make players use it to get/set the appropriate value. jellyfin-web/src/plugins/htmlAudioPlayer/plugin.js Lines 280 to 283 in 3df2a1f
jellyfin-web/src/components/htmlMediaHelper.js Lines 6 to 14 in 3df2a1f
We can use We also need to move this to
|
return this.set('themeSongsVolumeLevel', val.toString()); | ||
} | ||
|
||
return parseInt(this.get('themeSongsVolumeLevel') || '100', 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it would be better to make the default volume lower.
return parseInt(this.get('themeSongsVolumeLevel') || '100', 10); | |
return parseInt(this.get('themeSongsVolumeLevel') || '50', 10); |
@@ -9,6 +9,7 @@ export interface DisplaySettingsValues { | |||
enableItemDetailsBanner: boolean; | |||
enableLibraryBackdrops: boolean; | |||
enableLibraryThemeSongs: boolean; | |||
libraryThemeSongsVolumeLevel: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was supposed to be in alphabetical order.
@@ -87,6 +87,7 @@ async function loadDisplaySettings({ | |||
enableItemDetailsBanner: Boolean(settings.detailsBanner()), | |||
enableLibraryBackdrops: Boolean(settings.enableBackdrops()), | |||
enableLibraryThemeSongs: Boolean(settings.enableThemeSongs()), | |||
libraryThemeSongsVolumeLevel: settings.themeSongsVolumeLevel(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was supposed to be in alphabetical order.
const handleSliderChange = useCallback(() => (e: Event, newValue: number | number[]) => { | ||
const target = e.target as HTMLInputElement; | ||
const fieldName = target.name as keyof DisplaySettingsValues; | ||
const value = Array.isArray(newValue) ? newValue[0] : newValue; | ||
|
||
if (values?.[fieldName] !== value) { | ||
updateField({ | ||
name: fieldName, | ||
value: value.toString() | ||
}); | ||
} | ||
}, [updateField, values]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't handleFieldChange
(onChange
callback) enough?
About
Implement basic input slider in "Display" settings menu to set theme song volume level during playback.
Changes
Issues
Fixes #3086
Details
Currently, I've only setup the ui and backend logic for setting and getting the theme song volume level.
I'm able to set the theme song's volume level quite easily in
components/themeMediaPlayer.js
, but doing that has the unintended side effect of also setting the volume level of all media playback. This is because there's only 1 shared media player for the application.I tried a workaround involving storing the previous audio level then restoring it once the theme song playback ended, but couldn't get it to work perfectly without any issues / minor bugs.
I also wasn't able to test the changes made to
src/apps/experimental/features/preferences/components/LibraryPreferences.tsx
because I'm not sure where to access that UI.