flathub / com.spotify.Client

https://flathub.org/apps/details/com.spotify.Client
75 stars 35 forks source link

Can't Raise player via MPRIS #4

Open JasonLG1979 opened 7 years ago

JasonLG1979 commented 7 years ago

Neither by the actual MPRIS raise method nor by activate_full (How most MPRIS Clients actually raise players to prevent focus stealing)

The only log message I get is: Failed to start flatpak-com.spotify.Client-25904.scope.

Every time I start Spotify.

TingPing commented 7 years ago

Hmm, well I can reproduce Raise() not doing anything but it doesn't cause any error either. (On a side note Pithos doesn't seem to actually be raised either, related to your focus stealing comment probably).

JasonLG1979 commented 7 years ago

I haven't tested the actual raise method on a non-flatpak install. Spotify's MPRIS implementation is garbage. I wouldn't be surprised if it didn't actually work, but activate_full does. I really don't know anything about flatpak but maybe it's a permissions thing?

TingPing commented 7 years ago

Permissions seem fine, though its probably worth testing a non-flatpak version to see if flatpak is even relevant.

JasonLG1979 commented 7 years ago

I have tested the non-flatpak version a lot to try to work around the crappy MPRIS implementation. I ended up disabling all but the most basic functionality in my extension because it's so bad, but activate_full works at least so it can be raised.

JasonLG1979 commented 7 years ago

Honestly though the desktop Spotify client is just crap all the way around on Linux anyway. (flatpak or not) It's a huge memory hog and it hangs every time I try to close it. I have to kill the processes. Linux users are better off just using the web interface. The only thing you'll be missing is local playback(there are a billion music players anyway) and barely functional MPRIS support. I basically just filed this bug as a marker for my known player bugs wiki page for the extension.

JasonLG1979 commented 6 years ago

OK I've managed to workaround this.

A summary of the issue:

  1. Spotify has no native Mpris raise method.
  2. In GNOME Shell the Shell App System can not find the "app" because the provided Mpris "DesktopEntry" prop does not match the Flatpak'd desktop entry.

Here is a workaround:

let appSystem = Shell.AppSystem.get_default();
let shellApp = appSystem.lookup_app("DesktopEntry") || appSystem.lookup_startup_wmclass("Identity");

Where "DesktopEntry" = the Mpris "DesktopEntry" prop and "Identity" = the Mpris "Identity" prop

You can then call shellApp.activate(); to raise Spotify.

The best solution would be for Spotify to package their own Flatpak...

JasonLG1979 commented 6 years ago

To expand on my workaround here is a "focus wrapper" for ShellApp that can keep track of different instances.

https://gist.github.com/JasonLG1979/28e967fbbac27463f890119afd6beb34

JasonLG1979 commented 6 years ago

@TingPing Give it a try. https://github.com/JasonLG1979/gnome-shell-extensions-mpris-indicator-button

JasonLG1979 commented 6 years ago

Not only can you raise the Spotify Flatpak you can also minimize it. Right clicking the indicator icon acts like a raise/minimize toggle.

TingPing commented 6 years ago

@JasonLG1979 btw did you see this: https://lists.freedesktop.org/archives/mpris/2018q3/000074.html

JasonLG1979 commented 6 years ago

Here is my sort to tell which player I consider the active player. https://github.com/JasonLG1979/gnome-shell-extensions-mpris-indicator-button/blob/master/mprisindicatorbutton%40JasonLG1979.github.io/extension.js#L860-L884

JasonLG1979 commented 6 years ago

I really don't think we want players telling us when they are focused. It's just one more thing for them to F up. A lot of devs would accidently or on purpose screw that up. They would be like "yep I want the media keys so I'm going to signal 20 times a sec that I'm focused no matter what."

TingPing commented 6 years ago

@JasonLG1979 Your behavior relies on extensions being part of the compositor, but there will exist some mpris client that cannot access focus time and would need the players to tell it.

JasonLG1979 commented 6 years ago

If a player isn't a GUI app it really can't have focus. I mean how would you even define "focus" in that case? In the case of, say a command line player I already have "statusValue" and "statusTime" factored in the sort. statusValue being a rank based on PlaybackStatus, Playing > Paused > Stopped and statusTime being the last time the PlaybackStatus changed.

TingPing commented 6 years ago

I mean how would you even define "focus" in that case?

It returns 0 and is ignored?

JasonLG1979 commented 6 years ago

It returns 0 and is ignored?

Then the prop would be redundant. I can already tell if and when a player with a GUI is focused. A player without a GUI telling me it's not focused is pointless. I already know it doesn't have a GUI and therefore can't be focused.

JasonLG1979 commented 6 years ago

I think it should be up to the desktop/window system to decide what is "focused".

JasonLG1979 commented 6 years ago

If it's a matter of gapplication's not being able to have true separate instances and therefore you can't tell which fake instance is focused from the outside then I'd fix that bug instead of introducing a new prop in MPRIS.

TingPing commented 6 years ago

If it's a matter of gapplication's not being able to have true separate instances and therefore you can't tell which fake instance is focused from the outside then I'd fix that bug instead of introducing a new prop in MPRIS.

It isn't a bug, you either use GApplication (whos entire point is to have unique instances) or you disable the majority of GApplications functionality and no longer have unique instances.

JasonLG1979 commented 6 years ago

It isn't a bug, you either use GApplication (whos entire point is to have unique instances) or you disable the majority of GApplications functionality and no longer have unique instances.

It seems like there is a valid use case for multiple real instances.

JasonLG1979 commented 6 years ago

Anyway, if your going to start introducing stuff like focus into MPRIS you may as well create a new interface. something like org.mpris.MediaPlayer2.WindowControls

There is already Raise and FullScreen which should be moved to WindowControls. You could also have basic window controls like minimize, maximize and close.

JasonLG1979 commented 6 years ago

And why focus and a number and not just a focused bool. IMHO it's a bad idea for players to send some kind of number representing time. It would be better to just have a focused bool and let the client decide how it wants to deal with time. Otherwise you end up with players having different ways to measure time.

TingPing commented 6 years ago

And why focus and a number and not just a focused bool.

Because the purpose is to send mediakey bindings to the last focused window, which means a timestamp.

JasonLG1979 commented 6 years ago

Because the purpose is to send mediakey bindings to the last focused window, which means a timestamp.

Well someone has to middleman the signal anyway, like I said the client should set the time. Player devs will F that up I promise you.

JasonLG1979 commented 6 years ago

You'll end up with players sending all sorts of invalid time types and value.

JasonLG1979 commented 6 years ago

A bool is much simpler

onfocused() {
    let eventTime = whatever.get_eventTime();
    if (this.proxy.Focused) {
       this.otherTing.doStuff(eventTime);
    } else {
       this.otherTing.doOtherStuff(eventTime);
    }
}
JasonLG1979 commented 6 years ago

If it were up to me org.mpris.MediaPlayer2 would look something like this:

<node>
<interface name="org.mpris.MediaPlayer2">
  <method name="Raise" />
  <method name="Quit" />
  <method name="Minimize" />
  <method name="Maximize" />
  <method name="Close" />
  <property name="CanQuit" type="b" access="read" />
  <property name="CanRaise" type="b" access="read" />
  <property name="Fullscreen" type="b" access="readwrite" />
  <property name="CanSetFullscreen" type="b" access="read" />
  <property name="HasTrackList" type="b" access="read" />
  <property name="Identity" type="s" access="read" />
  <property name="DesktopEntry" type="s" access="read" />
  <property name="SupportedUriSchemes" type="as" access="read" />
  <property name="SupportedMimeTypes" type="as" access="read" />
  <property name="CanMinimize" type="b" access="read" />
  <property name="CanMaximize" type="b" access="read" />
  <property name="CanClose" type="b" access="read" />
  <property name="Focused" type="b" access="read" />
</interface>
</node>

It already has Fullscreen, why not other window controls? Note that Close is distinctly different from Quit. If closing the window would Quit the player the player should set CanClose to False.

memeplex commented 4 years ago

@JasonLG1979 you seem like you can help with https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/3179, would you take a look? Thanks

jr1991-g commented 11 months ago

Latest commits 8d038c57f777a6a36e3e202cdd7a4cce6f486715e5464b9354d36d39c3fd7aa1 (2023-11-30) and f2e534a3b5f444506de97d07d92535548f4cf222ae1d1fb4ff7680490b665934 (2023-11-29) break this functionality.

Last good commit is 7e5bf02adf3346efc2d09c10d9ba73ee51e6ecf8a558ffa6347c9d9c0d3af291 dated 2023-11-06.

Fedora 39, GNOME.

System