[Feature Request] Sort playlists in the navigation pane by title rather than playlist id

[Zomtir]

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.

[paulijar]

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: https://github.com/owncloud/music/blob/b23a56a16b4eaced53b4a882a0688672c05b0671/templates/partials/navigation.php#L40-L47

The list playlists referred there comes from here: https://github.com/owncloud/music/blob/b23a56a16b4eaced53b4a882a0688672c05b0671/js/app/services/libraryservice.ts#L585-L587 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: https://github.com/owncloud/music/blob/b23a56a16b4eaced53b4a882a0688672c05b0671/js/app/controllers/navigationcontroller.js#L319-L327

...and for renaming here: https://github.com/owncloud/music/blob/b23a56a16b4eaced53b4a882a0688672c05b0671/js/app/controllers/navigationcontroller.js#L92-L99

[Zomtir]

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.

diff --git a/js/app/controllers/maincontroller.js b/js/app/controllers/maincontroller.js
index 7fb2efc0..0f9d9346 100644
--- a/js/app/controllers/maincontroller.js
+++ b/js/app/controllers/maincontroller.js
@@ -140,6 +140,7 @@ function ($rootScope, $scope, $timeout, $window, ArtistFactory,
            Restangular.all('playlists').getList().then(function(playlists) {
                libraryService.setPlaylists(playlists);
                $scope.playlists = libraryService.getAllPlaylists();
+               $scope.playlists.sort((a, b) => a.name.localeCompare(b.name));
                $rootScope.$emit('playlistsLoaded');
            });

diff --git a/js/app/controllers/navigationcontroller.js b/js/app/controllers/navigationcontroller.js
index 66d56748..1be06a34 100644
--- a/js/app/controllers/navigationcontroller.js
+++ b/js/app/controllers/navigationcontroller.js
@@ -49,6 +49,7 @@ angular.module('Music').controller('NavigationController', [
            if ($scope.newPlaylistName.length > 0) {
                createPlaylist($scope.newPlaylistName, $scope.newPlaylistTrackIds);
                $scope.closeCreate();
+               $scope.playlists.sort((a, b) => a.name.localeCompare(b.name));
            }
        };

@@ -93,6 +94,7 @@ angular.module('Music').controller('NavigationController', [
            if (playlist.name.length > 0) {
                Restangular.one('playlists', playlist.id).customPUT({name: playlist.name}).then(function (result) {
                    playlist.updated = result.updated;
+                   $scope.playlists.sort((a, b) => a.name.localeCompare(b.name));
                });
                $scope.showEditForm = null;
            }

[paulijar]

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 LibraryService.#sortByTextField() which could be utilized. It does locale-specific sorting using the locale selected in the cloud user settings, instead of using the browser language setting.

So, what would be now needed, would be one new function LibraryService.sortPlaylists() which would be called from NavigationController upon playlist rename. In addition, the same function could be called internally in LibraryService.setPlaylists() and LibraryService.addPlaylist(). This is basically the same solution as we use with the radio stations: https://github.com/owncloud/music/blob/b23a56a16b4eaced53b4a882a0688672c05b0671/js/app/services/libraryservice.ts#L434-L449

[paulijar]

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.

[Zomtir]

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.

I had the very same concerns. But I found no immediate way to move the renaming functionality inside of libraryservice.ts, where it could also make use of the sorting. The renaming is spread across app/controllers/navigationcontroller.js and templates/partials/navigationitem.php:

<input type="text" class="edit-list" maxlength="256" on-enter="$parent.commitEdit(playlist)" ng-model="playlist.name"/>

I think the ng-model="playlist.name" directly updates the local and remote object. I am not very familiar with Angular unfortunately.

If I move the sorting mechanism exclusively to libraryservice.ts, wouldn't this also suggest to move the renaming functionality there (libraryService.renamePlaylist())?

You are totally correct about the async issues. I didn't realize it at that point.

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.

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.

[paulijar]

If I move the sorting mechanism exclusively to libraryservice.ts, wouldn't this also suggest to move the renaming functionality there (libraryService.renamePlaylist())?

That would be an option, but simpler solution is to just add a function call libraryService.sortPlaylists() to the function commitEdit(playlist) in navigationcontroller.js. And this is similar to how we were already handling the sorting of radio stations.

I'm not sure if renaming is a guaranteed outcome.

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 https://github.com/owncloud/music/issues/764.

Eventually identical names should be prevented

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.

[Zomtir]

I see that the commit is already on the master branch. Thank you very much!

[paulijar]

This change is now released in Music v1.9.1.