Support for playlist management (v2)

[girst]

This is now done :D

I've adapted Myers' diffing algorithm (the one that's used by e.g. git-diff) to support moving ranges of tracks. It generates a relatively optimal edit script, to minimize API requests without the diffing algorithm taking too long. Especially getting moves to work was quite tricky (took me over a week of daily head-scratching), but I have written a whole bunch of tests for it. the web_client_mock now also supports editing playlists.

One thing we haven't yet talked about is what to do when API requests fail. The worst case (and only serious one) is when we clear-and-repopulate a playlist. When we fail mid-way through, we lose data. Should we do something (e.g. save an m3u playlist somewhere) then?

What's still todo is:

• [x] figure out what to do on an API error
• [x] extending the authenticator on the website to request additional scopes
• [x] Ignore non-Spotify tracks
• [x] Handle unplayable/unavailable tracks

CC: @BlackLight closes #22, closes #236

[codecov[bot]]

Codecov Report

Merging #287 (1c0358a) into master (66d40fc) will increase coverage by 0.42%. The diff coverage is 97.77%.

@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
+ Coverage   96.68%   97.10%   +0.42%     
==========================================
  Files          13       13              
  Lines        1207     1418     +211     
==========================================
+ Hits         1167     1377     +210     
- Misses         40       41       +1     

Impacted Files	Coverage Δ
mopidy_spotify/browse.py	91.30% <ø> (ø)
mopidy_spotify/playlists.py	97.22% <95.68%> (+2.93%)	:arrow_up:
mopidy_spotify/translator.py	96.35% <100.00%> (+0.01%)	:arrow_up:
mopidy_spotify/utils.py	100.00% <100.00%> (ø)
mopidy_spotify/web.py	96.63% <100.00%> (+0.54%)	: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 66d40fc...1c0358a. Read the comment docs.

[girst]

I found and fixed the problem with the cache (the playlist's tracks are fetched from two endpoints, not just the one I cleared). While at it, I also moved the newly added code to use the new shorter playlist URIs that spotify offers (as the old ones are deprecated). Also started testing this out on my personal spotify account, and it pretty much works.

This leaves us with only one major item to figure out: what do do on errors. For this, I'd like to solicit some ideas from you guys.

[girst]

OK, so I think this is done. I'm going to add @jodal, @adamcik and @kingosticks as potential reviewers. Looking forward to your feedback!

PS: in case you feel initmidated by the patch size: 70% of it is just tests.

edit nov21: just for fun, i did some calculations (on commit 468a811): only 230 lines or 19% of this patch is actual non-test code (excluding comments, closing braces and blank lines):

signontest=$(git diff upstream/master bug22 mopidy_spotify/ | grep -v '^diff\|^index\|^+++\|^---\|^@@\|^ ' | cut -b2- | grep -v '^ *$' | grep -v '^ *#' | grep '^ *\S\{3\}' | wc -l)
allchanges=$(git diff upstream/master bug22 | grep -v '^diff\|^index\|^+++\|^---\|^@@\|^ ' | wc -l)
echo significant non-test changes: $(( 100*signontest / allchanges ))'% (' $signontest lines ')'

# significant non-test changes: 19% (230 lines)

[kingosticks]

Link to the Zulip discussion

[girst]

We currently filter out all unplayable tracks and this means we'll be removing them whenever we make any playlist change, which is wrong.

agreed, that's bad.

I think local files might also be treated as unplayable but I'm not sure if Spotify still supports this feature I never used it.

i don't have any experience here either. looking at the docs, uris look like spotify:local:{artist}:{album_title}:{track_title}:{duration_in_seconds}, so that's something we could simulate.

change the current playlist behaviour to now include unplayable tracks in playlists. info log message (which we will want to adjust).

that seems like a good plan. should we do this in a followup to keep this patch from growing even further?

I've not looked closely at the myers implementation here, I assume it's the normal one that's available everywhere.

yep, pretty much. it has a extra memory optimisation, but that's about it.

But we do have Python's standard difflib.SequenceMatcher, so what does using myers provide over just using that?

I was under the impression that difflib only worked on strings of text, not lists as I used here. Your comment made me look again, and it turns out I was wrong. That said, the interface is pretty different to my version. I'm up for rewriting it, but it'll take me a few days. I think we can continue the review with the current implementation, and have a drop-in replacement for myers() and diff() later on.

I did find multiple (why???) packages for Google's diff-match-patch library.

I'd rather avoid introducing 3rd party dependencies if possible.

At a higher level, some of what has been added to playlist.py belongs in web.py.

I guess, you#re talking about _patch_playlist(), _replace_playlist() and _playlist_edit()? i can move that functionality to web.py.

[girst]

ok, myers diff is gone. porting to difflib turned out easier than expected, once i familiarized myself with it.

[girst]

OK, coming back to the "not filter out unplayable/local tracks" issue: I've pushed a commit, 6667e0e, that should "fix" that. I haven't tested that, as I don't have any unplayable tracks in my playlists AFAIK. Maybe you can have a look at that.

ninja edit: i did add a test for it, of course. ninja edit x2: that test doesn't seem to test the right thing, though; it passes even before that patch is applied. :/ less-ninja-ry edit 3: fixed the test.

[girst]

@kingosticks, sorry for the delay, but spotify:local is now handled (as in "refused"). This should have all your (valuable, btw) input addressed now, no?

[girst]

Urgh, I so wish, python would have had null-coalescing/none-aware operators much earlier, then the whole flagile .get()-dance could be avoided alltogether.

Thanks for finding this; I'll pull from master once your PR is merged.

[girst]

Oh, and if you're feeling adventurous, @kingosticks, i'd be very interested in what happens when you patch out the last commit (refuse with local tracks present) and try to delete a local track (probably wise to do in a dedicated test playlist. note that the web api can't add local tracks, so you'd have to do that outside mopidy). the docs say local tracks "should" be deleted using different arguments to the delete call, but I wonder what would happen when ignoring this advice.

If you don't have the time, no worries, though. If it worked at all, I would if at all implement local track deletion in a seperate PR, not that one.

[kingosticks]

Have merged #290

I'm not sure if the playlist that was causing this was actually mine or just one I follow. But I will have a little play if I get more time. I am going to prioritise a more thorough review of this as it is.

[girst]

On Sat, Dec 05, 2020 at 04:51:07PM -0800, Nick Steel wrote:

Have merged #290

Pulled.

I am going to prioritise a more thorough review of this as it is.

Appreciated, thanks!

[girst]

I hate to be pushy, but do you think you can find some time over the holidays to review this, @kingosticks? Let's end 2020 on a high note (pun intended) ;-)

[kingosticks]

I know! This has been on my todo list for so long, I apologise. Had a busy end to the year but holidays have started now so is time before Christmas. Hopefully we can rope some others in too.

[blacklight]

I've tested the branch and taken a look at the code and it all seems good to me. Can't wait for this feature to be finally merged - and thanks @girst for picking it up!

[girst]

first off, thanks for the huge review! i've applied most of your suggested fixes (pending a black/flake pass), but i'm running out of steam today for the rest.

regarding your initial message:

We normally do not go for the style of mocking out the API functionality like this. I understand it results in less boilerplate/repetition in the actual tests but it's more complex and I think we should stick to the Responses-based approach we've used so far.

There are quite a lot of tests verifying the correctness of the diff calculation (and every one of them is testing some other potential bug); generating Responses for each of them seems very tedious. and given that the main point in those tests is verifying that the diffing works, won't be very useful either. that said, i'm fine with someone else writing some extra tests using Responses.

I am also curious as to why the playlist tests run so slowly.

calculating the diff, many times over for each test.

I will push some commits to your branch (I think that is probably the best way) over the next couple of days.

please do!

---

edit: wow, getting flake8 to pass was a pain. i can't reproduce the black changes locally, and the ci won't give me diff output :|

[kingosticks]

There are quite a lot of tests verifying the correctness of the diff calculation (and every one of them is testing some other potential bug); generating Responses for each of them seems very tedious. and given that the main point in those tests is verifying that the diffing works, won't be very useful either.

I agree, responses at this level would be unpleasant and also not useful. But testing multiple things isn't ideal. I think we can restructure a little and test at lower levels to avoid writing too many Responses and hopefully also avoid a Python version of the Web API. It may end up being more boilerplate but I'll have a go and see where I land. Either way, doing this will probably help me review.

[kingosticks]

Sorry, I did a few bits, hit a small issue, managed to context switch along the way and lost my momentum. Would you mind rebasing on master when you get a spare moment.

[girst]

On Sun, Jan 10, 2021 at 09:25:49AM -0800, Nick Steel wrote:

Sorry, I did a few bits, hit a small issue, managed to context switch along the way and lost my momentum. Would you mind rebasing on master when you get a spare moment.

thanks for those "few bits!" if there's anything i can do to get you back up-and-running, shout now ;)

pulled your changes in (and propagated them to the new endpoints).

[kingosticks]

I've got another review in progress but it's not quite done yet. Probably less work for you if it's all at once.

[blacklight]

Hey guys, what's the current status with this PR?

[kingosticks]

It's not yet merged. It's waiting on me, it's my fault. I've been meaning to dig this out for weeks and I have only a couple of days before I'm unavailable for a while. Nothing motivates like an impending deadline.

[girst]

I'm still interested to get this merged; it's just that since getting it to work for myself, the pressure is off. also, work has been a bit more time and energy consuming, in the last couple of months (although i expect that to be over relatively soon).

On Mon, May 03, 2021 at 02:57:04PM -0700, Nick Steel wrote:

It's not yet merged. It's waiting on me, it's my fault.

don't beat yourself up over it. we're all doing this as a hobby! if you don't have the time, or just plain don't feel like working on it, there are no hard feelings from me :)

[kingosticks]

We've all spent a lot of time on this and it deserves to be finished; I think in your shoes I might have been quite annoyed so I mainly want to say I really appreciate the patience and understanding here. I did lose momentum and focus on this, and have also scaled back how much time I invest in this hobby but this kind response is exactly the motivation I need. Having said that, I'm about to take on an enormous priority shift (complete with a couple of weeks off work and zero free time) so while I would love to say I'll properly get back into this right now but I'm not sure that's remotely feasible for a few weeks.

[dsynkd]

@kingosticks any chance we could get this approved?

[kingosticks]

I spent time reviewing this and reworking the tests to fit the way we had done things previously - I don't want to maintain the code as it is. I have that local branch somewhere, I have been meaning to pick it up for the last 10 months I just have not had the time to invest and get back into it. So yes, there is a chance to merge this in a slightly different form. I should publish that branch and then someone can finish it off rather than wait any longer.

[girst]

On Tue, Mar 22, 2022 at 10:00:44AM -0700, Nick Steel wrote:

I spent time reviewing this and reworking the tests to fit the way we had done things previously - I don't want to maintain the code as it is.

if you give me some direction on what you want done differently, i'd be happy to work on it again :)