tehkillerbee / mopidy-tidal

Tidal Backend plugin for Mopidy
Apache License 2.0
93 stars 28 forks source link

Playlist modification? #86

Open 2e0byo opened 2 years ago

2e0byo commented 2 years ago

Currently the code will just add all additions to the end of the upstream playlist.

Does this break any clients? Would it be better to do the naive pop-and-push algorithm, removing all tracks down to the first added track and then adding them all back in? It would be slow, but the user client would find it doing what it expected. This would also allow implementing moving tracks, although it's probably better to PR proper support into tidalapi, but this would do as a stopgap.

Should we implement this?

(Sorry to have missed the discussion on the pr where this really belonged.)

@BlackLight (with apologies for being behind the game) @tehkillerbee

tehkillerbee commented 2 years ago

@2e0byo No problem, I quickly merged it in when I realized that the main issue was caused by Iris and not an issue with the PR itself. But perhaps, I should've tested further ;)

I am not sure how much work is needed to support this in tidalapi. But instead of making a workaround in mopidy-tidal, it might be a good idea to investigate and add support directly in tidalapi if it is possible somehow.

2e0byo commented 2 years ago

I've opened an issue for tidalapi. To clarify the current logic is fine (minus one bug squashed in my pr). When I'm next stuck on a train I'll have a look at what the client does, unless someone's done it before then.

blacklight commented 2 years ago

@2e0byo it shouldn't break any clients that I know of, because by the end of the save method we force a fetch for the whole playlist, so unless the client keeps an out-of-date cache after removal (and, AFAIK, no clients should do that), or it infers the changes without synchronizing back with the server, the new snapshot should be applied.

I know that the "always append to the end" logic is far from ideal, but that's what the API currently exposes, and all the clients I know of (tested with ncmpcpp, M.A.L.P, Iris and mopidy-mobile) have an "add to playlist" option, not an "add at this position", so at least functionally speaking there should be no issues (I'm not sure if some clients supports drag&drop of tracks into specific positions, but none of those I know supports it).

So the issue only applies to clients that directly use backend messages to perform changes to playlists (via mpd or mopidy), but that can probably be documented.

The pop-and-push logic would be wayyyyy to expensive, especially in the case of large playlists, but it's probably alright if we keep in mind that all the UI clients out there don't support the "add in the middle", so the pop-and-push logic won't be executed unless somebody does something fancy with custom scripts.

I also think however that these issues should be addressed on the tidalapi side, and also support tracks swap (not currently supported). I'm pretty sure that Tidal provides these endpoints, but they haven't been implemented on the tidalapi side. We only have append to the end and remove by index/URI. I'm not sure if it's worth implementing workarounds on this projects for use-cases that should be covered by the API.

2e0byo commented 2 years ago

@BlackLight thanks for clarifying that re. refresh. For some reason it wasn't obvious to me last night what that refresh was doing. I'll add a comment in pr in case anyone else is similarly tired.

Iris does support drag-to-reorder, but it currently fails with correctly with tidal IIRC. This just calls playlist.save with all the new tracks, so if we used reordering logic we'd need to detect moved lines. Perhaps an algorithm something like (pseudocode):

def invert_action(line: str):
    action = line[0]
    invert = "+" if action == "-" else "-"
    return action + line[1:]

not_moves = set(l for l in difflib_output if invert_action(l) not in difflib_output)
same_logic_as_before_for_additions_removals(not_moves)
new_uris = [tr.uri for tr in new]
for move in set(difflib_output) - not_moves:
    playlist.move(get_id(move), new_uris.index(get_uri(move))

However if as I hope the tidal endpoints actually support bulk actions, we can probably just ping the whole playlist at them and let them do the hard work. If that's so we'll have to decide which is preferable.

In any case the consensus is to push this on the tidalapi side then, which makes sense. When I get a mo I'll spin up a mitm again and have a look at what it does. Or did you use the in-browser client? At one point that used a different api, but I think tidalapi has moved over to it now?

In any case we had no playlist support before this was merged and it's great to have it!