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

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

Closed
Zomtir opened this issue Aug 20, 2023 · 8 comments

Comments

@Zomtir
Copy link

Zomtir commented Aug 20, 2023

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
Copy link
Collaborator

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:

<li navigation-item
playlist="playlist" text="playlist.name" destination="'#/playlist/' + playlist.id"
ng-repeat="playlist in playlists"
ui-on-drop="dropOnPlaylist($data, playlist)"
drop-validate="allowDrop(playlist, $data)"
drag-hover-class="drag-hover"
title="{{ trackCountText(playlist) }}"
icon="'playlist'"></li>

The list playlists referred there comes from here:

getAllPlaylists() : Playlist[] {
return this.#playlists;
}

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:

function createPlaylist(name, trackIds) {
const args = {
name: name,
trackIds: trackIds.join(',')
};
Restangular.all('playlists').post(args).then(function(playlist) {
libraryService.addPlaylist(playlist);
});
}

...and for renaming here:

$scope.commitEdit = function(playlist) {
if (playlist.name.length > 0) {
Restangular.one('playlists', playlist.id).customPUT({name: playlist.name}).then(function (result) {
playlist.updated = result.updated;
});
$scope.showEditForm = null;
}
};

@Zomtir
Copy link
Author

Zomtir commented Aug 24, 2023

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
Copy link
Collaborator

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:

setRadioStations(radioStationsData : any[]) : void {
this.#radioStations = _.map(radioStationsData, (station) => this.#wrapRadioStation(station));
this.sortRadioStations();
}
sortRadioStations() : void {
this.#sortByPlaylistEntryTextField(this.#radioStations, 'stream_url');
this.#sortByPlaylistEntryTextField(this.#radioStations, 'name');
}
addRadioStation(radioStationData : any) : void {
this.addRadioStations([radioStationData]);
}
addRadioStations(radioStationsData : any) : void {
let newStations = _.map(radioStationsData, (station) => this.#wrapRadioStation(station))
this.#radioStations = this.#radioStations.concat(newStations);
this.sortRadioStations();
}

@paulijar
Copy link
Collaborator

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
Copy link
Author

Zomtir commented Aug 25, 2023

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 added a commit that referenced this issue Aug 26, 2023
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
paulijar added a commit that referenced this issue Aug 26, 2023
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
@paulijar
Copy link
Collaborator

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 #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
Copy link
Author

Zomtir commented Aug 27, 2023

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

@Zomtir Zomtir closed this as completed Aug 27, 2023
@paulijar
Copy link
Collaborator

paulijar commented Oct 9, 2023

This change is now released in Music v1.9.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants