search

Mange / mpris-rs

Idiomatic MPRIS D-Bus interface library for Rust
Apache License 2.0
67 stars 17 forks source link

Suggestion: Use PlaybackStatus property to determine active player #57

Closed dogue closed 1 year ago

dogue commented 3 years ago

Rather than just returning the first player from PlayerFinder::find_active(), is it possible to query the PlaybackStatus property of each player on the bus and return (the first) one with the "Playing" status?

https://specifications.freedesktop.org/mpris-spec/latest/Player_Interface.html#Property:PlaybackStatus

Edit: typo

Mange commented 3 years ago

He he, that was actually always planned to be done "at some point". It's the reason for the last notice inside this function's documentation:

https://github.com/Mange/mpris-rs/blob/c3f47a33d2e816bedaabb6dd1019b884c1681522/src/find.rs#L81-L89

It's good to have it documented here as a feature request, though. Much better source of information that just inside my skull, that's for sure!

Kanjirito commented 3 years ago

Should be easy enough to implement so I'll give it a shot but I have questions about how it should work.

  1. What should be the order that the players are checked in? I'm not sure how D-Bus determines the order so if the order is random maybe the players should be sorted alphabetically by name (or something else) so that the behavior is more predictable.
  2. What should it return if no player is currently playing? There are 3 options that I see:
    • Return NoPlayerFound if no player playing
    • If no player playing check for paused players and if there are none return NoPlayerFound (a return_any method might need to be created in this and the above case)
    • The above but return NoPlayerFound only when there is no player at all and otherwise just return any player after no playing/paused player found.
Mange commented 3 years ago

Good point. Maybe there should be many finders there. Without thinking about what is reasonable to implement, I could imagine these:

I too am unaware about the order given by DBus. I support the idea of sorting the players on their names before iterating. I don't think there's any way to do "find latest" and "find last" seems to have little enough use to not warrant inclusion.

I'm on board with trying to implement this before shipping the final release of 2.0 as the last big breaking change.

I'd like this setup, I think:

Only find_active is needed to be in place before the final release as the others are new and can come in a 2.1 release.

If you want to go ahead and try implementing any of this, I would appreciate it very much. I could also do it myself if you do not feel up for it. :smile:

Kanjirito commented 3 years ago

I think I can do this. I'm just wondering if into_iter is really needed since find_all already returns a Vec which is easy enough to iterate over and just do your own search. The IntoIterator trait doesn't have a way to return errors (which can happen in this case) and returning an empty iterator seems like a bad idea. So just using find_all seems like the better solution to me. I'll also add a method to find a player by name since that seems like a common use case.

Mange commented 3 years ago

I think I can do this. I'm just wondering if into_iter is really needed since find_all already returns a Vec which is easy enough to iterate over and just do your own search.

Yeah, but if you just want the first one it is wasteful to allocate a vec and load all the players. It's more idiomatic in Rust to return iterators and let the callers collect them to a Vec if they want one.

It's a whole lot easier to implement as a Vec, though, which is why I did so in the initial release.

The IntoIterator trait doesn't have a way to return errors (which can happen in this case) and returning an empty iterator seems like a bad idea.

Good point. Maybe some other trait is better. Alternatively we could have a find_iter(): Result<Iterator<Player>, Error> or IntoIter iter(): Iterator<Result<Player, Error>>.

No need for you to implement any of that if you don't want to. I'm mostly thinking aloud here!

So just using find_all seems like the better solution to me.

It should definitely not go away. :)

I'll also add a method to find a player by name since that seems like a common use case.

Oh yes. How could I forget about this? Yes, for sure!

Kanjirito commented 3 years ago

I think I can implement this one way or another but it should probably get it's own issue/PR to discuss how exactly it should work. For now I'll just finish what I started, the iterator can be added later.

dogue commented 1 year ago

Closing this per https://github.com/Mange/mpris-rs/issues/64