mopidy / mopidy-gmusic

DEPRECATED (Mopidy extension for playing music from Google Play Music)
https://mopidy.com
Apache License 2.0
214 stars 60 forks source link

Enable download buffering to improve playback performance #238

Closed jjok closed 3 years ago

jjok commented 4 years ago

Fixes #161 Fixes #160

Relies on mopidy/mopidy#1888

codecov[bot] commented 4 years ago

Codecov Report

Merging #238 into master will decrease coverage by 0.02%. The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
- Coverage   60.42%   60.40%   -0.03%     
==========================================
  Files           9        9              
  Lines         748      750       +2     
==========================================
+ Hits          452      453       +1     
- Misses        296      297       +1     
Impacted Files Coverage Δ
mopidy_gmusic/playback.py 92.30% <50.00%> (-7.70%) :arrow_down:

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 a4a7846...e049565. Read the comment docs.

belak commented 4 years ago

Does this mean we should bump the required modify version? Thanks for pushing this across the finish line btw!

jjok commented 4 years ago

Oh, I hadn't thought of that. It's not actually a requirement. It's just that this new method won't mean anything with an older version. I guess it's required of you want this extension to work well, but we'd have to wait for a Mopidy release before merging this. What do you think?

kingosticks commented 4 years ago

FYI I'm just adding the uri parameter to should_download, might as well get that in there now.

jjok commented 4 years ago

Ok. Is that just so it has the same interface as is_live() or can you see a situation where the method might need to do some logic based on the uri?

kingosticks commented 4 years ago

Mostly the second. There might be a day when we tell this workaround isn't required for certain tracks, or where we want to avoid doing it for really long tracks. I see no reason not to pass this extra information in and make the API more flexible, the backend does not have to use it.

jjok commented 4 years ago

I'll add that param here too then, yeah?

kingosticks commented 4 years ago

I'll add that param here too then, yeah?

Sorry, yes. We've merged https://github.com/mopidy/mopidy/pull/1907 and added the new API to the v3.1 milestone, so that's the new minimum Mopidy version you must target if using should_download. There's only currently one other thing to finish off in v3.1 before releasing so hopefully it won't be long.

jodal commented 3 years ago

Closing because Google Play Music has been shut down, and this project is being discontinued.