Add sort options to playback requests #4786

v0idMrK · 2023-09-17T22:30:07Z

Changes
Added getting the SortOptions on shortcuts.js and list.js files and sending them in the playbackmanager.js play method

Issues
Fixes #4841

CONTRIBUTORS.md

src/components/shortcuts.js

src/components/playback/playbackmanager.js

Adds shortcuts.js function to get sort order Added sort order technique to most types of media on playbackmanager.js

v0idMrK · 2023-09-18T18:37:31Z

@dmitrylyzo Just updated this with improvements suggested by your comments.

Also added the same technique to the other media types on playbackmanager.js. Tested them and they seemed to work fine.

Please let me know if you have any other sugestion! Thank you

dmitrylyzo

We should add sort here (Play in the context menu):

jellyfin-web/src/components/itemContextMenu.js

Lines 592 to 595 in 1e47abb

    
           playbackManager[method]({ 
        
               items: [item], 
        
               startPositionTicks: startPosition 
        
           });

src/components/shortcuts.js

…js and list.js to use the above method

v0idMrK · 2023-09-19T21:30:46Z

@dmitrylyzo Hey, applied the changes. Let me know if anything else is missing :D

Thank you for your feedback.

src/controllers/list.js

dmitrylyzo · 2023-09-19T21:51:33Z

Let me know if anything else is missing

We should add sort for Play in the context menu: #4786 (review)
At least I didn't notice any more.

There is also adding to the play queue, but it doesn't work for photos, so we can leave it for the future.

grafixeyehero · 2023-09-20T01:45:40Z

@dmitrylyzo @v0idMrK
A quick Question I wonder what is the use of queryOptions?

jellyfin-web/src/components/playback/playbackmanager.js

Line 1789 in 055a500

const queryOptions = options.queryOptions || {};

jellyfin-web/src/components/playback/playbackmanager.js

Lines 1852 to 1860 in 055a500

    
           promise = getItemsForPlayback(serverId, mergePlaybackQueries({ 
        
               ParentId: firstItem.Id, 
        
               Filters: 'IsNotFolder', 
        
               Recursive: true, 
        
               SortBy: options.shuffle ? 'Random' : 'SortName', 
        
               // Only include Photos because we do not handle mixed queues currently 
        
               MediaTypes: 'Photo', 
        
               Limit: UNLIMITED_ITEMS 
        
           }, queryOptions));

v0idMrK · 2023-09-20T08:40:38Z

@dmitrylyzo @v0idMrK A quick Question I wonder what is the use of queryOptions?

jellyfin-web/src/components/playback/playbackmanager.js

Line 1789 in 055a500

const queryOptions = options.queryOptions || {};

jellyfin-web/src/components/playback/playbackmanager.js

Lines 1852 to 1860 in 055a500

promise = getItemsForPlayback(serverId, mergePlaybackQueries({

ParentId: firstItem.Id,

Filters: 'IsNotFolder',

Recursive: true,

SortBy: options.shuffle ? 'Random' : 'SortName',

// Only include Photos because we do not handle mixed queues currently

MediaTypes: 'Photo',

Limit: UNLIMITED_ITEMS

}, queryOptions));

I will try to check today. If I don't have time, then I will go after it tomorrow.

Thank you for the feedback!

dmitrylyzo

A quick Question I wonder what is the use of queryOptions?

It doesn't seem to be passed anywhere in options, but good catch.
In theory it could be used to adjust the base query (only folders are currently using it).

Looking at list.js, the sort options (SortBy and SortOrder) can be placed in queryOptions.
For reference:

jellyfin-web/src/controllers/list.js

Lines 77 to 78 in 055a500

    
           query.SortBy = sortValues.sortBy; 
        
           query.SortOrder = sortValues.sortOrder;

In the list.js and shortcut.js, we can put sort options as queryOptions.SortBy and queryOptions.SortOrder.
In the playbackmanager.js, we can drop getSortOptions and revert my suggestion (#4786 (comment), most of f922742), and instead merge queryOptions using mergePlaybackQueries.

I made test changes:
test-photo.diff.txt

Btw, SyncPlay duplicates the playback manager. So it should be revised as well.

jellyfin-web/src/plugins/syncPlay/core/Helper.js

Line 107 in 055a500

export function translateItemsForPlayback(apiClient, items, options) {

But it doesn't seem to support Photos, so I am not sure if we should fix it right now.

src/components/shortcuts.js

grafixeyehero · 2023-09-20T16:41:16Z

@dmitrylyzo thanks for the clarification. the test change is better in my opinion

Null verification for parentid on shortcuts.js itemContextMenu is now obeying to sorting

v0idMrK · 2023-09-20T16:45:59Z

@dmitrylyzo @grafixeyehero Hi!

Implemented the suggestions. Anything else please let me know!

Thank you for the feedback 👍

src/components/itemContextMenu.js

src/components/shortcuts.js

src/controllers/list.js

src/components/playback/playbackmanager.js

src/scripts/settings/userSettings.js

… only play 1 item

UserSettings added param to function comment

Restoring "Play All" functionality

src/components/itemContextMenu.js

grafixeyehero · 2023-09-20T23:54:31Z

@dmitrylyzo it needs your Approval to Thanks👍

v0idMrK · 2023-09-21T17:15:28Z

I agree with the above points.

As soon as I have time I will implement them :)

(Not sure if today or tomorrow)

Thank you all for the feedback!

… with the "Play" action Code cleanup

v0idMrK · 2023-09-23T21:51:52Z

@dmitrylyzo Hey! Just pushed the latest changes 👍

Please let me know if anything else is missing!

Thank you for the feedback.

src/components/playback/playbackmanager.js

src/components/itemContextMenu.js

v0idMrK · 2023-09-27T14:47:15Z

Sorry for not being able to fix this right away. Been busy lately with stuff in life.

Hopefully next week Im able to finish it :)

thornbill · 2023-09-27T16:51:41Z

no worries @v0idMrK whenever you are able to get to it will be fine!

…lists Revert line change on playbackmanager.js

v0idMrK · 2023-10-02T22:27:33Z

@dmitrylyzo Finally got to it after a while :D

Please let me know if there is anything else missing!

Thank you!

dmitrylyzo

Almost good.

src/scripts/settings/userSettings.js

jellyfin-bot · 2023-10-05T13:02:17Z

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

dmitrylyzo

LGTM
Need to update the PR title to suit the changelog.

Found another regression: SortMenu doesn't restore current settings because it expects a PascalCase but receives camelCase.

jellyfin-web/src/components/sortmenu/sortmenu.js

Lines 21 to 22 in c8bed05

    
           context.querySelector('.selectSortOrder').value = settings.SortOrder; 
        
           context.querySelector('.selectSortBy').value = settings.SortBy;

jellyfin-web/src/controllers/list.js

Lines 964 to 965 in c8bed05

    
           sortBy: userSettings.getFilter(basekey + '-sortby') || this.getDefaultSortBy(), 
        
           sortOrder: userSettings.getFilter(basekey + '-sortorder') === 'Descending' ? 'Descending' : 'Ascending'

This should be fixed in a separate PR. Maybe just use PascalCase for SortBy and SortOrder everywhere (list, shortcuts, etc.).

src/components/playback/playbackmanager.js

jellyfin-bot · 2023-10-12T04:26:56Z

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

grafixeyehero · 2023-10-13T01:15:23Z

LGTM Need to update the PR title to suit the changelog.

Found another regression: SortMenu doesn't restore current settings because it expects a PascalCase but receives camelCase.

jellyfin-web/src/components/sortmenu/sortmenu.js

Lines 21 to 22 in c8bed05

context.querySelector('.selectSortOrder').value = settings.SortOrder;

context.querySelector('.selectSortBy').value = settings.SortBy;

jellyfin-web/src/controllers/list.js

Lines 964 to 965 in c8bed05

sortBy: userSettings.getFilter(basekey + '-sortby') || this.getDefaultSortBy(),

sortOrder: userSettings.getFilter(basekey + '-sortorder') === 'Descending' ? 'Descending' : 'Ascending'

This should be fixed in a separate PR. Maybe just use PascalCase for SortBy and SortOrder everywhere (list, shortcuts, etc.).

@dmitrylyzo #4875

v0idMrK · 2023-10-16T11:51:53Z

@thornbill @dmitrylyzo

Hey! Sorry still didnt have time!

When I do I will fix it, just wanted you to know that I didnt forget

v0idMrK · 2023-10-16T20:15:44Z

@thornbill @dmitrylyzo

Could you please check it out when you find the time 😃

Thank you !

sonarcloud · 2023-10-16T20:16:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

jellyfin-bot · 2023-10-18T17:29:47Z

Cloudflare Pages deployment

Latest commit	`298aa01`
Status	✅ Deployed!
Preview URL	https://36f21002.jellyfin-web.pages.dev
Type	🔀 Preview

View build logs
View bot logs

Fixing bug jellyfin#5584 by sending the proper Sort params to the API

6cfcf36

v0idMrK requested a review from a team as a code owner September 17, 2023 22:30

v0idMrK added 2 commits September 17, 2023 23:33

Updated CONTRIBUTORS.md

344a6bf

Removing bad trailing space

ecba6dc

dmitrylyzo reviewed Sep 18, 2023

View reviewed changes

CONTRIBUTORS.md Outdated Show resolved Hide resolved

src/components/shortcuts.js Show resolved Hide resolved

src/components/playback/playbackmanager.js Outdated Show resolved Hide resolved

Contributors file updated correctly

f922742

Adds shortcuts.js function to get sort order Added sort order technique to most types of media on playbackmanager.js

dmitrylyzo reviewed Sep 19, 2023

View reviewed changes

src/components/shortcuts.js Outdated Show resolved Hide resolved

src/components/shortcuts.js Outdated Show resolved Hide resolved

Added getSortOptions to userSettings.js and cleaned up the shortcuts.…

580ad5f

…js and list.js to use the above method

dmitrylyzo reviewed Sep 19, 2023

View reviewed changes

src/controllers/list.js Show resolved Hide resolved

dmitrylyzo reviewed Sep 20, 2023

View reviewed changes

src/components/shortcuts.js Outdated Show resolved Hide resolved

Changed from using sortOptions to using already implemented queryOptions

ee791f9

Null verification for parentid on shortcuts.js itemContextMenu is now obeying to sorting

grafixeyehero requested changes Sep 20, 2023

View reviewed changes

src/components/itemContextMenu.js Outdated Show resolved Hide resolved

src/components/shortcuts.js Outdated Show resolved Hide resolved

src/controllers/list.js Outdated Show resolved Hide resolved

dmitrylyzo reviewed Sep 20, 2023

View reviewed changes

src/components/playback/playbackmanager.js Outdated Show resolved Hide resolved

dmitrylyzo reviewed Sep 20, 2023

View reviewed changes

src/scripts/settings/userSettings.js Show resolved Hide resolved

v0idMrK added 3 commits September 20, 2023 22:50

Removed unneeded calls to getSortValues on playbackManager calls that…

4488a58

… only play 1 item

Added queryOptions to MusicGenre call on playbackmanager

af61ae0

UserSettings added param to function comment

Code cleanup

ef8d923

Restoring "Play All" functionality

v0idMrK requested a review from grafixeyehero September 20, 2023 22:24

grafixeyehero requested changes Sep 20, 2023

View reviewed changes

src/components/itemContextMenu.js Outdated Show resolved Hide resolved

Removed unused imports

c935ba9

v0idMrK requested a review from grafixeyehero September 20, 2023 23:48

grafixeyehero approved these changes Sep 20, 2023

View reviewed changes

Reverting itemContextMenu changes to support folder/playlist ordering…

5d20523

… with the "Play" action Code cleanup

dmitrylyzo reviewed Sep 23, 2023

View reviewed changes

src/components/playback/playbackmanager.js Outdated Show resolved Hide resolved

src/components/itemContextMenu.js Outdated Show resolved Hide resolved

src/components/itemContextMenu.js Outdated Show resolved Hide resolved

src/components/itemContextMenu.js Outdated Show resolved Hide resolved

itemContextMenu only supports folders and using parentId instead for …

d62a36a

…lists Revert line change on playbackmanager.js

dmitrylyzo reviewed Oct 4, 2023

View reviewed changes

src/scripts/settings/userSettings.js Outdated Show resolved Hide resolved

jellyfin-bot added the merge conflict Conflicts prevent merging label Oct 5, 2023

v0idMrK added 2 commits October 5, 2023 22:49

Removed unused method

ad5bcb6

Merge branch 'master' into Sorting-Only-Thumbnail-Fix-5584

0f8d29a

jellyfin-bot removed the merge conflict Conflicts prevent merging label Oct 5, 2023

dmitrylyzo approved these changes Oct 6, 2023

View reviewed changes

thornbill reviewed Oct 7, 2023

View reviewed changes

src/components/playback/playbackmanager.js Outdated Show resolved Hide resolved

thornbill added the bug Something isn't working label Oct 11, 2023

jellyfin-bot added the merge conflict Conflicts prevent merging label Oct 12, 2023

v0idMrK added 2 commits October 16, 2023 21:02

Using lodash implementation to merge playback queries

b33d927

Merge branch 'master' into Sorting-Only-Thumbnail-Fix-5584

fa40c3f

jellyfin-bot removed the merge conflict Conflicts prevent merging label Oct 16, 2023

thornbill approved these changes Oct 18, 2023

View reviewed changes

thornbill changed the title ~~Fix #5584 by sending the proper Sort params to the API~~ Add sort options to playback requests Oct 18, 2023

thornbill merged commit edd9405 into jellyfin:master Oct 18, 2023
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sort options to playback requests #4786

Add sort options to playback requests #4786

v0idMrK commented Sep 17, 2023 •

edited by dmitrylyzo

Loading

v0idMrK commented Sep 18, 2023

dmitrylyzo left a comment

v0idMrK commented Sep 19, 2023

dmitrylyzo commented Sep 19, 2023

grafixeyehero commented Sep 20, 2023 •

edited

Loading

v0idMrK commented Sep 20, 2023

dmitrylyzo left a comment •

edited

Loading

grafixeyehero commented Sep 20, 2023

v0idMrK commented Sep 20, 2023

grafixeyehero commented Sep 20, 2023

v0idMrK commented Sep 21, 2023

v0idMrK commented Sep 23, 2023

v0idMrK commented Sep 27, 2023

thornbill commented Sep 27, 2023

v0idMrK commented Oct 2, 2023

dmitrylyzo left a comment

jellyfin-bot commented Oct 5, 2023

dmitrylyzo left a comment

jellyfin-bot commented Oct 12, 2023

grafixeyehero commented Oct 13, 2023

v0idMrK commented Oct 16, 2023

v0idMrK commented Oct 16, 2023

sonarcloud bot commented Oct 16, 2023

jellyfin-bot commented Oct 18, 2023

	playbackManager[method]({
	items: [item],
	startPositionTicks: startPosition
	});

	query.SortBy = sortValues.sortBy;
	query.SortOrder = sortValues.sortOrder;

	context.querySelector('.selectSortOrder').value = settings.SortOrder;
	context.querySelector('.selectSortBy').value = settings.SortBy;

	sortBy: userSettings.getFilter(basekey + '-sortby') \|\| this.getDefaultSortBy(),
	sortOrder: userSettings.getFilter(basekey + '-sortorder') === 'Descending' ? 'Descending' : 'Ascending'

Add sort options to playback requests #4786

Add sort options to playback requests #4786

Conversation

v0idMrK commented Sep 17, 2023 • edited by dmitrylyzo Loading

v0idMrK commented Sep 18, 2023

dmitrylyzo left a comment

Choose a reason for hiding this comment

v0idMrK commented Sep 19, 2023

dmitrylyzo commented Sep 19, 2023

grafixeyehero commented Sep 20, 2023 • edited Loading

v0idMrK commented Sep 20, 2023

dmitrylyzo left a comment • edited Loading

Choose a reason for hiding this comment

grafixeyehero commented Sep 20, 2023

v0idMrK commented Sep 20, 2023

grafixeyehero commented Sep 20, 2023

v0idMrK commented Sep 21, 2023

v0idMrK commented Sep 23, 2023

v0idMrK commented Sep 27, 2023

thornbill commented Sep 27, 2023

v0idMrK commented Oct 2, 2023

dmitrylyzo left a comment

Choose a reason for hiding this comment

jellyfin-bot commented Oct 5, 2023

dmitrylyzo left a comment

Choose a reason for hiding this comment

jellyfin-bot commented Oct 12, 2023

grafixeyehero commented Oct 13, 2023

v0idMrK commented Oct 16, 2023

v0idMrK commented Oct 16, 2023

sonarcloud bot commented Oct 16, 2023

jellyfin-bot commented Oct 18, 2023

Cloudflare Pages deployment

v0idMrK commented Sep 17, 2023 •

edited by dmitrylyzo

Loading

grafixeyehero commented Sep 20, 2023 •

edited

Loading

dmitrylyzo left a comment •

edited

Loading