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

emit stream_title_changed when title differs from track name #1751

Merged
merged 1 commit into from Sep 25, 2019

Conversation

@kingosticks
Copy link
Member

kingosticks commented Mar 12, 2019

I've been using this change quite happily and seems worth it until we have proper stream type support.

@jodal
jodal approved these changes Jun 9, 2019
Copy link
Member

jodal left a comment

LGTM.

Wondering if we should allow this in 2.2.3 or let it wait for 3.0...

Any comments, @adamcik?

@adamcik

This comment has been minimized.

Copy link
Member

adamcik commented Jun 10, 2019

Either way, maybe just a point or minor release since it's been ages since there has been a release. So just put small things like this there and get whatever it adds up to out,

@jodal

This comment has been minimized.

Copy link
Member

jodal commented Jun 10, 2019

Any comments on the change itself?

@adamcik

This comment has been minimized.

Copy link
Member

adamcik commented Jun 10, 2019

Nope, not really.

@jodal jodal added this to the v2.2.3 milestone Jun 11, 2019
@jodal jodal modified the milestones: v2.2.3, v2.2.4 Jun 19, 2019
self.playback._stream_title = title
CoreListener.send('stream_title_changed', title=title)
current_track = self.playback.get_current_track()
if current_track is not None and current_track.name != title:

This comment has been minimized.

Copy link
@eamanu

eamanu Jul 16, 2019

what about

if current_track and current_track.name != title:

?

@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Sep 9, 2019

Any reason not to merge this as it is? I'm happy for it to sit in develop and wait for 3.0 if that's easier.

@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Sep 9, 2019

Actually, I need to update some tests.

@kingosticks kingosticks force-pushed the kingosticks:fix/emit-stream-title-event branch 3 times, most recently from b912d9a to 12fed9e Sep 9, 2019
@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Sep 9, 2019

Still needs a changelog entry but I need to know into which branch it's headed.

@jodal

This comment has been minimized.

Copy link
Member

jodal commented Sep 9, 2019

Target it at release-2.2. We can decide at release time if it is 2.2.4 or 2.3.0.

@kingosticks kingosticks force-pushed the kingosticks:fix/emit-stream-title-event branch 3 times, most recently from a944a5f to a4b9f1b Sep 10, 2019
@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Sep 11, 2019

OK done.

@kingosticks kingosticks changed the base branch from develop to release-2.2 Sep 11, 2019
@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Sep 11, 2019

Why is there no CI build triggered here?

@jodal

This comment has been minimized.

Copy link
Member

jodal commented Sep 12, 2019

I've turned on this CircleCI setting now:

Build forked pull requests

Run builds for pull requests from forks. CircleCI will automatically update the commit status shown on GitHub's pull request page.

Try force-pushing to trigger a build.

@kingosticks kingosticks force-pushed the kingosticks:fix/emit-stream-title-event branch from a4b9f1b to dae56b4 Sep 12, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #1751 into release-2.2 will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release-2.2    #1751      +/-   ##
===============================================
+ Coverage        80.99%   80.99%   +<.01%     
===============================================
  Files               83       83              
  Lines             6644     6646       +2     
===============================================
+ Hits              5381     5383       +2     
  Misses            1263     1263
Impacted Files Coverage Δ
mopidy/core/actor.py 94.66% <100%> (+0.07%) ⬆️

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 f91c573...dae56b4. Read the comment docs.

@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Sep 12, 2019

Now that is a coverage report!

@jodal jodal merged commit a3904c7 into mopidy:release-2.2 Sep 25, 2019
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 80.99%)
Details
codecov/project 80.99% (+<.01%) compared to f91c573
Details
test Workflow: test
Details
@jodal jodal added this to the v2.3.0 milestone Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.