Mange / mpris-rs

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

Change finder methods #59

Closed Kanjirito closed 3 years ago

Kanjirito commented 3 years ago

Implements changes mentioned in #57

All find methods are now sorted alphabetically by their bus name which are (probably) a better way to sort than the MPRIS Identity property. For example, Firefox sets it's bus name to org.mpris.MediaPlayer2.firefox.instanceXXX while the Identity is Mozilla Firefox and it makes more sense to sort by just Firefox. find_active now first looks for a playing player, then paused, then one with track metadata and after that fails returns the first found. I'm not sure if there is a better way of implementing it since it requires iterating over a Vec multiple times and Rust doesn't like that. Added find_first which just returns the first player found. Added find_by_name which finds a player by it's MPRIS Identity property.

I've tested this with Cantata, Firefox, mpv and Dragon Player. Seems to work as intended but I will try to test it with more players just in case.

Mange commented 3 years ago

I'm not sure if there is a better way of implementing it since it requires iterating over a Vec multiple times and Rust doesn't like that.

One could write it in a more imperative style. Pseudo code:

pub fn find_active_player(): Result<Player, FindingError> {
  let players = …?;
  if let Some(index) = find_active_player_index(&players) {
    Ok(players.remove(index))
  } else {
    Err(FindingError::NoPlayerFound)
  }
}

fn find_active_player_index(players: &Vec<Player>): Option<unit> {
  let mut first_paused: Option<uint> = None;
  let mut first_with_track: Option<uint> = None;

  let first_player: Option<uint> = if players.is_empty() { None } else { Some(0) };

  for (index, player) in players.iter().enumerate() {
    if is_playing(&player) {
      return Some(index);
    }

    if first_paused.is_none() && is_paused(&player) {
      first_paused.replace(Some(index));
    }

    if first_with_track.is_none() && has_track(&player) {
      first_with_track.replace(Some(index));
    }
  }

  first_paused.or(first_with_track).or(first_player)
}
Kanjirito commented 3 years ago

I rewrote it in the way you suggested so it now avoids unneeded iterating and sending of DBus messages. I also added Metadata::is_empty() to avoid having to iterate over the internal HashMap and then collecting it just to check if there even is any metadata set. Let me know if you see something that should/could be changed.

Mange commented 3 years ago

Thank you very much for your contribution! :heart: