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

Add a blocking status query #37

Closed
repomaa opened this issue Oct 29, 2016 · 14 comments
Closed

Add a blocking status query #37

repomaa opened this issue Oct 29, 2016 · 14 comments
Milestone

Comments

@repomaa
Copy link

repomaa commented Oct 29, 2016

this would enable waiting for song changes etc.

@acrisci
Copy link
Member

acrisci commented Nov 6, 2016

Hi,

That's already possible with the library. A script to do this might be a good contribution.

@acrisci
Copy link
Member

acrisci commented Nov 7, 2016

A command like playerctl watch [ATTRIBUTES] in the cli might be a good idea too. I'll think about that.

@acrisci
Copy link
Member

acrisci commented Oct 4, 2018

Yeah, actually I think we can do this.

playerctl status --follow

@acrisci
Copy link
Member

acrisci commented Oct 10, 2018

Ok how should this work? Here are the issues:

  1. What happens if the player is not present when the command is run? I can see how that would be a problem for integrating this into a stautsline because you'd basically have to either poll or watch the bus and both of those are not ideal. Maybe the library could just watch the bus for you.
  2. What happens when the player has empty metadata? The format string parameters in that case will just be empty. Maybe just print an empty line?
  3. What happens when the player exits? That's pretty much the same problem as 2) though.
  4. Do we want to support multiple players when in follow mode? I'm leaning towards no right now because then you can't distinguish players easily in the case where metadata is empty.

Does anyone have any thoughts?

@roobre
Copy link

roobre commented Oct 11, 2018

Hey,
Just leaving some suggestions to your points:

  1. I think it's either failing with an error or wait until some player shows up. I'm not sure what should be the best option here. Exiting may seem cleaner, but blocking seems like a better idea since allows using the command as a "notification" source. I think I'd go for blocking.
  2. I'd go for the empty line here perhaps overrideable with an user defined default (--placeholder "Unknown"?) or anything else we can derive from the data.
  3. Empty line, for sure. Whatever is consuming the output must be able to distinguish between "unknown song being played" and "no song being played".
  4. Would it be difficult to maintain the behavior of -p here? I don't know the internals of dbus interface, so no idea here.

@acrisci
Copy link
Member

acrisci commented Oct 11, 2018

I think it's either failing with an error or wait until some player shows up. I'm not sure what should be the best option here. Exiting may seem cleaner, but blocking seems like a better idea since allows using the command as a "notification" source. I think I'd go for blocking.

Yeah I think we'll block if the player isn't up and connect to any player that matches when it shows up.

I'd go for the empty line here perhaps overrideable with an user defined default (--placeholder "Unknown"?) or anything else we can derive from the data.

Would it be enough to extend the format template language to add a default() helper function? What about playerctl metadata --format '{{ default(artist, "Unknown") }}' --follow?

Empty line, for sure. Whatever is consuming the output must be able to distinguish between "unknown song being played" and "no song being played".

Yes, there are two cases here. Either there is no song because the player has exited, or no song because the player has stopped. Empty line would work here, but you can't distinguish between those two cases then.

Would it be difficult to maintain the behavior of -p here? I don't know the internals of dbus interface, so no idea here.

Yes, anything is possible. But the rules on this are going to be very subtle because there's a lot of edge cases with the players coming and going. The details will be noticible.

@roobre
Copy link

roobre commented Oct 11, 2018

Would it be enough to extend the format template language to add a default() helper function?

I'm not sure if that approach makes the point clear to the user. Having a default per variable makes it sound like a beautifier for some missing data, instead of a feature needed in order to distinguish all metadata being empty from no song being played. I like the default() feature but if we are going down this path we should clearly state in the help that omitting default values for all variables will cause problems, since the program will treat unknown songs the same way as nothing being played.

Either there is no song because the player has exited, or no song because the player has stopped. Empty line would work here, but you can't distinguish between those two cases then.

This is right, but I don't think it matters to the user. There is no active playback, no matter the cause.

Yes, anything is possible. But the rules on this are going to be very subtle because there's a lot of edge cases with the players coming and going. The details will be noticible.

We can default to output lines for any player when the state changes, and only for a given player if --player is given. I think that should cover most use cases, right?

@acrisci
Copy link
Member

acrisci commented Oct 11, 2018

we should clearly state in the help that omitting default values for all variables will cause problems, since the program will treat unknown songs the same way as nothing being played.

Yeah that seems like the most intuitive thing to do. I was thinking about a case where we could inject other variables into the template context for metadata on demand like playerctl metadata --format 'status: {{status}}, artist: {{artist}}' but that approach would certainly require either the format to be machine readble (which requires insight on the part of the user) or conditionals in the template language which I don't want to do. But maybe that's asking too much of a single blocking loop. You could probably accomplish that same thing with two blocking loops (one for the status and one for the metadata), although they would be hard to sync if that's needed. But at a certain point of complexity, I'm just going to point people toward the library anyway.

@acrisci
Copy link
Member

acrisci commented Oct 11, 2018

We can default to output lines for any player when the state changes, and only for a given player if --player is given. I think that should cover most use cases, right?

Yeah, there's one edge case I can think of here when multiple players are given.

If I do playerctl --player=vlc,rhtyhmbox metadata --follow and at first rhythmbox is up, I connect to it. But then if later vlc shows up, should I then connect to vlc even if rhythmbox is still running?

The rule is pretty simple. If a new player shows up, just rerun the selection algorithm. It's also conceptually a bit simpler because you don't have to worry about what player you ran last. You can just look at the output and if vlc is up, then that's it. It also allows the user to express that the first player in the list is more important and takes precedence. There might be good arguments against it though.

@acrisci
Copy link
Member

acrisci commented Oct 11, 2018

Ok I just committed something in the master branch. Please test and let me know what you think about the behavior, especially with opening up new players and passing multiple players.

@roobre
Copy link

roobre commented Oct 15, 2018

If I do playerctl --player=vlc,rhtyhmbox metadata --follow and at first rhythmbox is up, I connect to it. But then if later vlc shows up, should I then connect to vlc even if rhythmbox is still running?

I wasn't even thinking about the possibility of supplying multiple players, but that would be cool. Either the priority approach you mentioned would be fine, or perhaps even a pure whitelist approach where only the players passed will be shown but in a fcfs basis. Either is fine to me as long as it's documented, since as I said I think it's not a common use case specifying more than one player and wanting one to take precedence over the other.

Ok I just committed something in the master branch. Please test and let me know what you think about the behavior, especially with opening up new players and passing multiple players.

Sure, I'll give it a quick look and come back with some feedback.

@roobre
Copy link

roobre commented Oct 15, 2018

I'm back.
First, I'm getting some warnings when the player stops after it gets attached:

(playerctl:16636): GLib-CRITICAL **: 10:33:45.249: g_variant_get_string: assertion 'value != NULL' failed

(playerctl:16636): GLib-CRITICAL **: 10:33:45.249: g_variant_unref: assertion 'value != NULL' failed

Also, regarding the help message:

-F, --follow Block and append the query to output when it changes. Exit when the players exit.

I haven't been able to reproduce this behavior, playerctl blocks until a new player appears and then shows the info (which in fact it is what I think is better (help typo?))

Also I'm having some issues by attaching this to polybar, but it's probably my fault so I'll come back with more feedback when I get it working.

@acrisci
Copy link
Member

acrisci commented Oct 15, 2018

First, I'm getting some warnings when the player stops after it gets attached:

What player is this? I haven't gotten this message but I can try to reproduce it.

I haven't been able to reproduce this behavior, playerctl blocks until a new player appears and then shows the info (which in fact it is what I think is better (help typo?))

Yeah, that's where I ended up with it. When multiple players are matched, the default is to only attach to the one with the highest priority. Pass --all-players to get the second behavior, which I think is probably better as well.

@roobre
Copy link

roobre commented Oct 15, 2018

Yeah, that's where I ended up with it. When multiple players are matched, the default is to only attach to the one with the highest priority. Pass --all-players to get the second behavior, which I think is probably better as well.

I got the non-existing behavior by not supplying neither -a nor -p (which I think is interpreted like an implicit -a). I'm not a fan of the exit thing but as long as it can be prevented with -a is fine to me.

What player is this? I haven't gotten this message but I can try to reproduce it.

Rhythmbox (which is terrible but the only thing supporting ampache afaik).

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

No branches or pull requests

3 participants