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

add configuration option for bus type #954

Merged
merged 1 commit into from
Sep 30, 2022
Merged

add configuration option for bus type #954

merged 1 commit into from
Sep 30, 2022

Conversation

eladyn
Copy link
Member

@eladyn eladyn commented May 15, 2021

This adds a new configuration option to configure which bus should be used by spotifyd. It addresses situations like #944 or one I personally found me in, where access to the MPRIS interface on headless systems would really help a lot.

Since I'm quite new to the codebase as well as the Rust ecosystem in general, I'm not sure if everything I changed / added was a good idea. Therefore, I'm always happy to take improvement proposals and adapt the PR accordingly. 😀

@stale
Copy link

stale bot commented Aug 14, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Issues that will not be fixed under any circumstances label Aug 14, 2021
@eladyn
Copy link
Member Author

eladyn commented Aug 17, 2021 •
edited
Loading

This PR might be somewhat obsolete, if #977 gets accepted. It would be great to get some feedback from the maintainers, what I should do about this PR!

Edit: I also rebased on the latest master, if you want to consider merging it.

@stale stale bot removed the wontfix Issues that will not be fixed under any circumstances label Aug 17, 2021
@eladyn eladyn force-pushed the add_bus_type_config branch from cdaa5d3 to 8ef32fb Compare August 17, 2021 07:38
@stale
Copy link

stale bot commented Nov 15, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Issues that will not be fixed under any circumstances label Nov 15, 2021
@eladyn
Copy link
Member Author

eladyn commented Nov 15, 2021

Anyone able to look at this (or decide that it is no longer relevant / needed)?

@stale stale bot removed the wontfix Issues that will not be fixed under any circumstances label Nov 15, 2021
@eladyn eladyn force-pushed the add_bus_type_config branch from 8ef32fb to bfceb56 Compare December 12, 2021 22:36
@eladyn
Copy link
Member Author

eladyn commented Dec 12, 2021

I rebased this on top of master. @simonpersson could you have a look at this? (Or someone else from the maintainer team, of course)

@SimonTeixidor
Copy link
Member

Hi! This looks OK to me, but I don't use dbus myself so I don't feel comfortable to merge. If you could fix the rustfmt check I'll approve, and then we hope some other maintainer can show up to test it a bit more.

@eladyn eladyn force-pushed the add_bus_type_config branch from bfceb56 to 3e96276 Compare December 13, 2021 20:54
@eladyn
Copy link
Member Author

eladyn commented Dec 13, 2021

Thanks for your feedback! I ran rustfmt (and the other pre-commit hooks) on the PR and CI should be fine now.

@slondr slondr added the pinned Apply to make stale bot ignore issue/PR. label Jan 14, 2022
@eladyn eladyn mentioned this pull request Feb 13, 2022
Closed
eladyn added a commit to eladyn/spotifyd that referenced this pull request Mar 4, 2022
@eladyn
Merge branch 'add_bus_type_config' into pr_collection
453dcd2 
Spotifyd#954
@eladyn eladyn mentioned this pull request Mar 4, 2022
Closed
@eladyn eladyn force-pushed the add_bus_type_config branch from 3e96276 to 8d1d5fc Compare April 18, 2022 20:36
@eladyn
Copy link
Member Author

eladyn commented Apr 18, 2022

I just noticed that this PR had merge conflicts since quite some time and resolved them. The changes are not too complex, so it should be easy enough to review.

@eladyn eladyn mentioned this pull request Apr 19, 2022
Closed
@GChalony
Copy link

For what it's worth, I can confirm that this PR works for me on a Raspberry Pi.
I've been looking for a way to control spotifyd without the Spotify Web API (too slow), so I tried to use MPRIS and ended up here.

However, the situation is still unclear to me :

Feedback :
Installation was relatively smooth, I just had to install libdbus-1-dev to be able to compile it properly. I've also edited /usr/share/dbus-1/system.conf to allow the spotifyd to access system dbus (with a systemctl reload dbus to reload the config).
I can see the org.mpris.MediaPlayer2.spotifyd service over the system dbus (I use python-dbus).

@eladyn
Copy link
Member Author

eladyn commented Jun 21, 2022

@GChalony, glad to hear that it works!

  • does spotifyd run network calls to answer MPRIS queries ?

Unfortunately, it has to, at least in some cases. Simple operations though like Play, Pause, Next or VolumeUp are executed locally.

At least as far as I know, the issue is still not resolved. The first thing you linked to was closed by the stale bot and the second one did not introduce the possibility to use the system bus for MPRIS, it only reworked some backend stuff.

@GChalony
Copy link

Right that's basically what I suspected. It's still a win if it doesn't take 2s to pause.

Well as far as I'm concerned thus PR can be merged. I don't know much about Rust, but I can review if you need an approver :)

@eladyn eladyn mentioned this pull request Jul 12, 2022
Closed
@GChalony GChalony mentioned this pull request Jul 25, 2022
Closed
6 tasks
@eladyn eladyn mentioned this pull request Aug 19, 2022
Closed
@eladyn eladyn force-pushed the add_bus_type_config branch from 5f13de5 to 7f249ff Compare September 9, 2022 00:11
@eladyn eladyn requested a review from a team September 9, 2022 00:12
@eladyn
add config option for bus type for MPRIS
4c6664b 
Add a configuration option that determines if the MPRIS interface
should use the session or the system bus. The default is the
session bus.
@eladyn eladyn force-pushed the add_bus_type_config branch from 7f249ff to 4c6664b Compare September 9, 2022 14:06
slondr
slondr approved these changes Sep 29, 2022
View reviewed changes
Copy link
Member

@slondr slondr left a comment

Choose a reason for hiding this comment

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

cool

@eladyn eladyn merged commit d9c6d0c into Spotifyd:master Sep 30, 2022
@eladyn
Copy link
Member Author

eladyn commented Sep 30, 2022

Thank you!

@eladyn eladyn deleted the add_bus_type_config branch September 30, 2022 21:29
@eladyn eladyn mentioned this pull request Oct 11, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slondr slondr slondr approved these changes

@robinvd robinvd robinvd approved these changes

Assignees
No one assigned
Labels
pinned Apply to make stale bot ignore issue/PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eladyn @SimonTeixidor @GChalony @slondr @robinvd