mopidy-spotify icon indicating copy to clipboard operation
mopidy-spotify copied to clipboard

Support for playlist management (creation and tracks add/rm/swap).

Open blacklight opened this issue 5 years ago • 33 comments

MPD isn't that optimal in managing playlist changes - it sends the whole snapshot of the new playlist to the save method, while the Spotify Web API only requires the track changes to be submitted. It means that we have to build the deltas within the save method itself. This implementation gets the job done but I'm sure that it could be further optimized.

Known limitations:

  • In order to make the delta calculation a bit more optimized the old and new playlists are currently indexed by track uri. It means that things can get messy if a playlist has duplicate track URIs - if a user removes an instance of a track that appears twice in the playlist then both the instances will be removed, if a user adds a track to a playlist where the track is already present then no changes will be calculated (this isn't such a bad thing IMHO, even the Spotify frontend asks for user confirmation if a track is added to a playlist where it's already present).

  • The current implementation won't allow the invalidation of single cache items. It means that we have to call self._backend._web_client.clear_cache() and invalidate the whole cache even if we have changed only one playlist.

  • The save method doesn't look that good now, as it handles removals, additions and swaps within the same block and progressively updates the cur_tracks map. I believe however that it can be split into three distinct rm, add and swap methods: all the MPD clients I know allow user interactions that fall into only one of those categories, not "bulk edits" that require a whole playlist to be updated through multiple rm/add/swap at the same time.

  • Tests should be added.

blacklight avatar Dec 12 '19 19:12 blacklight

Codecov Report

Merging #236 into master will decrease coverage by 5.18%. The diff coverage is 16.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
- Coverage   96.56%   91.37%   -5.19%     
==========================================
  Files          13       13              
  Lines        1136     1183      +47     
==========================================
- Hits         1097     1081      -16     
- Misses         39      102      +63
Impacted Files Coverage Δ
mopidy_spotify/web.py 93.19% <72.72%> (-2.89%) :arrow_down:
mopidy_spotify/playlists.py 52.81% <8.33%> (-41.71%) :arrow_down:
mopidy_spotify/library.py 100% <0%> (ø) :arrow_up:
mopidy_spotify/translator.py 98.76% <0%> (+2.83%) :arrow_up:
mopidy_spotify/browse.py 93.93% <0%> (+4.65%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 16de1e5...6d567ab. Read the comment docs.

codecov[bot] avatar Dec 12 '19 19:12 codecov[bot]

That same argument can be applied to what we have here. If something goes wrong in the remove step or the add step then you've got a mess left behind. And you still need to batch these requests up into chunks of 100 tracks.

If the playlist is 100 tracks or less its one replace call and done, regardless of the operations performed.

You've already got a local copy of the original playlist so if something goes majorly wrong you could then restore the original from that.

The m3u backend replaces the entire playlist. It's by far the simplest and most robust approach, I do appreciate it's not as easy here.

kingosticks avatar Dec 14 '19 11:12 kingosticks

That same argument can be applied to what we have here. If something goes wrong in the remove step or the add step then you've got a mess left behind. And you still need to batch these requests up into chunks of 100 tracks.

The difference is that with the current approach the mess only impacts the newly added/removed/swapped tracks, not the entire playlist.

If the playlist is 100 tracks or less its one replace call and done, regardless of the operations performed.

True, but most of my playlists, for instance, have more than 100 tracks. I also have playlists with >1000 tracks and one with 1500 tracks. In the latter case adding a single track would result in 17 API calls (one for clearing the playlist, 15 to add tracks in chunks of 100 each, and one more to refresh the playlist), and if anything goes wrong in the process the revert will also cost 17 calls. The latency and the risk of hitting the web API rate limits will both be high in this case.

I had even thought of sacrificing the performance gain introduced by the URI-based indexing to be more consistent when it comes to duplicate tracks, but an inner loop to check the changes in the two playlists position-by-position would mean an O(N²) nightmare - 1,000,000 iterations for a 1000 items playlist, the latency and load may really be too high in this case.

Eventually, I chose the current approach because it seemed by far the most efficient, and maybe we could live with the inconsistent behaviour when it comes to duplicate tracks, as long as it's properly documented. And I think we are kind of consistent anyway. The current implementation won't allow addition of duplicate tracks either, and the Spotify apps also show a confirmation message to prevent duplicates from being added.

You've already got a local copy of the original playlist so if something goes majorly wrong you could then restore the original from that.

A rollback try/except logic should indeed wrap the whole save method. My only concern with this approach would be the possibility of a nasty feedback loop if the API calls failed because of a number of calls above the rate limit: in this case, the rollback logic will make even more API calls, resulting either in the limit being broken again and again, or in the playlist tracks being lost. We could still check through the exception message whether the failure was due to rate limit and prevent further calls, but I'm unsure about what should be done in that case - spawn a thread that retries 5 minutes later, or leave the mess in place and advise the user to head to the web client to restore the playlist?

blacklight avatar Dec 14 '19 14:12 blacklight

I also have playlists with >1000 tracks and one with 1500 tracks. In the latter case adding a single track would result in 17 API calls (one for clearing the playlist, 15 to add tracks in chunks of 100 each, and one more to refresh the playlist),

I agree that's not acceptable and what's why we'd need to detect these simpler add/remove cases and do something more sensible. Which, in this example, would be a single replace request at the playlist end and a refresh (which I think is always going to be unavoidable so we can ignore it).

Now consider adding new tracks sparsely, in the very worst case consider adding a track after every other track. For your 1500 track playlist that's 750 API requests. Less contrived versions are possible in real-world usuage. I appreciate it might not be how you edit your playlists but we need to consider the broader cases, up to a realistic point.

And for the record, I don't think you can clear a large playlist with a single API call as you are still restricted by the 100 track limit when removing. But you don't need to clear it anyway so that's moot.


Ultimately I think we need to think about this more. And we definitely need to have it soak before including it in a release. I'm going to concentrate on our Mopidy 3 release (including merging parts of Mopidy-Spotify-Web into Mopidy-Spotify) as that's comming up next week. Once that's out of the way I, and hopefully also others, will come back to this.

kingosticks avatar Dec 16 '19 12:12 kingosticks

Now consider adding new tracks sparsely, in the very worst case consider adding a track after every other track. For your 1500 track playlist that's 750 API requests. Less contrived versions are possible in real-world usage. I appreciate it might not be how you edit your playlists but we need to consider the broader cases, up to a realistic point.

Which clients support such a way of inserting tracks in a playlist? I agree that we have to consider the broadest use cases, but none of the MPD/mopidy clients I know of support things like sparse add/remove/swap on the same playlist within the same action. I'm not even sure if any of the clients out there support add/remove/swap within the same atomic action, so anyway the save method will only perform one of the three operations when invoked. Of course it's still technically possible to do it by sending a raw MPD message, but I'm pretty confident that 99% of the use cases out there won't fall into this category :)

I agree however that we must find a trade-off point between handling only the delta and clearing and re-populating the whole playlist. As a thumb rule the logic should minimize the number of API calls. It means that if the number of computed non-contiguous addition/removals grouped in batches of 100 is greater than the size of the playlist divided by 100 then we should clear and re-populate the playlist, otherwise do an incremental addition/removal - what do you think?

And for the record, I don't think you can clear a large playlist with a single API call as you are still restricted by the 100 track limit when removing.

Actually the Spotify Web API provides the DELETE https://api.spotify.com/v1/playlists/{playlist_id}/tracks endpoint to clear a playlist within a single call. Anyway, the issue isn't much with clearing the playlist, it's with inserting back the tracks, especially if the playlist is a large one.

blacklight avatar Dec 16 '19 13:12 blacklight

Which clients support such a way of inserting tracks in a playlist?

There's no reason you can't add tracks at whatever position you want to. I've no idea what MPD clients support that as I don't use MPD clients day to day. I had intended musicbox-webclient to allow you to manipulate playlists however you want and save all the modifications in one go.

Actually the Spotify Web API provides the DELETE https://api.spotify.com/v1/playlists/{playlist_id}/tracks endpoint to clear a playlist within a single call.

If you don't sppecify tracks with the DELETE method you get:

{ "error": { "status": 400, "message": "Missing tracks" } }

If you do specify tracks then you are limited to 100 entries. What am I missing?

kingosticks avatar Dec 16 '19 14:12 kingosticks

I had intended musicbox-webclient to allow you to manipulate playlists however you want and save all the modifications in one go.

Got it - that's more a kind of edit-and-commit approach as opposed to the edit-on-the-fly approach implemented by most of the clients I use (ncmpcpp, Iris and MPDroid).

However my approach will also work in these cases - it might be slower if compared to a clear-and-repopulate approach, but if we assume that in most of the cases the user adds and removes a couple of tracks instead of restructuring the entire playlist (is it a correct assumption or am I biased by my own use cases?) it may still be an acceptable trade-off.

If you don't sppecify tracks with the DELETE method you get:

{ "error": { "status": 400, "message": "Missing tracks" } }

You're right, I missed the required part on the tracks attribute...

If that's the case then it's one more argument against the clear-and-repopulate approach: in case of a large playlist the number of calls will really be cumbersome.

What do you think about the trade-off I've proposed in my previous comment?

expected_api_calls_delta = [int(len(block)/100 + 1) for block in contiguous_updates]
expected_api_calls_repopulate = 2 * int(len(playlist.tracks)/100 + 1)

if expected_api_calls_delta < expected_api_calls_repopulate:
    do_delta_update()
else:
    do_clear_and_repopulate()

blacklight avatar Dec 16 '19 15:12 blacklight

You're right, I missed the required part on the tracks attribute...

It's easily done, their documentation is poor. They don't even mention the positions argument unless you use their web console.

Sorry, but I need to come back to this after the 3.x release and get into it properly.

kingosticks avatar Dec 16 '19 15:12 kingosticks

Any chance to resume the discussion about this topic and/or get more developers involved on what's the best strategy to tackle it?

blacklight avatar Jan 16 '20 11:01 blacklight

I, for one, can look again next week.

kingosticks avatar Jan 17 '20 23:01 kingosticks

(Following this with interest as it's funcionality I desperately want)

As far as an MPD client is concerned, the only things it can do with a stored playlist are: Add a track to the end Move a track (or range of tracks) to a new position Remove a track (or range of tracks) Rename the playlist

Each of these is one operation, however MPD has the concept of command lists, where multiple operations can be batched into one atomic operation, so it's perfectly possible for a client to do any combination of the above in one go. 50 commands is the default maximum length of a command list; so 49 adds and one move would be the sort of thing you'd expect to have to handle. If a user tires to add more than 50 tracks, this would result in 50 adds, a write, 50 more adds, and so on.

As far as a client goes, responsiveness is important. As far as the user of a client is concerned, it's everything. 17 API calls for a simple playlist operation is far too slow. Some clients will freeze while they wait for a response, most users will get impatient. In the early days of Rompr it was quite slow at retrieving the play queue, and people complaied because it took about 3 seconds to populate when there were more than 5000 tracks in the queue. I thought that was a ridiculous amout of tracks and therefore hadn't optimised any of the code, but it actually seems to be not unusual. People like big playlists. In my view, anything that can be done to make the process as efficient as possible should be done, or people will moan :)

fatg3erman avatar Jan 24 '20 21:01 fatg3erman

@fatg3erman I totally agree, I also feel that minimizing the number of API calls, by treating as many changes as possible as deltas compared to the current state, should be favoured over the wipe-and-populate approach. And responsiveness should be everything, even if the cost is a loss of consistency in some corner cases like e.g. duplicate IDs within the same playlist in order to implement a hash lookup instead of a linear lookup.

However, I also acknowledge that in some corner cases the wipe-and-populate approach may be more efficient. If I want to remove all the tracks in a playlist and add a single new track then it's faster to do a wipe-all instead of a scan-the-deltas (provided that the client actually supports such a feature within a single atomic call). So my questions are:

  • How much more common are such use cases compared to the addition/removal of n tracks, where n << size of the playlist?

  • Is it worth to implement an alternative wipe-and-populate logic to cover such cases?

  • If so, is it worth to heuristically estimate, before performing the operation, how many API calls that operation will involve both with the scan-the-deltas and wipe-and-populate approach, and opt for the one that minimizes the amount?

Once settled, we've still got the rollback issue to tackle. I believe that if some calls inside save() fail then we should try to get the playlist back to the previous state, but if we already successfully performed some of the required actions we may be in a quite inconsistent state. Worst, if the code failed because the API rate limit exceeded, the restore process will result in even more API calls and in an endless loop, so that case should probably be targeted separately. In the worst case scenario, we may just return a message to the user such as "there was a problem while manipulating your playlist: please restore it from the Spotify web interface".

blacklight avatar Jan 28 '20 11:01 blacklight

@BlackLight: What's the state of this PR? As far as I can see from the comments, minimizing API requests is still to-do? I'm not sure if you and @kingosticks have agreed a plan for this yet or not.

If you aren't interested in working on this anymore, I can pick it up, given a list of outstanding tasks. I've worked a bit on that code (fix some errors, make tests pass, implement deletion, less cache invalidation) at https://github.com/fork-graveyard/mopidy-spotify/tree/bug22, fyi.

ETA: i had a play around with ncmpcpp and cantata; both seem to add/move tracks to playlists one-by-one. this means, that for many clients, the api request optimisation might be a moot point.

ETA2: one more thing I forgot to mention is that the current api scope is too narrow: we're missing playlist-modify-public, meaning we can't modify playlists that have its visibility set to public (we can modify private playlists, though). While at it, we might as well request user-library-modify, to allow modifying "my music" later on.

girst avatar Nov 03 '20 20:11 girst

@girst I believe that we eventually want a reasonable trade-off depending on the case - if the user is adding a single track to a playlist then updating the whole playlist is overkill; if the user is clearing all the tracks in a playlist then doing track-by-track updates is overkill.

I think that we just need to agree on where to draw the dividing line for the cases between these two extremes - the code should probably try and estimate the average-case number of required API calls in both the cases, given the size of the playlist and the number of requested changes, and apply the logic that minimizes such number. However, for large playlists the estimation logic itself can be overkill - the mpd save command only sends the snapshot of the new playlist and diffing doesn't come for free. @kingosticks what do you think?

I don't have much time to work on this PR in this period (I've recently expanded the family and babies seem to require much more maintenance than code), but feel free to pick it up if you have some time.

blacklight avatar Nov 03 '20 23:11 blacklight

regarding the algorithm, i propose the following:

Given the lists of new tracks (playlist.tracks) and old tracks (saved_playlist.tracks), calculate the diff between them (probably using something based on LCS). This gives us a list of block-additions and block-deletions. In a second step, try to pair an addition-block with a deletion-block; this is a block move. finally, split ranges > 100 tracks into chunks.

now that we have a set of instructions on how to transform the playlist, we can check if replacing it outright (in chunks of 100) is cheaper. the spotify api has an endpoint for "clear-and-put-100-tracks", so clear_and_repopulate needs exactly ceil(len(playlist.tracks)/100) operations.

as pseudocode:

operations = diff(saved_playlist.tracks, playlist.tracks)
# operations := { {type:add, tracks:[...]}, {type:del, tracks:[...] }
additions = {op for op in operations if op.type == 'add'}
deletions = {op for op in operations if op.type == 'del'}
moveblock = addition & deletions

patchall   = (additions ^ deletions) | moveblock
replaceall = math.ceil(len(playlist.tracks) / 100)

if replaceall < len(adddelmov):
	clear_and_repopulate(playlist.tracks)
else:
	patch_playlist(adddelmov)

# finally, check if playlist was renamed

i have noticed that both mpd clients i tested (ncmpcpp and cantata) will not use 'command lists'. assuming this is common practise (i belive so), the diffing algorithm will hardly ever do more than insert or remove a single track. hence, as a further optimisation, we could delay 'writing' the playlist to the API by a few seconds to wait for more save() commands. that will require storing the playlist in memory temporarily, and only when no new save commands came in for e.g. 5 seconds, write to the API asynchronously.

On Tue, Nov 03, 2020 at 03:46:05PM -0800, Fabio Manganiello wrote:

(I've recently expanded the family and babies seem to require much more maintenance than code)

congrats!

girst avatar Nov 04 '20 17:11 girst

Command lists are very commonly used. You chose two of the most basic clients for your test.

On 4 Nov 2020, at 17:12, girst [email protected] wrote:

 regarding the algorithm, i propose the following:

Given the lists of new tracks (playlist.tracks) and old tracks (saved_playlist.tracks), calculate the diff between them (probably using something based on LCS). This gives us a list of block-additions and block-deletions. In a second step, try to pair an addition-block with a deletion-block; this is a block move. finally, split ranges > 100 tracks into chunks.

now that we have a set of instructions on how to transform the playlist, we can check if replacing it outright (in chunks of 100) is cheaper. the spotify api has an endpoint for "clear-and-put-100-tracks", so clear_and_repopulate needs exactly ceil(len(playlist.tracks)/100) operations.

as pseudocode:

operations = diff(saved_playlist.tracks, playlist.tracks)

operations := { {type:add, tracks:[...]}, {type:del, tracks:[...] }

additions = {op for op in operations if op.type == 'add'} deletions = {op for op in operations if op.type == 'del'} moveblock = addition & deletions

patchall = (additions ^ deletions) | moveblock replaceall = math.ceil(len(playlist.tracks) / 100)

if replaceall < len(adddelmov): clear_and_repopulate(playlist.tracks) else: patch_playlist(adddelmov)

finally, check if playlist was renamed

i have noticed that both mpd clients i tested (ncmpcpp and cantata) will not use 'command lists'. assuming this is common practise (i belive so), the diffing algorithm will hardly ever do more than insert or remove a single track. hence, as a further optimisation, we could delay 'writing' the playlist to the API by a few seconds to wait for more save() commands. that will require storing the playlist in memory temporarily, and only when no new save commands came in for e.g. 5 seconds, write to the API asynchronously.

On Tue, Nov 03, 2020 at 03:46:05PM -0800, Fabio Manganiello wrote:

(I've recently expanded the family and babies seem to require much more maintenance than code)

congrats! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

fatg3erman avatar Nov 04 '20 17:11 fatg3erman

On Wed, Nov 04, 2020 at 09:48:14AM -0800, Mark Greenwood wrote:

Command lists are very commonly used. You chose two of the most basic clients for your test.

point me to some then, please.

girst avatar Nov 04 '20 17:11 girst

There's this one, for a start

https://fatg3erman.github.io/RompR/

this will, if required, use command lists of up to 50 commands (which is the default setting of MPD)

fatg3erman avatar Nov 04 '20 18:11 fatg3erman

On Wed, Nov 04, 2020 at 10:03:35AM -0800, Mark Greenwood wrote:

There's this one, for a start

https://fatg3erman.github.io/RompR/

thanks! i'll use it to test the proposed algorithm once implemented.

this will, if required, use command lists of up to 50 commands (which is the default setting of MPD)

what are you using command lists for? just adding/deleting multiple tracks, or fully mixing different commands (e.g. add 3 tracks here, 2 there and remove that one)?

girst avatar Nov 04 '20 18:11 girst

I think we covered not assuming one-track-at-a-time changes previously. Regardless of what some MPD clients do (I'm not sure if the current Mopidy-MPD implementation has anything to prevent each stored playlist command in your list performing a save, but I've not verified that), web clients and potentially other frontends allow you to make multiple changes at a time so we need to support that fully.

I think that we just need to agree on where to draw the dividing line for the cases between these two extremes

Sounds good to me. I'd really like to avoid making this crazy complicated. We do have to test this.

And a belated congratulations to you @BlackLight!

kingosticks avatar Nov 04 '20 18:11 kingosticks

Basically I build a list of commands and if it’s longer than 1 I use a command list. But for playlists it would be add a group of tracks to the end and then move them to a position (because MPD doesn’t support adding tracks at a position). Adding 2 here 3 there isn’t possible in one operation through the UI so I wouldn’t do that.

On 4 Nov 2020, at 18:10, girst [email protected] wrote:

 On Wed, Nov 04, 2020 at 10:03:35AM -0800, Mark Greenwood wrote:

There's this one, for a start

https://fatg3erman.github.io/RompR/

thanks! i'll use it to test the proposed algorithm once implemented.

this will, if required, use command lists of up to 50 commands (which is the default setting of MPD)

what are you using command lists for? just adding/deleting multiple tracks, or fully mixing different commands (e.g. add 3 tracks here, 2 there and remove that one)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

fatg3erman avatar Nov 04 '20 18:11 fatg3erman

On Wed, Nov 04, 2020 at 10:27:38AM -0800, Nick Steel wrote:

Regardless of what some MPD clients do [...], web clients and potentially other frontends allow you to make multiple changes at a time so we need to support that fully.

of course. i mentioned the single-track changes only to propose a further optimisation of bundling multiple save()s together.

I think that we just need to agree on where to draw the dividing line for the cases between these two extremes

Sounds good to me. I'd really like to avoid making this crazy complicated. We do have to test this.

girst avatar Nov 04 '20 18:11 girst

On Wed, Nov 04, 2020 at 10:35:01AM -0800, Mark Greenwood wrote:

But for playlists it would be add a group of tracks to the end and then move them to a position

that sounds reasonable and is probably a good fit for my proposed diff-based algorithm. mopidy save() will see one large(ish) insertion of x tracks at offset y, and that will map nicely to one of the available endpoints on spotify's api.

girst avatar Nov 04 '20 18:11 girst

On Wed, Nov 04, 2020 at 09:48:14AM -0800, Mark Greenwood wrote:

Command lists are very commonly used. You chose two of the most basic clients for your test.

i tried rompr, and it too caused seperate requests for each track. so i started digging around in mopidy-mpd's source.

turns out, my mpd clients were not the culprit: mopidy-mpd is issuing a context.core.playlists.save(playlist) for every mpd command -- so it won't matter if one is using command_lists. (see mopidy_mpd/protocol/stored_playlists.py lines 181, 265, 308)

does anyone know of a non-mpd-client that can edit playlists? i tried all the web clients on mopidy.com/ext (mobile, mowecl, muse, musicbox_webclient and iris). the first three can't select more than 1 track to add/move/delete, musicbox can't edit at all and iris talks to the spotify api "behind my back" on its own, so mopidy_spotify is never involved in handling it.

girst avatar Nov 05 '20 15:11 girst

turns out, my mpd clients were not the culprit: mopidy-mpd is issuing a context.core.playlists.save(playlist) for every mpd command

Yes, I mentioned this.

I think it's a bit of a chicken and egg problem. There hasn't so far been a compelling use-case for rich playlist modification using Mopidy's API - m3u playlists are not very exciting - so web clients implementations have so far been basic, if at all. For the record, musicbox-webclient allows you to edit a "favourites" m3u playlist, but that is it.

If you want to test the functionality, write some tests. If you want to test the user experience then we are a bit stuck right now. If you are trying to better understand the status quo in order to support a limited subset of the full Mopidy Playlist API then that is something else and I don't think that's a good idea considering the chicken+egg mentioned. If we have to limit the scope (why?) then we can but we need to reason about that rather than looking at current frontend implementations.

EDIT: And I will also add: Mopidy's playlist API is not set in stone. It could change. If we did an updated survey of backends and what playlist editing support they wanted we could look at improvements to better support non-local backends. I'm not even sure what there is currently other than Spotify that supports playlist modification. My 5 min survey shows the only backend to do any playlist modification is m3u and mopidy_bookmarks!

kingosticks avatar Nov 05 '20 16:11 kingosticks

On Thu, Nov 05, 2020 at 08:31:40AM -0800, Nick Steel wrote:

Yes, [I mentioned this].

sorry, i read right over this :/

I think it's a bit of a chicken and egg problem. There hasn't so far

it's a bit of a downer to implement a feature that in practise can't be used. still, i intend to continue working on this.

If you want to test the functionality, write some tests. If you want to test the user experience then we are a bit stuck right now. If you are trying to better understand the requirements in order to limit the scope then that is something else.

i was just trying to get a feel for my implementation before figuring out how to test stuff. looks like i won't be able to procrastinate on that.

girst avatar Nov 05 '20 17:11 girst

I'm up for attempting to write the feature for musicbox webclient (or maybe just a standalone proof of concept).

kingosticks avatar Nov 05 '20 17:11 kingosticks

And if you can come up with a way for Mopidy-MPD to do something more optimised with a command list of playlist modifications targeting the same playlist then that sounds interesting. My thoughts right now are all massive hacks that we'd never merge (I hope).

kingosticks avatar Nov 05 '20 17:11 kingosticks

I often wonder what on earth saved playlists are for in this era, but "better support for playlist editing" is amongst my most frequently requested features. So I'd definitely say don't artificially limit what you can do just because you can't see a use case for it... someone out there will disagree :)

On 5 Nov 2020, at 17:49, Nick Steel [email protected] wrote:

 And if you can come up with a way for Mopidy-MPD to do something more optimised with a command list of playlist modifications targeting the same playlist then that sounds interesting. My thoughts right now are all massive hacks that we'd never merge (I hope).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

fatg3erman avatar Nov 05 '20 21:11 fatg3erman

i've pushed some more commits to fork-graveyard/mopidy-spotify/bug22.
there is now logic implemented for generating an edit script and applying it, replacing playlists outright, which path to take and the beginnings of a backend mock that can modify the playlists.

The diffing algorithm is essentially Myers diff, with some post-processing to group insertions/deletions and merge them into a move. it lives in mopidy_spotify/utils.py.

On Thu, Nov 05, 2020 at 09:44:51AM -0800, Nick Steel wrote:

I'm up for attempting to write the feature for musicbox webclient (or maybe just a standalone proof of concept).

That'd be great!

On Thu, Nov 05, 2020 at 09:49:06AM -0800, Nick Steel wrote:

And if you can come up with a way for Mopidy-MPD to do something more optimised with a command list of playlist modifications targeting the same playlist then that sounds interesting. My thoughts right now are all massive hacks that we'd never merge (I hope).

I had a go at this yesterday, but didn't get far. My idea was to let command handlers know whether they were executed inside a command_list, and instead of saving the playlist, return a list of which playlists were modified. the command_list_end handler would then read this list and save all the modified playlists at once. but I couldn't figure out how to pass the state in or the list back out.

girst avatar Nov 06 '20 11:11 girst