Use `Identity` mpris info for menu, icon and source filtering

[Batwam]

I noticed that Shortwave in the menu was coming up as shown below. I fixed it by only keeping the section after the last dot.It works even with string which don't have . in them. I also changed it in the player list (prefs.js) for consistency.

Before / After: |

Also, as Shortwave comes up capitalised in its mpris address which looked a bit odd in the filter list, I thought about making the filtering non-case sensitive.

[Batwam]

turns out Celluloid does the same thing (screenshot below based on current main):

I haven't tried but that should be resolved with the new code too.

[Batwam]

This PR is probably the best place to include the use of mpris Identity info to work out the Menu name as it would eliminate the workaround proposed for shortname athough it would still be required for the filter. Unless we want to filter based on Identity so Brave, Chromium, Chrome don't all get filtered at the same time when chromium is excluded?

[Moon-0xff]

I was aware some sources will be shown that way. Trying to "guess" the correct name by the naming convention might fail with some players. This is the same problem we have with the guessing work for the icon.

This Identity looks like the perfect solution but maybe is another desktopEntry. Have you tested it in code?

[Batwam]

Yes I have, check out my latest comment under issue #8

[Batwam]

quick implementation of the Identity info with the following benefits:

• addresses initial Shortwave issue (tested, confirmed)
• separate icons for all chromium browsers (tested and confirmed separate icons now for chrome, chromium, brave) and more reliable search (simpler code) although some manual adjustment still had to be done for Mozilla Firefox which should search as Firefox
• also implemented in the filter list, again to allow better granularity between chromium browsers

[Moon-0xff]

This is great. One thing: on the icon side a fallback to the address name might be a good idea because firefox might not be the only icon that's not found with Identity.

Merging this PR first is preferred.

[Moon-0xff]

• entryInterface can be merged wirh mprisInterface
• updatePlayerList might need a rewrite (perhaps adding all players to the list and doing the filtering later)
• icons.js could be merged with the Player class

Those are my thoughts after looking the quick implementation code closer.

I suppose the goal is to use and interface with Identity only in the players.js file, while incorporating it to the Player class in a more natural way.

[Batwam]

Yeah, it was a first pass to make sure I could implement gradually without breaking everything but it definitely opens a few possibilities for code cleanup.

Regarding the fallback to address info, the idea of leaving the code crossed my mind but I am not convinced this is a reliable fallback. If anything, we know that people can get quite liberal with what they put into address and that the info extracted from address would fail for most chromium browsers. I have tried quite a few sources and Firefox was the only one which failed, presumably because they wrote "Mozilla Firefox" as Identity but the .desktop is only called Firefox (that's my assumption, I didn't check). The reality is that we don't really know what the most reliable fallback is until we know what source fails. For example, we could argue that cycling through each words "Mozilla" and "Firefox" of the Identity string would also lead to a working solution with less lines of code which may (or may not) be more reliable.

One thing I was thinking about as well would be to filter out "Mozilla " at the moment we read Identity so the player is called "Firefox" in the source filter list and we don't need any additional processing for the icon.

Another thing in terms of capabilities if we have a more reliable line of signed between mpris source and associated app could be to have the ability to open the app with a left click as I often find myself wishing I could do that but that's a discussion for another day 😉

Edit: I had a look and the .desktop file for Firefox and it does indeed not include the word Mozilla anywhere. Essentially, it's more of an issue with the way they named the mpris Identity field.

[Batwam]

I'm a bit worried that by making too many changes, we might increase the risk of running into merge conflicts with the filter and sound control PRs. Do you think that it might be worth doing some sort of interim merge before we go deep and start changing some of the internal mechanics?

[Moon-0xff]

I would say it's better to solve the merge conflicts in their respective branches. Using git cleverly can avoid most merge conflicts (but solving them manually is simpler and in our case probably faster).

[Batwam]

Yeah, ok, let's see how that goes. I agree that we probably want to finish and merge this PR first so we can see if it has any impact on the rest of the changes.

[Moon-0xff]

You can test it early by merging the branches locally and then reverting the commits with the hard option.

git merge x
git revert --hard [merge commits]

Beware that i'm recalling this from memory Edit: there's probably an option on git merge to check for merge conflicts

[Batwam]

Ironically, firefox is actually one of the few (with Spotify) following the standard properly as it's DesktopEntry which is intended to match the .desktop based on the specification and they specify Firefox as DesktopEntry. So, yet another alternative which would work would be to use DesktopEntry as fallback... Since there is only one source failing with the current approach, there are quite a few way to come up with a solution! I think that the current one is fit for purpose and would propose to review if we find other sources failing the icon search.

[Batwam]

ok, I installed a bunch of apps in a VM to stress test the system. It succefully handled Spotify, Chromium, Chrome, Firefox, Vivaldi, Opera, Brave, Shortwave, VLC, Amberol, Celluloid, Rhythmbox and they all work flawlessly. I'm kind of running out of sources to try!

One thing I did notice but it's more of a side thing is that some apps like Brave/Opera will not show mpris info when installed as snaps. That's more of a snap/packaging permissions issue though.

note that with the latest commit, I got rid of searchInDesktopEntries since there is edge case to handle. Feel free to reinstate if you feel like it's necessary but I am yet to find a source which fails the current implementation.

[Moon-0xff]

Using desktopEntry as a fallback sounds like a very good idea,actually.

Even if unreliable, it's supposed to be the right way of getting the icon. So there's few chance of error with that approach

[Batwam]

ok, DesktopEntry fallback implemented as requested and ready for resting. I am no longer deleting "Mozilla " so all info is as originally intended by the various sources.

I have included the name of the .desktop file into the Player class. I think that this is what you suggested above. I feel like this might come handy in the future as it opens up possibilities if we want to implement things like opening the app on left or middle click (I often wished we could do that).

[Moon-0xff]

Merge the icons.js file to the Player class. The file is very tiny now. The command for deletion is git rm

[Batwam]

ok job done. happy to merge?

[Moon-0xff]

Sure!