-
Notifications
You must be signed in to change notification settings - Fork 455
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
Implement PropertiesChanged event for dbus_mpris #1025
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this!
I assume that you are aware of #575? I personally think that your code looks much cleaner, as it reuses most of the existing logic, although I have not yet tested it.
(I'm not a maintainer, just wanted to add my thoughts on this.)
To be more specific: this now implements the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this PR for some time now and it works really well! (tested with the native GNOME indicator as well as with playerctl
)
It should probably also fix #910.
Tested on my setup ( |
Thank you ! It's a great feature |
Great to see this PR. |
As per this comment, I'll just try and ping the maintainers of this project. @mainrs, @JojiiOfficial, @slondr, @simonpersson: It would be great, if someone "official" could review this PR as it really solves several longstanding problems! Sidenote, unrelated to this specific PR: I really appreciate the time and energy that you put into this project, to keep it going. Still, it would definitely help, if there was some guideline (e.g. in the |
Please rather add review requests than pings. I appreciate your effort but it feels wrong for me to approve bigger PRs, as I barely have knowledge of the existing code nor the code style of spotifyd. Also I don't have the time to check on that everything is bugfree and works as intended. |
To be honest, we should probably turn off stalebot. It made sense at the time, but development has slowed down since 2020 (and I certainly have less free time now to review issues and PRs). |
Thank you both for the immediate answers! Unfortunately it seems that "normal" people (non-members) can't request reviews (I definitely can't do that on my own PRs). |
@MDeiml |
You are right, this is not implemented as of now. My implementation only listens to |
Ah I see.. I was looking at: If I see this correctly, you haven't actually changed much of the existing code, but instead added the properties changed events separately by using the DBus_crossroads framework. So the old code is still handling commands or requests towards spotifyd. |
@NNEU-1 There was a short discussion on the |
@eladyn Anyways, back to the issue.. |
use double between 0.0 and 1.0 instead of u16 between 0 and 65535
Thank you for the PR. |
Co-authored-by: eladyn <59307989 [email protected]>
add volume properties changed event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks very good! sad that the other pr never got merged, but i think this one is more complete and up to date.
if you can fix the lints i think this is ready!
also sorry for the delays and again thx for the PR!
@JojiiOfficial maybe just test it and approve? |
This should fix #457.