-
Notifications
You must be signed in to change notification settings - Fork 197
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
[Feature Request] Sort playlists in the navigation pane by title rather than playlist id #1083
Comments
Yeah, might make sense. Another option would be to make the order user-defined with drag-and-drop, but that would probably take significantly more effort. This is where the playlists are laid out on the UI: music/templates/partials/navigation.php Lines 40 to 47 in b23a56a
The list music/js/app/services/libraryservice.ts Lines 585 to 587 in b23a56a
This LibraryService might be a good candidate for placing the logic for sorting by name.
Then, care has to be taken also that the ordering remains correct when new lists are created or existing lists renamed. The UI-logic for creation is here: music/js/app/controllers/navigationcontroller.js Lines 319 to 327 in b23a56a
...and for renaming here: music/js/app/controllers/navigationcontroller.js Lines 92 to 99 in b23a56a
|
Would this work? This patch is written against the master branch and compiles error free. But I didn't test it on a live Nextcloud instance.
|
It doesn't quite work when creating new playlists because you are sorting the existing playlists synchronously but the new playlist gets added asynchronously. On the other hand, when renaming the playlist, you reorder the playlists in the asynchronous callback but here it could be made synchronously, to get better responsiveness. But even if this did work, I wouldn't make it exactly like this. This now splits the ordering logic to two controller modules which is suboptimal. LibraryService is the module responsible of alphabetizing many kinds of lists and doing also this reordering there would make sense. There, there´s also the utility function So, what would be now needed, would be one new function music/js/app/services/libraryservice.ts Lines 434 to 449 in b23a56a
|
I think that we should also make such a UI-tweak that after creating or renaming a playlist, the UI would automatically navigate to the said playlist. This would highlight the new/modified playlist in the navigation pane and help the user to keep track of where his playlist went. |
I had the very same concerns. But I found no immediate way to move the renaming functionality inside of
I think the If I move the sorting mechanism exclusively to You are totally correct about the async issues. I didn't realize it at that point.
I'm not sure if renaming is a guaranteed outcome. Eventually identical names should be prevented or the server state changed during the rename and leads to a bad response. |
Also, we now navigate automatically to any newly created or renamed playlist. This way, it will be easier for the user to see, where did her playlist go after giving the name. refs #1083
In case there are enough playlists to make the navigation pane scrollable, the link to the activated view may be off the view port. In such occasions, the navigation pane is now scrolled automatically upon view activation to make the link of the activated view visible. This is useful mostly when the view activation happens as response to some other action than clicking the navigation pane link (say, creating a new playlist or clicking the playing song name on the controls pane to jump to the playing view). refs #1083
That would be an option, but simpler solution is to just add a function call
Fair point but this is kind of separate topic. Our existing solution is such that we just assume the renaming to succeed and the UI state immediately reflects this. In practice, a failure there is so rare occurrence that this doesn't cause major problems. As the name anyway changes on the UI immediately, it would be odd if the sorting happened with a delay. The alternative would be to show a loading indicator there until the server responds with 'success' and only after that rename the playlist on the UI and move it to the correct position. And then we would need a graceful error handling, too, with some error message on the UI. Basically this same topic, but with broader scope, is discussed also in the old issue #764.
There's no technical reason to prevent identical names. And on the other hand, I don't think that user would easily do that by mistake. And even if it was a mistake, it is still easy to revert. So if identical names are what the user wants, then by all means, (s)he can do that. Anyway, I now implemented the sorting the way I suggested above, see the linked commits. So no more pull request needed for that. |
I see that the commit is already on the master branch. Thank you very much! |
This change is now released in Music v1.9.1. |
It would be nice if the playlists would be sorted by their playlist title. Currently they are sorted by their playlist id, which is tied to the creation date.
This makes it difficult to create a structure of playlists e.g. by prefixing.
If you could point me to the file that does the lazy load of the playlists, I could eventually prepare a patch myself.
The text was updated successfully, but these errors were encountered: