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

Fix time position in playback ended trigger #674

Merged
merged 2 commits into from Feb 6, 2014

Conversation

3 participants
@Fryie
Contributor

Fryie commented Jan 26, 2014

When using mopidy with the scrobbler plugin, I noticed that my tracks were not being scrobbled on last.fm. Turns out, scrobbling only happens after listening to the track for a certain amount of time. However, the scrobbler listener was always receiving 0 as the current time position which caused it to never scrobble a track.

This was happening because the time position was queried after stopping the track - at which point the time position is reset to 0. Saving the time position before calling stop and passing that value to the listener in the trigger fixes this.

Unfortunately I am no experienced Python developer (I'm a Rubyist :) ) and don't know how to write a test for this. Seems to me this would require a fair amount of mocking. However, I verified that I did not break any existing tests nor have given flake8 cause to complain. I also did manual testing on my machine and now my tracks are being happily scrobbled.

For the reference, I was using mopidy with ncmpcpp and listening to music from spotify, in case this issue is in any way specific to the setup.

edit:
Fixed some style issues :)

Pierpaolo Frasa added some commits Jan 26, 2014

Pierpaolo Frasa
@adamcik

This comment has been minimized.

Member

adamcik commented Jan 30, 2014

Lgtm, good catch. We should get this in, though I will probably change this as I cleanup audio and core events, that is once I get to it :-)

@jodal

This comment has been minimized.

Member

jodal commented Jan 30, 2014

Looks good to me.

@adamcik Feel free to merge if you add a test for it as well.

@jodal jodal modified the milestones: 0.18.2, v0.19 - MPD playlist mgmt and other MPD improvements Feb 6, 2014

jodal added a commit that referenced this pull request Feb 6, 2014

@jodal jodal merged commit b522491 into mopidy:develop Feb 6, 2014

1 check passed

default The Travis CI build passed
Details
@jodal

This comment has been minimized.

Member

jodal commented Feb 6, 2014

I tried testing this myself, and it was really hard and painful, thus lack of test approved.

Thanks for the debugging and patch! :-)

@jodal jodal self-assigned this Feb 6, 2014

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