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

Updated theme media playback to play in Random order #5714

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

ItsAllAboutTheCode
Copy link
Contributor

@ItsAllAboutTheCode ItsAllAboutTheCode commented Jun 17, 2024

Changes

The ThemeMusicPlayer code has been updated to playback theme media using the Random sort order which causes theme media to be added to the play queue in a Random order.

Issues

This is the implementation for the feature request located https://features.jellyfin.org/posts/2740/select-audio-from-the-theme-music-folder-at-random.
#5958

@ItsAllAboutTheCode ItsAllAboutTheCode requested a review from a team as a code owner June 17, 2024 06:26
@ItsAllAboutTheCode ItsAllAboutTheCode force-pushed the theme-media-shuffle branch 2 times, most recently from 050d57c to cfa4480 Compare June 17, 2024 06:43
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

@nielsvanvelzen
Copy link
Member

To me it doesn't make sense to add an option for this, just making it random by default should be sufficient.

@ItsAllAboutTheCode
Copy link
Contributor Author

ItsAllAboutTheCode commented Jun 17, 2024

To me it doesn't make sense to add an option for this, just making it random by default should be sufficient.

The reason I added an option is that I noticed that when multiple audio files existing within a theme-music/* directory, it plays ALL the songs in sequential order.

So the "SortName" option still allows users to control the order in which theme music and videos play via how they name their files within a "theme-music" or "backdrops" folder.

I didn't set the option to default to "Random" in order to maintain the current behavior, however I wouldn't be averse to changing the default to be "Random" instead of "SortName" if it is OK to not maintain backwards compatibility?

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.

My review in case you keep the order selection.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@ItsAllAboutTheCode
Copy link
Contributor Author

To me it doesn't make sense to add an option for this, just making it random by default should be sufficient.

The reason I added an option is that I noticed that when multiple audio files existing within a theme-music/* directory, it plays ALL the songs in sequential order.

So the "SortName" option still allows users to control the order in which theme music and videos play via how they name their files within a "theme-music" or "backdrops" folder.

I didn't set the option to default to "Random" in order to maintain the current behavior, however I wouldn't be averse to changing the default to be "Random" instead of "SortName" if it is OK to not maintain backwards compatibility.

I kept the order selection options, but changed the default to be "Random".

@jellyfin-bot
Copy link
Collaborator

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

@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jun 23, 2024
ItsAllAboutTheCode added a commit to ItsAllAboutTheCode/jellyfin that referenced this pull request Jun 24, 2024
The `GetThemeMedia, `GetThemeVideos` and `GetThemeSongs` functions can optionally sort the results based based on passing an ItemSortBy type and a SortOrder.

This is intended to be used by jellyfin-web in order to allow users to control the order of theme playback.
See PR: jellyfin/jellyfin-web#5714
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jul 21, 2024
@jellyfin-bot
Copy link
Collaborator

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

Bond-009 pushed a commit to jellyfin/jellyfin that referenced this pull request Jul 21, 2024
* Changed `GetThemeMedia` to support SortBy/Order options

The `GetThemeMedia, `GetThemeVideos` and `GetThemeSongs` functions can optionally sort the results based based on passing an ItemSortBy type and a SortOrder.

This is intended to be used by jellyfin-web in order to allow users to control the order of theme playback.
See PR: jellyfin/jellyfin-web#5714

* Update MediaBrowser.Controller/Entities/BaseItem.cs

Fix the `GetThemeVideos` two argument overload having both parameters defaulted.
For the two argument overload, both parameters are required.
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jul 21, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

@ItsAllAboutTheCode ItsAllAboutTheCode changed the title Added support to shuffle theme media playback Added sort selection support for theme media playback Jul 22, 2024
@ItsAllAboutTheCode ItsAllAboutTheCode changed the title Added sort selection support for theme media playback Add sorting support for theme media playback Jul 22, 2024
@ItsAllAboutTheCode
Copy link
Contributor Author

I have updated the PR to use the new GetThemeMedia Library api to perform the sorting and verified by locally generated the jellyfin SDK and testing the changes.

When this repo is eventually updated to use the jellyfin SDK which contains the change from commit jellyfin/jellyfin@24f355a, then the sort options will work transparently.

Until then the current behavior of not performing any sorting still occurs without error.

@nielsvanvelzen
Copy link
Member

I've merged the latest unstable version of the SDK via #5747. You should be able to update your branch now and use the API changes.

@ItsAllAboutTheCode
Copy link
Contributor Author

I've merged the latest unstable version of the SDK via #5747. You should be able to update your branch now and use the API changes.

Thanks for updating the API version.
Unfortunately it seems the jellyfin-sdk-typescript unstable was just updated before the LibraryController.cs changes were submitted, therefore it doesn't have the API changes.

I'll wait until the next the API updates to test changes again.

@ItsAllAboutTheCode
Copy link
Contributor Author

I've merged the latest unstable version of the SDK via #5747. You should be able to update your branch now and use the API changes.

Thanks for updating the API version. Unfortunately it seems the jellyfin-sdk-typescript unstable was just updated before the LibraryController.cs changes were submitted, therefore it doesn't have the API changes.

I'll wait until the next the API updates to test changes again.

I just tested using the latest jellyffin SDK unstable version update from the yesterday and the changes are all working as intended.
Thanks!

@jellyfin-bot
Copy link
Collaborator

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

@mihawk90
Copy link
Contributor

Ex. Sorting by Random

This will add the theme media to the play queue in random order

Just for clarification and documentation purposes:
As I'm reading this, this is actually a shuffle, not random, correct?
As in, shuffle takes the list of themes, randomises them, then puts them in the queue, but from that point on the order is set.
Randomised would mean that after each theme ends, a new one is randomly selected, which in theory could also lead to the same theme being played twice. It would also mean this allowed for "endless" playback (which right now is also not possible as far as I know, once the theme(s) played, the player stops and the fanart is shown).

@ItsAllAboutTheCode
Copy link
Contributor Author

ItsAllAboutTheCode commented Jan 11, 2025

Ex. Sorting by Random

This will add the theme media to the play queue in random order

Just for clarification and documentation purposes: As I'm reading this, this is actually a shuffle, not random, correct? As in, shuffle takes the list of themes, randomises them, then puts them in the queue, but from that point on the order is set. Randomised would mean that after each theme ends, a new one is randomly selected, which in theory could also lead to the same theme being played twice. It would also mean this allowed for "endless" playback (which right now is also not possible as far as I know, once the theme(s) played, the player stops and the fanart is shown).

You are correct, this will take the list of themes and shuffle them before adding them to the play queue.
Once they are added to the play queue, their order is not changed.
So it is form of static randomization before the themes are added to the play queue and not a form of on-demand randomization where the next theme is chosen at random.

@thornbill
Copy link
Member

To me it doesn't make sense to add an option for this, just making it random by default should be sufficient.

I agree with this. This is such a niche feature (really a niche of a niche of a niche as it requires people to have theme media, have multiple theme media, and care about the order they are played) that I find it hard to justify adding and maintaining 600 lines of code for it.

@ItsAllAboutTheCode
Copy link
Contributor Author

To me it doesn't make sense to add an option for this, just making it random by default should be sufficient.

I agree with this. This is such a niche feature (really a niche of a niche of a niche as it requires people to have theme media, have multiple theme media, and care about the order they are played) that I find it hard to justify adding and maintaining 600 lines of code for it.

I can update this PR to change the default sort order to random and revert all of the UI and settings changes.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

@ItsAllAboutTheCode
Copy link
Contributor Author

To me it doesn't make sense to add an option for this, just making it random by default should be sufficient.

I agree with this. This is such a niche feature (really a niche of a niche of a niche as it requires people to have theme media, have multiple theme media, and care about the order they are played) that I find it hard to justify adding and maintaining 600 lines of code for it.

I can update this PR to change the default sort order to random and revert all of the UI and settings changes.

I have updated the PR to set the theme media player to default to random playback.

@thornbill thornbill added enhancement Improve existing functionality or small fixes and removed feature New feature or request discussion needed Requires input from the community ui & ux This PR or issue mainly concerns UI & UX labels Feb 13, 2025
@thornbill thornbill added this to the v10.11.0 milestone Feb 13, 2025
@thornbill
Copy link
Member

Could you update the PR title and description to reflect the update? Thanks!

@ItsAllAboutTheCode ItsAllAboutTheCode changed the title Add sorting support for theme media playback Updated theme media playback to play in Random order Feb 13, 2025
@ItsAllAboutTheCode
Copy link
Contributor Author

Could you update the PR title and description to reflect the update? Thanks!

Done.

@thornbill thornbill merged commit 7433a4a into jellyfin:master Feb 13, 2025
15 checks passed
@ItsAllAboutTheCode ItsAllAboutTheCode deleted the theme-media-shuffle branch February 14, 2025 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality or small fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants