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

Allow the user to configure the position update interval #32

Merged
merged 2 commits into from
Mar 24, 2023

Conversation

cdown
Copy link
Contributor

@cdown cdown commented Mar 24, 2023

After I started using mpd-mpris on my laptop I noticed significantly
more wakeups than before even when mpd is not playing anything. This
seemed surprising, because applications using dbus typically do not
require polling.

Looking more closely, the polling comes from updating the current song
position, since this doesn't generate dbus events. As such, this also
happens even when mpd is idle.

The interval is now configurable, and if set to 0, never updates the
status just for position information (although, of course, it will be
updated on other changes that result in a status update).

On my machine, this renders mpd-mpris significantly more willing to
sleep and avoid wakeups, especially since I don't use this feature
anyway, so it's nice to be able to disable it with --interval 0.

@cdown cdown force-pushed the cdown/2023-03-23/interval_flag branch from 287dc63 to 2a13833 Compare March 24, 2023 04:21
@cdown
Copy link
Contributor Author

cdown commented Mar 24, 2023

And thank you for this useful application!

@natsukagami
Copy link
Owner

Indeed, it should be polling way less than 0.1 second. I wonder what the default should be? Perhaps a second? AFAIK a lot of mpris clients would just assume that the duration moves forwards even when no events are fired, so the polling is just more of a keep-in-sync.
Thanks for the PR!

cmd/mpd-mpris/main.go Outdated Show resolved Hide resolved
@cdown cdown force-pushed the cdown/2023-03-23/interval_flag branch from 37916e2 to 4df8737 Compare March 24, 2023 17:33
@cdown
Copy link
Contributor Author

cdown commented Mar 24, 2023

Thanks! Updated the code and readme output based on the suggestion.

After I started using mpd-mpris on my laptop I noticed significantly
more wakeups than before even when mpd is not playing anything. This
seemed surprising, because applications using dbus typically do not
require polling.

Looking more closely, the polling comes from updating the current song
position, since this doesn't generate dbus events. As such, this also
happens even when mpd is idle.

The interval is now configurable, and if set to 0, never updates the
status just for position information (although, of course, it will be
updated on other changes that result in a status update).

On my machine, this renders mpd-mpris significantly more willing to
sleep and avoid wakeups, especially since I don't use this feature
anyway, so it's nice to be able to disable it with `--interval 0`.
@cdown cdown force-pushed the cdown/2023-03-23/interval_flag branch from 4df8737 to 64dcc86 Compare March 24, 2023 17:49
@natsukagami
Copy link
Owner

Looking great, thank you so much!

@natsukagami natsukagami merged commit 8849b09 into natsukagami:master Mar 24, 2023
@Barbaross93
Copy link

Unfortunately, setting interval to anything other than 0 gives the following error: invalid value "1" for flag -interval: parse error. The help flag also doesn't give the default value, just (default is)

@cdown
Copy link
Contributor Author

cdown commented Mar 26, 2023

@Barbaross93 "1" isn't a valid time, since it's missing any unit (1 millisecond? 1 second? 1 minute?). If you want 500 milliseconds, then the value should be --interval 500ms.

The help flag also doesn't give the default value, just (default is)

Sounds like your terminal font doesn't distinguish 1 and i very strongly :-) The default text says "default 1s", not "default is".

@Barbaross93
Copy link

@cdown Ah, I read the help output too quickly and was confused. I'm pretty used to default values of seconds when specifying units of time (E.g. sleep command in the shell) so I expected it to work the same way; Especially when you give the example of "0" without units. It might be worth improving the error message or otherwise be explicit about requiring units.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants