Skip to content
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

core: Reset stream title after all title tag changes. #1875

Merged
merged 1 commit into from
Mar 16, 2020

Conversation

kingosticks
Copy link
Member

@kingosticks kingosticks commented Jan 29, 2020

Fixes #1871

PR #1751 introduced a regression where, following a normal gapless
track change, the stream_title was incorrectly set to the previous
track's title.

This still isn't very good as we are still emitting the bogus stream_title_changed event for the previous track's title. That is because we keep any tags we get after about_to_finish fires in pending_tags, and then emit them when the track change completes (on_stream_start fires), even if they were old tags for the previous track.

@kingosticks kingosticks force-pushed the fix/stream-title-update branch 2 times, most recently from 373da18 to e800f59 Compare January 29, 2020 00:51
@codecov
Copy link

codecov bot commented Jan 29, 2020

Codecov Report

Merging #1875 into develop will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #1875      +/-   ##
==========================================
+ Coverage    76.89%   76.9%   +<.01%     
==========================================
  Files           55      55              
  Lines         4666    4667       +1     
==========================================
+ Hits          3588    3589       +1     
  Misses        1078    1078
Impacted Files Coverage Δ
mopidy/core/actor.py 94.59% <100%> (+0.03%) ⬆️

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 292d90b...130ce03. Read the comment docs.

Fixes mopidy#1871

PR mopidy#1751 introduced a regression where, following a normal gapless
track change, the stream_title was incorrectly set to the previous
track's title.
@kingosticks kingosticks self-assigned this Jan 29, 2020
@kingosticks kingosticks added A-audio Area: Audio layer A-core Area: Core layer labels Jan 29, 2020
@kingosticks kingosticks added this to the v3.0.2 milestone Jan 29, 2020
@kingosticks
Copy link
Member Author

I guess we could return if the current track has a file: uri but that would only restrict the issue to non-stream http tracks like gmusic etc. Still need a better way to work out what is a stream and what isn't.

@leaty
Copy link

leaty commented Jan 30, 2020

I've been looking into this a little bit. @kingosticks Does mopidy know what is or isn't a stream at any point, or is that information lost after the backend extension?

@kingosticks
Copy link
Member Author

kingosticks commented Jan 30, 2020 via email

@leaty
Copy link

leaty commented Jan 30, 2020

I figured. If we did that, I'm assuming extensions would have to start providing it for the ultimate fix to work. We could still allow them to function until they do though, by keeping this hack for a while. Anyway I approve this change.

@kingosticks
Copy link
Member Author

I think we'd always want to reset the stream_title, before then deciding if we actually want to actually set it and emit the event (which we should only do when Track.is_stream). It's maybe less of a hack than it first looks!

Copy link
Member

@jodal jodal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 from me, but I'd like @adamcik to have a look too.

Copy link
Member

@adamcik adamcik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is OK, but would file a bug to followup. The bug might still get ignored, but less so than just a TODO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-audio Area: Audio layer A-core Area: Core layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mopidy randomly brings over the previous track title to the next track
4 participants