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

Improve player name detection #84

Merged
merged 1 commit into from
Jun 15, 2024
Merged

Improve player name detection #84

merged 1 commit into from
Jun 15, 2024

Conversation

Kanjirito
Copy link
Contributor

@Kanjirito Kanjirito commented Jun 7, 2024

Improves #81
Instead of just assuming the first part of the bus name is the player name it now instead checks if the last part is a instance part named like the example in the MPRIS spec (instanceID). If yes then the second to last part gets used.

This fully depends on whether players implement the instance part this way so it will require some further testing with more players but I don't believe there's another way to do this.

And I am aware that there's a rewrite branch but I haven't used it yet so it was simpler for me to do it here for now.

@ravener
Copy link

ravener commented Jun 8, 2024

I repeat here that 'Flatpak name parts' is a misleading term, they are called Application IDs, and even non-flatpak apps are encouraged to use them.

I'm also not sure what to feel about completely omitting those parts, on one hand, this gets pretty close to what player.identity() is for since that usually returns the player name and nothing related to the bus name.

The safety check is also not the best, like I mentioned in the issue, plenty of GitHub/GitLab hosted projects will use the format io.github.username.Name, nothing guarantees that a len() == 3 is a complete name.

@Kanjirito
Copy link
Contributor Author

Oh I was not aware that GNOME uses that ID system overall. I'll keep that in mind if this goes anywhere.

I do agree that it should return the whole bus string just without the MPRIS prefix and the instance part if it gets detected. I kinda forgot that the point was to get the all of the name parts of the bus and not just the name itself.

And the check is mostly there for the len() == 1 case since a player that has a bus name that only has one part which starts with "instance" would cause a crash. len() == 3 just catches the edge case of something like org.github.instance since as I've mentioned before 3 parts mean that it's a GNOME ID without an instance part. It's technically not needed since I doubt there will be a player called that but with the check the edge case will only show up if a player has a longer ID and is called something that starts "instance" at which point there's just no way of knowing for sure.

Copy link
Owner

@Mange Mange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this doesn't solve all the issues in the original bug report, this does appear to fix at least some of those issues and I think its inclusion could be a net-positive.

No need to let the perfect be the enemy of good here.

I do wish to add some unit tests for this to make problems easier to visualize with realistic examples. I don't think it would be easy to set up fake Player instances, so if you cannot get that to work nicely, consider moving the implementation out to a free method in this module and then embedding some tests to test it without needing to export it.

pub fn bus_name_player_name_part(&self) -> &str {
  extract_player_name_part(&self.bus_name)
}

///

fn extract_player_name_part(bus_name: &str) -> &str {
  // ...
}

#[cfg(test)]
mod tests {
  use super::*;

  #[test]
  fn it_extracts_common_player_names_from_bus() {
    assert_eq!("org.mpris.MediaPlayer2.Spotify.instance2", "Spotify")
    // ...
  }
}

@Kanjirito
Copy link
Contributor Author

This should be all unless you want me to reword something.

Copy link
Owner

@Mange Mange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! 😍

@Mange Mange merged commit e25f5d0 into Mange:master Jun 15, 2024
@Kanjirito Kanjirito deleted the player-name branch September 11, 2024 19:55
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