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

Feature request #16

Open
rag-hav opened this issue Feb 7, 2021 · 17 comments
Open

Feature request #16

rag-hav opened this issue Feb 7, 2021 · 17 comments
Labels
enhancement New feature or request

Comments

@rag-hav
Copy link

rag-hav commented Feb 7, 2021

Love this extension, was wondering if it was possible to make it work with other music players like rhythm and clementine to

@mheine
Copy link
Owner

mheine commented Feb 15, 2021

Hey, thanks for the praise! I haven't used either Rhythm or Clementine, but if they have a CLI interface or have some sort of way of communicating using MPRIS, then it could possibly be integrated. If you have any ideas about how to proceed, feel free to create a Pull Request and I'll look into getting it merged :)

@koolskateguy89
Copy link
Contributor

I have an idea how to implement this.

The destination of the command used to get metadata can be configured to use the current MPRIS player open.

--dest=org.mpris.MediaPlayer2.spotify

We can do this by having a list of known MPRIS players, which according to the KDE Community Wiki are:

  • Amarok
  • Vvave
  • Elisa
  • JuK
  • Plasma Media Center
  • Gwenview
  • Plasma Browser Integration
  • IDEA: Okular (Presentation)
  • VLC
  • Spotify

However they aren't all exclusively music players and I'm not sure of their behaviour with the current command used to get metadata.

We can check which player is running using the command pgrep -i -x [player-name] (e.g. pgrep -i -x spotify). Usually pprep returns the process IDs of all process whose name contains the exact input. The flags (see this website for more info):

  • -i, (--ignore-case) makes it ignore case when checking process name

  • -x, (--exact) makes it return the processes whose name's contents exactly match the input

If the process (i.e. player) is not running, there is no output. However, there is a problem if multiple players are running - I'm not sure how to solve this 'properly' (we could just use first checked open player).

We would just loop through the list of players, checking which is open & saving it in a variable (or returning). We then use this variable to set the destination when running the command to get player metadata (I'm not sure of the destination of players with spaces in their name).

It appears the commands to interact with MPRIS are the same for all, so the 'only' changes are:

  • Checking which player is open
  • Set dbus-send command destination to be specifically that player

Here's a mock-up of code to do this (haven't tested):

const players = ['Amarok', 'Vvave', 'Elisa', 'Spotify', etc...];
function _getPlayer() {
    // check which player is open
    for (let i = 0; i < players.length; i++) {
	const player = players[i];

	let [res, out, err, status] = GLib.spawn_command_line_sync(`pgrep -i -x "${player}"`);

	if (out.length > 0)
	    return player.toLowerCase(); // (the dbus-send command destination uses lower case)
    }
}

// inside SpotifyLabel class
_loadData: function () {
    const player = _getPlayer();
    // check if no player is open
    if (!player) {
        // don't show anything in label
        this._refreshUI('');
        return;
    }

    let [res, out, err, status] = [];
    try {
        //Use GLib to send a dbus request with the expectation of receiving an MPRIS v2 response.
        [res, out, err, status] = GLib.spawn_command_line_sync(`dbus-send --print-reply --dest=org.mpris.MediaPlayer2.${player} /org/mpris/MediaPlayer2 org.freedesktop.DBus.Properties.Get string:org.mpris.MediaPlayer2.Player string:Metadata`);
    } catch (err) {
        this._refreshUI("Error. Please check system logs.");
        global.log("spotifylabel: res: " + res + " -- status: " + status + " -- err:" + err);
        return;
    }

    var labelstring = parseSpotifyData(out.toString());
    this._refreshUI(labelstring);
},

@koolskateguy89
Copy link
Contributor

I'll toy around with this and see if I can get it to work later this week

@koolskateguy89
Copy link
Contributor

koolskateguy89 commented Feb 18, 2021

Tbh if this is properly implemented, it won't be a "Spotify Song Label" anymore but more of "MPRIS Media Label" and might as well be a separate extension.

I'll comment on my findings as I go along instead a massive block comment as I won't test all players in one day.

There is an issue with using pgrep to check if a player is open: if a player has a background process running but is not open, it 'counts' as open - I'm not sure if this makes much sense. This was a problem when testing VLC. We could instead check window actors, similar to how the toggle window feature is implemented.

I've tested VLC with an .mp3 & .m4a and it works fine if the file actually has defined Artist & Title tags. I tested with an .mp4 video and it worked as long as it has defined Artist & Title tags. (I used EasyTAG to check & edit tags)

However there's a problem if the file doesn't have an defined Artist/Title tag - the output of the dbus-send command doesn't include an entry for whichever is missing, thus when getting the index of "xesam:[property]", -1 is returned and this messes up the substring etc. A non-tedious way to solve this is to not show anything when the Artist/Title entries aren't returned by the command.

Details is this commit

  • Amarok
  • Vvave
  • Elisa
  • JuK
  • Plasma Media Center
  • Gwenview
  • Plasma Browser Integration
  • IDEA: Okular (Presentation)
  • VLC
  • Spotify

@mheine
Copy link
Owner

mheine commented Feb 18, 2021

I like the idea! Feel free to create a PR at any point - even if it's unfinished, that makes it easier for me (personally) to look through the changes as you make them 👌

@mheine
Copy link
Owner

mheine commented Feb 18, 2021

Tbh if this is properly implemented, it won't be a "Spotify Song Label" anymore but more of "MPRIS Media Label" and might as well be a separate extension.

Must've missed this segment when I first read your comment. If you want to fork this (or create a completely new repository/extension) to continue this idea of making it a more general MPRIS Label extension (or even just a "Music Player Label Extension?) - then by all means go ahead! I'd appreciate a shoutout if you create a new repo where you use any of my existing code - but could be a a cool idea for an expansion in any case.

@koolskateguy89
Copy link
Contributor

Must've missed this segment when I first read your comment. If you want to fork this (or create a completely new repository/extension) to continue this idea of making it a more general MPRIS Label extension (or even just a "Music Player Label Extension?) - then by all means go ahead! I'd appreciate a shoutout if you create a new repo where you use any of my existing code - but could be a a cool idea for an expansion in any case.

I'm happy to add to it to this extension if it's fine with you. I shouted you out in an extension I made recently where I cloned this repo ;)

@koolskateguy89
Copy link
Contributor

koolskateguy89 commented Feb 19, 2021

I've tested Elisa and it works fine as well. Right now I'm testing out using the window actors and it seems to work, the only problem is it might be a bit slow - it's essentially getting the intersection of two arrays and the implementation I currently have looks to be O(n2), I'm researching potentially linear ways to do it now :)

  • Amarok
  • Vvave
  • Elisa
  • JuK
  • Plasma Media Center
  • Gwenview
  • Plasma Browser Integration
  • IDEA: Okular (Presentation)
  • VLC
  • Spotify

@koolskateguy89
Copy link
Contributor

koolskateguy89 commented Feb 19, 2021

Found a linear way to check which player is open. I commented it in code but I'll quote it here

Javascript Object is implemented using a HashTable (https://stackoverflow.com/a/24196259).
So to enable faster checking for players, 'players' should be implemented as an Object instead of an Array. Then
we can just iterate through the current open windows and check if that window is in players. Thus O(n) instead
of O(n^2) of checking if the window is in the players array. The Object should map the 'window_actor.meta_window.wm_class'
(i.e. what is given after mapping a windowActor to its name) to the name that is used to identify the path in the dbus-send
command.

Commit here, I'll create a PR in a min.

Also another problem I noticed was that if the label is in between other elements (with padding), when Spotify is closed and greeting off, there is just empty space which is a bit inconvenient. I have a few ideas how to solve this and will work on it, create an issue & PR later.

@koolskateguy89
Copy link
Contributor

There are more players that supported MPRIS, including Clementine and RhythmBox which the issue OP wanted, I'll add these tomorrow probably.

@koolskateguy89
Copy link
Contributor

I've found a problem with using window actors to check which players are open - JuK and VLC (and maybe more) can both be open in the system tray but without an open window, I'm really not sure how to circumvent this and actually check which is open - using pgrep as previously suggested may work

@koolskateguy89
Copy link
Contributor

koolskateguy89 commented Feb 19, 2021

Rhythmbox & JuK work as long as the song has defined artist & title entries.

  • Amarok
  • Vvave
  • Elisa
  • JuK
  • Plasma Media Center
  • Gwenview
  • Plasma Browser Integration
  • IDEA: Okular (Presentation)
  • VLC
  • Spotify
  • Rhythmbox
  • Clementine

@rag-hav rag-hav closed this as completed Feb 20, 2021
@rag-hav rag-hav reopened this Feb 20, 2021
@koolskateguy89
Copy link
Contributor

koolskateguy89 commented Feb 21, 2021

@rag-hav If you clone my fork and checkout to the multiple-player-dev branch it works perfectly with Rhythmbox & Clementine atm.

@mheine mheine added the enhancement New feature or request label Feb 21, 2021
@rag-hav
Copy link
Author

rag-hav commented Feb 26, 2021

@rag-hav If you clone my fork and checkout to the multiple-player-dev branch it works perfectly with Rhythmbox & Clementine atm.

It works. Big big thanks.

@Filbuntu
Copy link

Filbuntu commented Mar 11, 2021

Hi there! When I saw the article on UbuntuHandbook, I thought the same as @rag-hav 😄 . Unfortunately I am not a programmer. Could you, e.g. @rag-hav or @koolskateguy89, explain to me how to get the fork and install it?
I would use for Rhythmbox. Thanks in advance!

@koolskateguy89
Copy link
Contributor

@Filbuntu

(You can copy and paste these commands)

Uninstall your current installation:

rm -r ~/.local/share/gnome-shell/extensions/gnomespotifylabel@mheine.github.com

Clone the multiple-player-dev branch into the extensions folder

git clone https://github.com/koolskateguy89/gnome-shell-spotify-label.git --branch multiple-player-dev --single-branch ~/.local/share/gnome-shell/extensions/gnomespotifylabel@mheine.github.com

@Filbuntu
Copy link

Thank you very much, @koolskateguy89. This works great!
Hopefully this fork can be integrated into the original extension or made another official extension!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants