Activate Source on Click Option

[Batwam]

Simple code implementation to open player on click. As long as the .desktop selection is fixed, a simple .activate() will work reliably so it becomes 2 simple lines of code.

[Batwam]

by the way, what does entry.launch in row 242 of players.js do? It looks like it would launch the app but it's odd as it's part of the icon function and also, Gio.DesktopAppInfo doesn't have launch but only launch_action which as I have tested runs actions listed in .Desktop files.

[Moon-0xff]

That line was introduced in #27, specifically this commit

[Batwam]

ok, I should have checked that! This was a testing leftover so I have now removed the line (and confirmed it has no impact).

Are you happy with the way this PR works?

[Moon-0xff]

Using var is problematic due to scope reasons, and I would prefer Array.forEach instead of normal of normal for loops.
Overall it's good.

[Batwam]

Thanks for the feedback, I wasn't aware of the var thing although I did notice that it wasn't used anywhere else so that's good to know. I think that I tried the forEach originally but couldn't quite make it work. Let me look into this again.

[Batwam]

ok done.

It was a little bit fiddly as matchedEntries is a multi-dimensional array so a single forEach wouldn't work and I had to have 2 nested forEach. The original nested for loop idea came from here, the nested forEach loop from here.

[Batwam]

Last commit is slightly unrelated but I noticed that the current code prevented the open menu when hide label is active (which is a default option) as it would clear the current player. (It also prevents playing the sound again on play-pause click after pause which is even more problematic)

[Moon-0xff]

De-nested matchedEntries for the sake of clarity. Arguably a worse implementation but i think it's worth it.

edit: should the last match take priority?

[Batwam]

I actually just had a brain wave and realised from this old check that the result we are looking for is always going to be in matchedEntries[0][x] and we only need to loop through the values in matchedEntries[0]. See simplified code in next commit, successfully tested on Firefox, Chrome, Amberol, Rhythmbox, Spotify.

Also, as far as I can tell, you can't exit a forEach loop with return, it will always run its course so I removed the return entry inside the forEach.

On the last point, every playerObject is unique as far as I can tell as I haven't seen any case of muliple matches so I don't think that there will be a first or last.

[Moon-0xff]

DesktopAppInfo.search returns a sorted list of matches bundled by score. I suppose there's no guarantee that the correct result will always get the highest lower score, but all players that the first match isn't the correct result is because the first match was a symbolic link to the second, so maybe it's true that we only need to loop through [0]? I'm not sure.

return entry will indeed work here, using x => { //code } doesn't change the scope, but (x) => { //code } does. Or maybe the nesting changes the behaviour of return, not sure.

[Batwam]

I think that I spoke too quickly. I just checked matchedEntries.toString() to get a dump of all results and noticed that chrome does have matchedEntries[1][0], however, it's not a match. Doesn't really make any difference to the end result but cycling through matchedEntries[i][j] maybe be needed. On this basis, do you want to revert back to your more generic previous implementation and we call it a day?

[Moon-0xff]

Yes, i suppose, if you agree?
By the way, does return entry works on your end? It might be a difference between gjs versions.

[Batwam]

yeah, I agree that it's best to keep the double nest code you wrote yesterday just to be safe.

On the nested return, as far as I can tell, it will keep cycling whether regardless if (x) or not:

[Related thread on StackOverflow](https://stackoverflow.com/questions/2641347/short-circuit-array-foreach-like-calling-break#:~:text=There%20is%20no%20way%20to,()%20or%20some()%20instead.)

[Moon-0xff]

Does return match works if the arrow function is declared with (entry,match) => {?

[Batwam]

that leads to the same result on my end. It doesn't really matter as there are only a handful of search results to cycle through. I'd say let's leave as you had it and merge so we can start for on the volume control?

[Moon-0xff]

Agreed, but first: is Players.pick behaving the same way after applying 670f248? The removed code looks non-trivial

[Batwam]

Yeah, removing these 2 line worked for me. I don't see any reason the player picking method should behave differently if the text is hidden.

[Moon-0xff]

Well, i haven't tested it myself but it shouldn't have any unintended effect (i hope). I'm going to merge this now.