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

Implement PropertiesChanged event for dbus_mpris #1025

Merged
merged 10 commits into from
Mar 11, 2022

Conversation

MDeiml
Copy link
Contributor

@MDeiml MDeiml commented Dec 8, 2021

This should fix #457.

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.

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.)

src/dbus_mpris.rs Outdated Show resolved Hide resolved
@MDeiml
Copy link
Contributor Author

MDeiml commented Dec 13, 2021

To be more specific: this now implements the PropertiesChanged signal for the Metadata and PlaybackStatus properties as well as the Seeked signal.

eladyn
eladyn previously approved these changes Dec 13, 2021
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.

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.

@dave-tucker
Copy link

Tested on my setup (i3status-rust with spotifyd and spotify-tui) and I can also confirm that this works like a charm!
Thank you!!!

@PaulFinch
Copy link

Thank you ! It's a great feature
Waiting for this to be implemented, it will prevent me to poll the PlayingStatus (which sometimes returns 'Stopped' when Playing, awkwardly).

@NNEU-1
Copy link
Contributor

NNEU-1 commented Jan 13, 2022

Great to see this PR.
The DBus implementation has been the subject of many issues reported on this package.
It is sad however, that now that someone has put in the work, it doesn't get merged.
Should have been approved by the maintainers by now, or at least had feedback on why it isn't.

@eladyn
Copy link
Member

eladyn commented Jan 13, 2022

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 CONTRIBUTING.md) on what people, who want to make contributions to spotifyd, should do, if there is no reaction at all (except for those from the stale bot) on their PRs. At the very least, it could set the right expectations for potential contributors.

@JojiiOfficial
Copy link
Member

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.

@slondr
Copy link
Member

slondr commented Jan 13, 2022

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).

@eladyn
Copy link
Member

eladyn commented Jan 13, 2022

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).

@NNEU-1
Copy link
Contributor

NNEU-1 commented Jan 28, 2022

@MDeiml
Can you confirm that the Volume property is not working ?
I can see it being implemented, but it doesn't seem to emit any signals.

@MDeiml
Copy link
Contributor Author

MDeiml commented Jan 28, 2022

You are right, this is not implemented as of now. My implementation only listens to PlayerEvent::Stopped, PlayerEvent::Playing and PlayerEvent::Paused events. Volume could be implemented in a similar way by using the PlayerEvent::VolumeSet event. (see PlayerEvent)

@NNEU-1
Copy link
Contributor

NNEU-1 commented Jan 28, 2022

Ah I see..

I was looking at:
b.property("Volume").emits_changed_false().get(move |_, _| { let sp = create_spotify_api(&mv_api_token); let vol = sp .current_playback(None) .ok() .flatten() .map_or(0.0, |p| p.device.volume_percent as f64); Ok(vol) });

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.
At least some of them - Volume is a read only property which it shouldn't be.

@eladyn
Copy link
Member

eladyn commented Jan 28, 2022

@NNEU-1 There was a short discussion on the Volume property being readonly here (even if it was not really related to the actual issue). Changing the function to be read and write shouldn't be too difficult as there is a function in rspotify which does exactly that. (The librespot functions allow only in- or decreasing the volume and not setting a specific value, IIRC)

@NNEU-1
Copy link
Contributor

NNEU-1 commented Jan 28, 2022

@eladyn
Now that you're pointing it out, I recall having investigated this myself some time ago...
#962

Anyways, back to the issue..
I made a PR on this branch to implement volume changed events according to @MDeiml s proposal.
Tested and works.
I have no background or experience in rust, so i trust you'll let me know if this is good enough.

MDeiml#2 (comment)

use double between 0.0 and 1.0 instead of u16 between 0 and 65535
@Kerwood
Copy link

Kerwood commented Feb 5, 2022

Thank you for the PR.
This was much needed. Compiled and tested, works like a charm. 👍

NNEU-1 and others added 3 commits February 5, 2022 20:42
Copy link
Contributor

@robinvd robinvd left a 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!

src/main_loop.rs Show resolved Hide resolved
@MDeiml MDeiml requested a review from robinvd February 8, 2022 14:53
@robinvd
Copy link
Contributor

robinvd commented Feb 8, 2022

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.

@JojiiOfficial maybe just test it and approve?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Apply to make stale bot ignore issue/PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DBus PropertiesChanged event not fired
9 participants