search

JasonLG1979 / gnome-shell-extension-mpris-indicator-button

A full featured MPRIS indicator button extension for GNOME Shell 3.38+
https://extensions.gnome.org/extension/1379/mpris-indicator-button/
GNU General Public License v3.0
159 stars 21 forks source link

Detecting shuffle or loop status is broken #35

Closed fcastilloec closed 4 years ago

fcastilloec commented 4 years ago

Describe the bug

Reading the code, it seems that the function _testShuffleLoopStatus toggles the shuffle and loop to test if these are working. I have an MPRIS player (Headset) that has both shuffle and loop implemented correctly but they only work when a song is playing, otherwise, they don't do anything. So, when starting Headset and playing a song, this extension doesn't show either the loop or shuffle buttons. After toggling shuffle or loop from within the app, both buttons are shown in this extension but only the one that was toggled works. For example, if I shuffle within Headset, only the shuffle button will work in the extension, so pressing the loop button doesn't work even though my app can receive that mpris message.

So, maybe detecting that this._playerProxy.Shuffle !== null and this._playerProxy.LoopStatus !== null might be enough. Or maybe, it'll be better to run _testShuffleLoopStatus inside _updatePlayerProps?

Steps To Reproduce:

  1. Start Headset
  2. Play any song
  3. Check that this extension doesn't show either loop or shuffle buttons
  4. Toggle shuffle from within the app (or using any other mpris controller (i.e. playerctl)
  5. See that both shuffle and loop buttons are displayed in the extension but only shuffle works.

Expected behavior

After playing a song, both loop and shuffle buttons should be displayed and working.

System Details (please complete the following information):

Additional Notes:

From my player's point of view, there's no reason to change either the shuffle or loop status when nothing is playing (stopped not just paused). This will open the alternative that toggling shuffle/loop when nothing is playing will de-synchronize the internal status of the player with mpris. Not a lot of people will do this, but it's better to prevent this situation rather than introduce weird behavior.

JasonLG1979 commented 4 years ago

LoopStatus and Shuffle are optional properties, they are allowed to not exist. In Gjs's dbus proxy implementation null means they don't exist. Testing against null only tells us if the property exists, it doesn't tell us if it actually works. The only way to actually tell if maybe it works is to change the property and see if the player sends a property changed signal. There are a number of players that return bogus values for properties that actually don't work.

I have an MPRIS player (Headset) that has both shuffle and loop implemented correctly but they only work when a song is playing, otherwise, they don't do anything.

Not true. If a player puts up an incomplete interface it is not implemented correctly. All properties that the player wishes to implement should be present and functional when the interface is put up.

Having a property appear and disappear is a bug on the part of the player.

fcastilloec commented 4 years ago

Not true. If a player puts up an incomplete interface it is not implemented correctly. All properties that the player wishes to implement should be present and functional when the interface is put up.

Having a property appear and disappear is a bug on the part of the player.

The properties are all present when the interface is up, it's set to false for Shuffle and None for LoopStatus, they never disappear. If a signal comes from mpris asking it to change one those properties, and nothing is playing, the queue is empty, why should those properties change? It's like sending the play signal from mpris and expecting PlaybackStatus to change even if there's no songs or playlists loaded. The player doesn't know what to play so why should it report a change in its property. Shouldn't the same be valid for things like Shuffle and LoopStatus?

JasonLG1979 commented 4 years ago

Shuffle and LoopStatus are playback modes, it does not matter if there is nothing to play. It has nothing to do with what is currently playing or what isn't. It determines what happens after a track is played or in the case of hitting play on a playlist with shuffle toggled what will play. By your logic I couldn't shuffle a playlist until after it has started to play.

JasonLG1979 commented 4 years ago

The expectation being that I should be able to change playback mode at anytime similar to volume. Why be able to change the volume when there's nothing to play? Your same argument could be made.

fcastilloec commented 4 years ago

By your logic I couldn't shuffle a playlist until after it has started to play.

I understand that It makes sense to shuffle or loop a playlist before it starts playing if your player has local music, or in the case of Spotify, their music seems "local" from Spotify's point of view. My player uses youtube videos as the source, so it actually doesn't make sense to shuffle a playlist before it has started playing, that concept doesn't exist. You could start playing a playlist in shuffle mode, which we do implement, so shuffling happens at the same time as you start to play. The exact same behavior happens with looping, you can't tell youtube to loop a playlist if you haven't started playing it. I guess the way youtube music works wasn't designed with MPRIS and desktop players in mind. I understand that just checking for null is not a good idea since many players might not implement MPRIS correctly but what about just running _testShuffleLoopStatus inside _updatePlayerProps?

JasonLG1979 commented 4 years ago

so it actually doesn't make sense to shuffle a playlist before it has started playing, that concept doesn't exist.

Sure it does just grab the playlist and randomize the order yourself.

The exact same behavior happens with looping, you can't tell youtube to loop a playlist if you haven't started playing it.

Again just play the track or playlist over again.

You depend on YouTube to implement shuffle and repeat for you? That seems like you're just asking for trouble.

JasonLG1979 commented 4 years ago

I would assume you get playlists as an array of urls?

JasonLG1979 commented 4 years ago

what about just running _testShuffleLoopStatus inside _updatePlayerProps?

I prefer not change properties that often. That's just noise on the bus. That function gets called A LOT. Not to mention that it most likely cause the Shell to lock up because of recursion. Calling _testShuffleLoopStatus inside of _updatePlayerProps would cause _updatePlayerProps to be called again once a prop change signal was emitted.

fcastilloec commented 4 years ago

You depend on YouTube to implement shuffle and repeat for you? That seems like you're just asking for trouble.

We don't, we actually do implement shuffle and repeat in our app ourselves, we get an array and do the repeating and shuffling ourselves, not depending on youtube. We were trying to keep the experience as close to how youtube actually works on their site, which is not compatible with how things work on a regular desktop player.

I was hoping to not change the experience of the app just to have your extension working. Since the older MPRIS extension was deprecated, more people have complained about using this new one. The old extension didn't have any trouble with shuffle or loop so I went checking how things were done nwo. I thought a simple change in where you run _testShuffleLoopStatus will be easier to implement than changing the expectation on how our app should work.

Is this is something that can't be done from your side, I totally understand.

JasonLG1979 commented 4 years ago

We were trying to keep the experience as close to how youtube actually works on their site, which is not compatible with how things work on a regular desktop player.

Well you kinda are a regular desktop player so if I had to go one way or the other I'd behave more like a regular desktop player.

fcastilloec commented 4 years ago

I totally get that making the change will create more potential problems and it might not be worth it. We'll have to think of something from our side. Maybe some workaround, like checking for consecutive shuffle/loop messages coming right after the start of our app from MPRIS so we can actually respond to those.

Implementing things as a regular desktop player will be a bigger change that we'll have to discuss further from our side.

Thanks again for taking the time to reply here. I really appreciate all the work you do and I'm a big fan of your work.

JasonLG1979 commented 4 years ago

I totally get that making the change will create more potential problems and it might not be worth it.

You can blame the ambiguity of the MPRIS spec in general and misbehaving player in particular. In a perfect world everyone would be on the same page and all players would implement the spec correctly and I wouldn't have to sniff and poke properties to make sure they work.

I'll tell you what. What's the exact player name as presented to MPRIS in the Identity property? I'll whitelist LoopStatus and Shuffle in the extension to save us both dealing with pissed off users.

fcastilloec commented 4 years ago

You can blame the ambiguity of the MPRIS spec in general and misbehaving player in particular. In a perfect world everyone would be on the same page and all players would implement the spec correctly and I wouldn't have to sniff and poke properties to make sure they work.

I totally agree with you. When trying to implement MPRIS for the first time the documentation wasn't as straight forward, and relying on how other players implemented things didn't help. The only thing that actually helped me to get it correctly was your old extension. If something wasn't working with that it was because we did something wrong.

I'll tell you what. What's the exact player name as presented to MPRIS in the Identity property? I'll whitelist LoopStatus and Shuffle in the extension to save us both dealing with pissed off users.

I really appreciate that! You're saving us from a lot of work! Thanks so much! Our player's Identity is Headset

JasonLG1979 commented 4 years ago

I really appreciate that! You're saving us from a lot of work! Thanks so much! Our player's Identity is Headset

Right on. I downloaded and am testing Headset right now. The only issue I can see so far is that Headset is showing as 2 MPRIS interfaces. One being the intentional org.mpris.MediaPlayer2.headset and the other org.mpris.MediaPlayer2.chromium.instancerandom number. Chromium/Chrome as of v75 puts up it's own broken interface and I'm guessing electron being based on Chromium does the same.

Running headset with --disable-media-session-service prevents the 2nd broken interface from being created.

JasonLG1979 commented 4 years ago

It doesn't affect my extension because Chromium/Chrome mpris interfaces are ignored because they're broken. It will however show up twice in the default GNOME MPRIS controls.

JasonLG1979 commented 4 years ago

See: https://github.com/JasonLG1979/gnome-shell-extension-mpris-indicator-button/issues/10

fcastilloec commented 4 years ago

Thanks for finding that! I've never noticed it before. If debugging from the command line, I made sure to only see messages from org.mpris.MediaPlayer2.headset so never even check for another interface. On d-feet I only see Headset under org.mpris.MediaPlayer2 and nothing else. I'm testing on Ubuntu 20.04, and Headset is running on electron 8 which has Chrome v80, what OS do you use?

I'll just make sure that Headset runs with --disable-media-session-service at least on Linux. This might not make sense for Windows or macOS since they don't use MPRIS and very likely depend on the media-session-service to implement media keys

JasonLG1979 commented 4 years ago

I'm on Ubuntu 20.04 also. D-feet is also my go to for quick dbus debugging. I noticed it because I wanted to see how Headset fared in the default controls.

JasonLG1979 commented 4 years ago

This might not make sense for Windows or macOS since they don't use MPRIS and very likely depend on the media-session-service to implement media keys

Most Linux DE's are going to MPRIS for media controls also. That's one reason Chromium puts up the interface. Unfortunately MPRIS lacks any way to track which player is the "active" player so what ends up happening is that DE's just hands the mediakeys to the latest player to show up so you end up with situations where Chromium/Chrome will steal the mediakeys and has no way to give them back to any other player.

fcastilloec commented 4 years ago

I see it now!!! It's only shown once something starts playing. It's not loaded when the player starts. Thanks again for finding that.

JasonLG1979 commented 4 years ago

I have no idea how mediakeys work on Windows or Mac. I haven't used either in over 10 years.

fcastilloec commented 4 years ago

I'll move the conversation regarding disable-media-session-service flag to #10 because it's not really doing anything.

JasonLG1979 commented 4 years ago

I have a couple other nitpicks.

  1. I'm not sure if it's really possible but sometimes the artist field is empty and the title takes the form of something like Artist - Title it would be nice if there where anyway to do so to fix that. I realize that's what you get from YouTube but if there is any sort of pattern to it it maybe possible to split those.

  2. The thumbnail/Cover art is really low res and often distorted. The Thumbnails in the app look alright though.

JasonLG1979 commented 4 years ago

Generally speaking the defacto unofficial album art size is about a 500x500px square.

fcastilloec commented 4 years ago
  1. I'm not sure if it's really possible but sometimes the artist field is empty and the title takes the form of something like Artist - Title it would be nice if there where anyway to do so to fix that. I realize that's what you get from YouTube but if there is any sort of pattern to it it maybe possible to split those.

I can look into it, but we've have trouble with this in the past and might not be possible.

  1. The thumbnail/Cover art is really low res and often distorted. The Thumbnails in the app look alright though.

This happens because Youtube uses a rectangle for the thumbnails instead of a square, and that's what we pass to MPRIS. I've never been happy with the way they are displayed because they, indeed, look distorted. Headset can easily put a circle on the center of a thumbnail to only display that on the app using javascript, instead of cropping the image. To send a nice image to MPRIS, we would have to crop each image. This has been on my TODO list for a while, and I've avoided it because of the overhead of having to do image manipulation for each thumbnail. I don't expect your extension to do the cropping if they have the wrong size, so we've lived with that ugly thumbnail for a while now.

JasonLG1979 commented 4 years ago

I can look into it, but we've have trouble with this in the past and might not be possible.

I figured you were at the mercy of what YouTube gives you.

This has been on my TODO list for a while, and I've avoided it because of the overhead of having to do image manipulation for each thumbnail.

How much overhead is it actually? Not much I'd imagine.

I don't expect your extension to do the cropping if they have the wrong size, so we've lived with that ugly thumbnail for a while now.

The only thing I do is force it to be a square not because of this but because some themes murder the style sheets and try to force cover to be a set width. If I don't force a square the cover ends up being 16px wide, which makes all covers look like shit.

JasonLG1979 commented 4 years ago

For reference the cover size is dynamic, based on the height of the track info text next to it. So whatever the height of the text is the size of the cover art in a square.

JasonLG1979 commented 4 years ago

OK. Round 2 of nitpicks. Icons.

  1. The preferred icon format for Linux is svg. You can get fancy and pixel fit various sizes if you want.

  2. The icon looks fine at your average launcher size but it's somewhat of a blob at panel/tray icon size. This is a case for pixel fitting and/or designing a slightly simplified panel/tray sized icon.

  3. A monochrome "symbolic" icon would be a nice addition. From what I've seen that seems to be the way Windows is going for their tray icons and GNOME goes so far as to desaturate full color icons in the panel. This would also help with the blob situation.

fcastilloec commented 4 years ago

1. The preferred icon format for Linux is svg. You can get fancy and pixel fit various sizes if you want.

How does your extension get the icon of the app? For Linux, we provide icons in two places, the one used in the window itself and the one in the .desktop file. I'm guessing you're using the icon in the .desktop file, which we can easily make it SVG.

3. A monochrome "symbolic" icon would be a nice addition. From what I've seen that seems to be the way Windows is going for their tray icons and GNOME goes so far as to desaturate full color icons in the panel. This would also help with the blob situation.

How do you specify a secondary/alternative icon? Do you add a group header (which one?) in the .desktop file with another icon there? Or do you look for a particular name (suffix or prefix) in the default icon folders for the monochrome icon?

EDIT: Or do you look for a particular icon size? We could provide the colored SVG and a 32x32 PNG, or smaller, as monochrome, whichever size you use.

JasonLG1979 commented 4 years ago

How does your extension get the icon of the app? For Linux, we provide icons in two places, the one used in the window itself and the one in the .desktop file. I'm guessing you're using the icon in the .desktop file, which we can easily make it SVG.

Electron seems to do a good job of apparently putting the full color icon where it's supposed to be because the extension displays it just fine.

A 128 x 128 svg for the full color icon should do the trick.

How do you specify a secondary/alternative icon? Do you add a group header (which one?) in the .desktop file with another icon there? Or do you look for a particular name (suffix or prefix) in the default icon folders for the monochrome icon?

symbolic icons are 16 x 16px one color (bebebeff) icons with names end in -symbolic so if your full color icon is Headset.svg the symbolic icon is Headset-symbolic.svg The theme will replace bebebeff with what ever color it wants, so on light panels it will be a dark color and on dark panels it will be a light color. Generally speaking transparencies or gradients are discouraged and you want to pixel fit the icon since it's so small so it looks nice and crisp.

I'm not sure how to go about putting the icon in the proper place when building an electron app but it goes in:

icons > hicolor > symbolic > apps

fcastilloec commented 4 years ago

Thanks for the info! Support for this type of icons when packaging electron apps will be added soon, see https://github.com/electron-userland/electron-installer-common/pull/70

fcastilloec commented 4 years ago

Quick question, for the cover art, can you extension accept a base64 encoded jpg/png URI? Something like: mpris:artUrl: 'data:image/jpeg;base64,........'

JasonLG1979 commented 4 years ago

Quick question, for the cover art, can you extension accept a base64 encoded jpg/png URI? Something like: mpris:artUrl: 'data:image/jpeg;base64,........'

I'm not sure but I wouldn't. The spec is not clear on what format to use but I would stick to regular jpg and/or png. That's your safest bet to work with any MPRIS widget not just mine.

EDIT: No. mpris:artUrl is expected to be a url, preferably a local file.