rayzr522 / now-clocking

a conky widget that shows now playing information when music is playing or a clock when music is not playing
49 stars 14 forks source link

Add VLC support, and bug fixes #1

Closed jgabriel98 closed 4 years ago

jgabriel98 commented 4 years ago
rayzr522 commented 4 years ago

just taking a preliminary read over (I'm in mobile so will review properly when I'm back on my desktop), and I'm curious -- how does this solve the issue of multiple players being open? for all 3 players it appears we are still only checking that they're open/running in npart.lua, not that they're actively in a playing state, or that they were the last player to be in a playing state (but may now be paused)

jgabriel98 commented 4 years ago

just taking a preliminary read over (I'm in mobile so will review properly when I'm back on my desktop), and I'm curious -- how does this solve the issue of multiple players being open? for all 3 players it appears we are still only checking that they're open/running in npart.lua, not that they're actively in a playing state, or that they were the last player to be in a playing state (but may now be paused)

in the npart.lua script, we had the following code logic before:

${if_running spotify}${exec ./scripts/fetch-art spotify}
    ${image ./data/spotify.png -p 0,0 -s 125x125 -n}
${endif}
${if_match "" != "${exec playerctl -p vlc status}"}${exec ./scripts/fetch-art vlc}
    ${image ./data/vlc.png -p 0,0 -s 125x125 -n}
${endif}
${if_running cmus}${exec ./scripts/fetch-art cmus}
    ${image ./data/cmus.png -p 0,0 -s 125x125 -n}
${endif}

So, it checked for every player, and if it was playing, then it would update the art. So it means that the last active player (paused or not) would overwritte the art image.

note that the logic for checking active players in npart.lua is different from the np.lua, where the npart.lua reaches all ${if_} (it uses a bunch of "if ... if... if ..."), but the np.lua only enters one ${if_} (because it uses chained if-else-if-else-... logic like)

now we have this new code logic:

${if_running spotify}${exec ./scripts/fetch-art spotify}
    ${image ./data/spotify.png -p 0,0 -s 125x125 -n}
${else}${if_match "" != "${exec playerctl -p vlc status}"}${exec ./scripts/fetch-art vlc}
    ${image ./data/vlc.png -p 0,0 -s 125x125 -n}
${else}${if_running cmus}${exec ./scripts/fetch-art cmus}
    ${image ./data/cmus.png -p 0,0 -s 125x125 -n}
${endif}
${endif}
${endif}

now it follows the same if-else logic from np.lua script, where it enters only one active player (the first one), because the logic now uses chained if-else s

jgabriel98 commented 4 years ago

(you're welcome to merge whenever you're ready)

I don't have write permission to your repository... :/ So you can merge it, or you can give me write permissions (which i don't think is a very safe solution)

rayzr522 commented 4 years ago

odd, I thought me approving it gave you the ability to merge. perhaps I have something misconfigured. I'll merge it now, thanks!

jgabriel98 commented 4 years ago
  1. while this will now show only one album art at a time and not get as confusing, this does still have the problem of not respecting WHICH player is currently playing -- if I understand the ordering correctly, if I had spotify and cmus open and was playing music in cmus, since spotify is the first conditional, it would try and show art from spotify despite cmus being the one playing. the solution to this is non-trivial since it isn't as simple as showing the currently playing player; you also have to know what was the most recently playing player, since once you pause your music, it has to go off of the past to determine what one you want to see info for.

For that, the following logic would solve it:

inside the conky main loop (the main 1sec loop):

  1. run though playerctl playerList, and get each player playing status (playing, stopped or paused)
  2. compare those status to their previous status, something like this: spotify_prev_status != spotify_current_status
  3. if the status had changed, use this player as the active one for conky 'now playing' if not, keep the old active one player

note: the step which runs through the players comparing it status, can be done in a chained 'if else', since it is very unlileky to the user use more than one player at the same second interval.

This is just a suggestion that came to my mind when reading your post, maybe there's a better way to solve it, but i think this should be enought.

rayzr522 commented 4 years ago

i think actually the best way to do this, building off of the player-agnostic idea, would be to do what you suggested except in step #2 instead of comparing a previous & current status for each individual player, we just have a file with a "last playing player" stored in it. this gets updated any time that value changes. then in all the other metadata/art scripts, all they have to do is fetch the last playing player, see if it's still playing, and if so fetches the metadata/art

but yeah I think that's the general idea, you're welcome to contribute this if you would like otherwise I might try and tackle this in the upcoming weekend :)