plexamp play-pause workaround

[Batwam]

I recently started to play with Plex and when installing Plexamp, I noticed that while I could Pause, I couldn't resume the songs. the rest of the funcitonalities works (artwork, song info, ...) so that's good.

on further inspection, he reason PlayPause is flakey is because Plexamp incorrectly sets CanPause to false when the song is paused (see below, CanPause changing from true to false):

If your widget checks for CanPlay && CanPause at the same time to trigger PlayPauseRemote, it will assume that is can’t control the song. Normally, players leave CanPlay and CanPause to true when these functionalities are enabled which is the way the specification intends it to be so it’s a bug.

I have reported this bug on the Plexamp forum. However, since it's proprietary, I doubt the Plex team is going to fix it anytime soon so I took it upon myself to include a workaround. Basically, we need to only check CanPlay if the song status is Paused.

Note that technically, we could remove <method name="Play" /> and use this.proxy.PlayPauseRemote(). The main reason I'm using Play is because technically, CanPause is false and using Play is a fit clearer in my view. We could change that if you find this overly pedantic.

[Moon-0xff]

Perhaps we should just ignore CanPlay and CanPause and simply send the PlayPauseRemote signal, then check if PlaybackStatus changed (for the Stop fallback workaround).

[Moon-0xff]

As the commit states I haven't tested it.

[Batwam]

I just gave it a got but it didn't work as it was stopping the song every time I wanted to pause. I'm not sure if there is a lag or something but when pressing, even after the pause action is triggered, this.proxy.PlaybackStatus is showing Playing:

[Moon-0xff]

Makes total sense that the application can't switch status fast enough.

I guess we could add a small timer to the Stop workaround, adding a timer to the play/pause action sounds like unneeded complexity, but it seems to me like the best way to avoid buggy MPRIS interfaces, which aren't rare.

[Batwam]

yeah, or we keep things even simpler and only leave this.proxy.PlayPauseRemote(). Is there really a scenario where the user would want to stop the song on click considering that this means that the user won't be able to resume and lose where he/she was in the song? I think that a fall back where it does nothing would be fine too. I also haven't seen any app where play/pause hasn't been implemented.

I have pushed a proof concept with a timer. Based on my pc, 100ms is sufficient to get an updated PlaybackStatus, 50ms is too short. I put 1000ms to be safe as this is done asynchronously and we don't want false positives but to be frank, I think that it adds complexity and could potential still trigger if the machine was extra slow. I'd be keen to keep the code simple considering how little time we spend on it these days but if you'd rather keep that, it's fine with me too.

[Moon-0xff]

The Stop workaround was added because rhythmbox doesn't respond to Pause signals when using the radio (#48).
Though adding the workaround with a timeout, specially such a long timeout will probably bring more trouble than it solves.

I guess it's best to revert the changes to 98f025e

[Batwam]

Yeah, I think so too. Would you be able to revert and merge? I'm afraid I might mess it up.

[Moon-0xff]

For the record:

git revert HEAD~2..HEAD --no-commit
git commit

(without the --no-commit flag it becomes two separate commits).

[Moon-0xff]

It's a little bit frustrating to have two distinct workarounds on such a simple (or supposed to be simple) function.
But well, Software Development!

A one-size-fits-all solution against any buggy mpris interface sounds to me now, well, impossible.
I guess we are doing it well adding workarounds (or not adding them) in a case by case basis,
but that's work I personally would prefer to avoid.

[Batwam]

yeah, it's a bit frustrating having to work around improper implementation but that's probably a necessary evil. On the plus side all these players support the same standard and most of it works so it's not too bad. At least this isn't affecting performance for players which properly implement is since is should exist with the first this.proxy.CanPlay && this.proxy.CanPause check.