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.

Sign up for GitHub

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

Jump to bottom

Device Name in Spotify remains "Librespot" ulbi/spotifyd#1 #1197

Merged
merged 4 commits into from
Mar 28, 2023
Merged

Device Name in Spotify remains "Librespot" ulbi/spotifyd#1 #1197

merged 4 commits into from
Mar 28, 2023

Conversation

ulbi
Copy link
Contributor

@ulbi ulbi commented Mar 26, 2023

No description provided.

ulbi added 2 commits March 26, 2023 16:54
@ulbi
Device Name in Spotify remains "Librespot" #1
818606f
@ulbi
Discovered spotifyd does not direct directly and is shown under "othe…
5a5a344 
…r networks" #2

the device_id should not be the name, but something more unique.
Switched back to the session id.
@ulbi ulbi closed this Mar 26, 2023
@ulbi ulbi reopened this Mar 26, 2023
eladyn
eladyn reviewed Mar 26, 2023
View reviewed changes
Copy link
Member

@eladyn eladyn left a comment

Choose a reason for hiding this comment

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

Oh, nice catch! Good that you discovered that before the release. 🙈

Now that we're at it, at least from my testing, we can remove this restriction. (and the corresponding comment in the docs)

Apart from that, could you please run cargo fmt on that, otherwise CI will complain.

@eladyn eladyn added this to the v0.3.5 milestone Mar 26, 2023
@ulbi
Copy link
Contributor Author

ulbi commented Mar 26, 2023

Oh, nice catch! Good that you discovered that before the release. 🙈

Now that we're at it, at least from my testing, we can remove this restriction. (and the corresponding comment in the docs)

Apart from that, could you please run cargo fmt on that, otherwise CI will complain.

Thanks for the feedback. Did run cargo fmt and dropped the space limitations as well. Hopefully it will go through now.
BTW: I did add a log entry for the device_id - need this for monitoring and regularly checking whether the spotifyd is still running in it's docker container.

eladyn
eladyn requested changes Mar 26, 2023
View reviewed changes
Copy link
Member

@eladyn eladyn left a comment

Choose a reason for hiding this comment

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

Two last comments, after that I think we're good to go.

src/setup.rs Outdated Show resolved Hide resolved
docs/src/config/File.md Show resolved Hide resolved
@eladyn eladyn mentioned this pull request Mar 27, 2023
Closed
6 tasks
@eladyn eladyn linked an issue Mar 27, 2023 that may be closed by this pull request
Spotifyd registers twice in spotify connect #1198
Closed
6 tasks
eladyn
eladyn approved these changes Mar 27, 2023
View reviewed changes
Copy link
Member

@eladyn eladyn left a comment

Choose a reason for hiding this comment

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

Looks good and works. Thank you!

@eladyn eladyn merged commit ec75498 into Spotifyd:master Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladyn eladyn eladyn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.3.5
Development

Successfully merging this pull request may close these issues.

Spotifyd registers twice in spotify connect
2 participants
@ulbi @eladyn