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 sorting support for theme media playback #5714

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

Conversation

ItsAllAboutTheCode
Copy link

@ItsAllAboutTheCode ItsAllAboutTheCode commented Jun 17, 2024

Changes

The ThemeMusicPlayer code has been updated to support the following SortBy options.

  • Random
  • Sort Name
  • Album
  • Album Artist
  • Artist
  • Date Created
  • Date Played
  • Play Count
  • Runtime

The Sort By options sorts theme media based on the selected option on the server.
Furthermore the Sort Order options were also added.

  • Ascending
  • Descending

The Sort Order options support all Sort By options with the exception of "Random".

Ex. Sorting by Runtime + Descending

This will add the theme media to the play queue in order of longest runtime to shortest

Ex. Sorting by Album Artist + Ascending

This will add the theme media to the play queue in order in language order by Album Artist name

Ex. Sorting by Random

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

Updated the Display settings for both the current and experimental display mode to add a Background Playback Settings menu which groups the "Backdrop", "Theme Music" and "Theme Video" settings into one menu along with a Sort By dropdown and an arrow icon for selecting which category to sort theme media by and whether they should be sorted in ascending or descending 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

Preview

Legacy Background Playback Menu

BackgroundPlaybackMenu_v2.mp4

Experimental Background Playback Popover

ExperimentalBackgroundPlaybackMenu_v2.mp4

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

src/components/displaySettings/displaySettings.js Outdated Show resolved Hide resolved
src/components/themeMediaPlayer.js Outdated Show resolved Hide resolved
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
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.

src/components/themeMediaPlayer.js Outdated Show resolved Hide resolved
src/components/themeMediaPlayer.js Outdated Show resolved Hide resolved
src/components/themeMediaPlayer.js Outdated Show resolved Hide resolved
@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
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
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
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.

@thornbill thornbill added discussion needed Requires input from the community ui & ux This PR or issue mainly concerns UI & UX labels Aug 21, 2024
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Aug 23, 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.

@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Aug 25, 2024
@ItsAllAboutTheCode
Copy link
Author

I have opened up a discussion for this feature at #5958

@ItsAllAboutTheCode ItsAllAboutTheCode force-pushed the theme-media-shuffle branch 3 times, most recently from a5c74b4 to 9a5563f Compare September 15, 2024 06:36
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Oct 12, 2024

Cloudflare Pages deployment

Latest commit 5a38089
Status ✅ Deployed!
Preview URL https://941b76fe.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

@ItsAllAboutTheCode
Copy link
Author

I have opened up a discussion for this feature at #5958

I have updated the PR to incorporate the changes to the UI provided by @erikbucik.
More feedback would be welcome though!

@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 added the merge conflict Conflicts prevent merging label Jan 6, 2025
The ThemeMusicPlayer has been updated to support the "SortName" and "Random" Sort By options to allow sorting theme media for playback by either the sorted name or at random depending on the number of items.
The "Ascending" and "Descending" Sort Order options were also added, which adds support to playback the theme media in reverse order of the sorted name. This is only supported for the "SortName" option

Updated the Display settings for both the current and experimental display mode to add the "Theme media sort by" and "Theme media sort order" drop-down which allows users to modify the ThemeMusicPlayer sort options.

The Display Settings are only shown if either the "Theme songs" or "Theme videos" checkboxes are checked.

This is the implementation for the feature request located https://features.jellyfin.org/posts/2740/select-audio-from-the-theme-music-folder-at-random
Updated the themeMediaPlayer to use lodash `shuffle` function to randomly shuffle the Theme media array

Changed the default theme mediasort by option to be "Random".
Therefore theme music and theme videos from a folder will play in a random order.
The LibraryApi `getThemeMedia` is now forwarded the theme SortBy/SortOrder options to perform the sorting of theme media on the server.

Updated the both the current display settings and experimental displaySettings to also allow sorting by Album, AlbumArtist, Artist, DateCreated, DatePlayed, PlayCount and Runtime.
This is in addition to supporting Random and SortName(Track Name) sort fields.

Verified changes locally by
1. generating latest openapi.json file based on commit jellyfin/jellyfin@24f355a
2. Using the [jellyfin-sdk-typescript](https://github.com/jellyfin/jellyfin-sdk-typescript) package to generate the new client api and build an updated jellyfin SDK
3. Using the `npm install --save <path-to-local-sdk>` command to use the local jellyfin SDK
The lodash shuffle import and jellyfin sdk ItemSortBy and SortOrder imports were left over from the original client side implementation.
…sion jellyfin#5958

The Theme Media and Backdrop settings has been moved to separate popover component in the experimental layout. For the classic layout the settings were moved to a separate "Background Playback" settings menu.

Consolidated the widgets to select the Sort By/ Sort Order options for playing theme media to be one a single row with a \<select> (dropdown) widget providing the Sort by options followed by an \<IconButton> with arrow icons which indicates the Sort Order direction.
Also addressed other eslint changes such as changing single quotes to double quotes.
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jan 7, 2025
Copy link

sonarqubecloud bot commented Jan 7, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Requires input from the community feature New feature or request ui & ux This PR or issue mainly concerns UI & UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants