search

Moon-0xff / gnome-mpris-label

A music related GNOME extension.
GNU General Public License v3.0
51 stars 10 forks source link

Prioritise first app for icon match (Spot Fix) #35

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

With regards to issue #33, upon further inspect, due to the inability of the forEach to exit on match, the icon is set based on the last match.

In the case of Spot, if Spotify is open (unlikely by ok...), we get to a situation where matchedEntries: dev.alextren.Spot.desktop,com.spotify.Client.desktop which are both open apps. When we get to _matchRunningApps(), this the first entry matches a running app but so does the second one which is taken as the correct result.

We could use a for loop with a return instead. In this commit, I have instead implemented a matchFound flag so when a match is found, no other match is saved. This successfully fixes the issue and on the basis that matchedEntries are already ordered but Gnome in order of probability, I believe that it is fair to prioritise the first match.

I don' t believe that this introduces any regression but do let me know if you have any comment. It's a shame we can't run forEach backwards!

Moon-0xff commented 1 year ago

It's a shame we can't run forEach backwards!

We can by chaining it with reverse() but i don't think we should do that.

for loops are unnecessarily verbose and except in rare occasions javascript grants us a more easier to read alternative.
Instead of working around the limitations of forEach we should use another of those alternatives, this is written in the docs:

There is no way to stop or break a forEach() loop other than by throwing an exception. If you need such behavior, the forEach() method is the wrong tool.

Early termination may be accomplished with looping statements like for, for...of, and for...in. Array methods like every(), some(), find(), and findIndex() also stops iteration immediately when further iteration is not necessary.

Batwam commented 1 year ago

Ok, it looks like .find() might be the way to go.

Batwam commented 1 year ago

See proposed implementation using .find().

Note that I have left the matchedEntries[0] fallback but I'm not convinced it is necessary (or preferable) to show the icon of an app which isn't running as it's quite likely to be an incorrect icon. I would personnally prefer returning undefined and let the extension return the default icon so we know the app wasn't found.

Moon-0xff commented 1 year ago

Maybe it's more likely that _matchRunningApps fails than matchedEntries[0] is a wrong icon. I really don't know but we didn't crosscheck with running apps until recently and since #27 the only issues with icons so far has been #33

Batwam commented 1 year ago

yeah, _matchRunningApps is the function we worked on recently in #29 . The objective was to fix the function for Firefox and Rhythmbox which return multiple results and matchedEntries[0][1] (and not matchedEntries[0][0]) was actually the correct one. The .desktop is then used to identify the correct icon and player to activate so we need to make sure this is robust.

Turns out we (I) overlooked the unlikely case where different apps are returned in matchedEntries and both are open. This PR should solve this and make the function more robust.

If we keep the matchedEntries[0], it will most likely return something but if we get to this point of the code, it means that it corresponds to an app which isn't currently running and therefore unlikely to be the correct one. If we delete the fallback, this.desktopApp will be undefined. The extension will show the default icon and activatePlayer won't do anything but that's about it. I don't think that desktopApp is used anywhere else in the extension.

Anyway, that's more of a general design philosophy decision regarding whether it's best to return something or leave the extension show that something fails (having made sure it doesn't crash). Let me know how you want to proceed. Either way, I believe that the original issue with Spot is now fixed without affecting the Firefox/Rhythmbox fix from #29.

Moon-0xff commented 1 year ago

You almost convinced me, it's a pretty interesting design choice, but we can also raise a more explicit signal that something doesn't work with the OSD window manager, and we have been lucky getting correct icons from matchedEntries[0]

It all comes down as how likely is _matchRunningApps to fail vs how likely is matchedEntries[0] to deliver the wrong icon, and we don't know the answer of that.

Batwam commented 1 year ago

no worries. Regarding the last point, we know from #29 that Firefox and Rhythmobx have 100% chance of returning the wrong .desktop (on my system). I am not aware of any case where _matchRunningApps would fail, but I also didn't foresee this Spot situation.

I guess worst case scenario would be one where both _matchRunningApps && matchedEntries[0] fail which should be pretty remote as it would be essentially a situation were the Seach returns a completely incorrect app or the identity field isn't populated correctly by the source (e.g. some Browser identifies as Chrome) but we haven't seen this happen (yet).