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

player.bus_name_player_name_part() returns wrong value on reverse domain package names #81

Open
ravener opened this issue May 12, 2024 · 10 comments

Comments

@ravener
Copy link

ravener commented May 12, 2024

Apps, especially ones from Flatpak use a reverse-domain notation like io.bassi.Amberol this makes their D-Bus name show up like org.mpris.MediaPlayer2.io.bassi.Amberol and in return using the player.bus_name_player_name_part() only returns io instead of io.bassi.Amberol

I understand it's partly due to the attempt to remove that instance part on other players, so I am not sure if there is a feasible solution in this case but I wanted to report that such things happen to start a discussion.

@Mange
Copy link
Owner

Mange commented May 13, 2024

To ease up on discussion, here's the code in question:

pub fn bus_name_player_name_part(&self) -> &str {

I agree with this issue and it should be possible to make a better guess of the app name.

I'm very much open for PRs on this, until I get the time and motivation to look at it myself.

@Kanjirito
Copy link
Contributor

I don't believe that there's a clean solution for this because MRPIS doesn't specify how exactly the instance part should look like.

In the case where the media player allows multiple instances running simultaneously, each additional instance should request a unique bus name, adding a dot and a unique identifier to its usual bus name, such as one based on a UNIX process id.

Theoretically doing something as simple as a incrementing counter is fine, so mpv.2 or even something silly like mpv.second would be completely valid. This means that there isn't really a completely safe way of detecting it because the only certain thing is that the instance part will be a separate part after the name and Flatpak's ID convention says that you can and should use many parts in the name so it's not that simple to detect.

I was thinking about counting the dots in the name which would kinda work:

  • 0 dots = regular program, no instances so it's just the name
  • 1 dot = regular program, it's an instance so remove the last part
  • 2 dots = Flatpak program, no instances so the last part is the name
  • 3 or more dots = Flatpak program with instances or just a longer name so would still need to be a guess

This is clearly a hack but I guess it does slightly improve the situation. Though I think the smarter thing would be to just assume that all implementations use the example shown on the MPRIS page which is .instanceID. I've never used Flatpak so I'd have to set that up before I can test it with some programs from there but so far I've tested mpv-mpris and VLC which both do it exactly that way while Firefox does .instance_1_52 which would still get detected by a simple .starts_with("instance") .

@ravener
Copy link
Author

ravener commented Jun 8, 2024

I agree that there isn't a foolproof solution to this.

It's also worth noting that the dotted notation isn't just unique to Flatpak, it's generally recommended by gnome so calling it specific to flatpak can be misleading.

I have one player that is also available on flatpak but I have it installed from system packages and it still has that kind of naming since that's just how application-ids work.

Nor are flatpaks forced to comply with this for their D-Bus name, Blanket for example reports it as simply Blanket and not the whole application-id (although technically these would be a 'bad practice' since it's recommended for the D-Bus name to match the application-id)

That said I guess the instance part seems fairly consistent across players I haven't seen any that breaks it yet, I just tested Celluloid and it reports io.github.celluloid_player.Celluloid.instance-1 (this also shows the 3+ dots case, many apps that are hosted on GitHub/GitLab follow this notation)

I'm not sure what to suggest in the end:

  • You either still accept the fact that it's a 'guess'
  • I could also accept it if the method just trimmed the mpris prefix but kept the instance, allowing the user to do what they want, for identifying players generally the player.identity() function is a better solution.

@Mange
Copy link
Owner

Mange commented Jun 9, 2024

Moving towards making fewer guesses would be my preferred solution; presenting it mostly as-is and letting the client care more about it. However, a "guessing" method could still be useful to provide since the pattern would be very common and I think having all the little edge cases documented in the central library is better than having all clients need to rediscover the exceptions during their lifetimes.

I'm pretty sure I based this implementation a bit around how some other software handled it, for example playerctl. It could be useful to look at how those projects work around this issue in the spec.

Lastly, one approach that could be possible for guessing is honestly to do a list of known examples. Players like "spotify", "totem", "celluoid", "blanket", and so on could get very accurate detection from being listed in their respective "buckets" for how to "parse" the instances.
Players not on any list would then try the most common way,

(Sounds like a nightmare to get this one right and I'm not sure I really see the full value of it, but hey - it's an idea.)

@Kanjirito
Copy link
Contributor

Kanjirito commented Jun 10, 2024

playerctl used to look for ".instance" but now it just splits it on a dot and assumes everything after is the instance part (relevant issue). I've also found a bash scrip listed on the Arch wiki and it assumes the instance part is the last part that starts with "instance" it also only uses that to determine some info for the notification and nothing more.

What is the use case of removing the instance part in the first place though? The bus name is only useful if you want to control a specific player so removing the unique part seems to be counterproductive. I can't think of a situation where the stripped bus name would be more useful than player.identity() so I don't think trying to implement a catch-all-filter here is worth the effort. The identity property is required by the spec so I don't see any point in cleaning up the dbus name outside of removing the prefix.

@Mange
Copy link
Owner

Mange commented Jun 11, 2024

I agree with not really understanding it. It as introduced in #53.

Perhaps we should just deprecate it and leave apps to do this themselves if they need it. I'm leaning pretty heavy towards that.

@Kanjirito
Copy link
Contributor

I think that's the best solution. There is no actual spec for the instance part and there's no clean solution for it so I think it's fair to say that it's up to the person using this library to decide how they want to deal with this issue. The only edge case I can think of where this would be relevant is when 2 players have the same name but for some reason use a different bus name (GNOME ID vs regular name or one of the players avoiding a name conflict) but this seems unlikely and not worth the effort of maintaining a big list of players and their quirks.

I'm for either just changing bus_name_player_name_part to only trim the prefix or leaving it as it is now, marking it as deprecated and creating something like bus_name_trimmed which only does the prefix trim. That way in case someone does actually use the current implementation they can keep using it for now. With that if some program decides to filter players out by their bus name to avoid this potential issue they can just do something simple like:

let finder = PlayerFinder::new().unwrap();
for player in finder
    .iter_players()
    .unwrap()
    .map(|p| p.unwrap())
    .filter(|p| p.bus_name_trimmed().starts_with("mpv"))
{
    // Do something with only mpv instances
 }

I thought about maybe adding this functionality to PlayerFinder but this really is just a .filter() on either find_all or the PlayerIter so I don't think it's worth it. (Though I have thought about making the finder methods more flexible which would make this a potential feature but that's for another time)

@Mange
Copy link
Owner

Mange commented Jun 12, 2024

I'm making the call now. Thank you all for the discussion! ❤️

The method will be deprecated and removed in the next major release.


I'm for either just changing bus_name_player_name_part to only trim the prefix or leaving it as it is now, marking it as deprecated and creating something like bus_name_trimmed which only does the prefix trim.

Changing its behavior is a breaking change, so I want to leave it alone. We should mark it as deprecated only.

I thought about maybe adding this functionality to PlayerFinder but this really is just a .filter() on either find_all or the PlayerIter so I don't think it's worth it. (Though I have thought about making the finder methods more flexible which would make this a potential feature but that's for another time)

Having PlayerFinder presenting its result as an iterator is IMO the most flexible and powerful way to do PlayerFinder, and I see no reason to extend it further. Any filtering is already possible directly on the iterator.

If someone picks up this suggestion to deprecate, please include the code example above in the Changelog and function documentation as a way to work around the deprecation.

@Kanjirito
Copy link
Contributor

Should I just force push changes into the existing PR or make a new one? And I'm assuming that bus_name_trimmed should be included too.

@Mange
Copy link
Owner

Mange commented Jun 13, 2024

A force push would be appreciated.

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

No branches or pull requests

3 participants