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

implement a shift feature for playerctld #173

Merged
merged 12 commits into from
Jun 6, 2020
Merged

Conversation

dmsalomon
Copy link
Contributor

@dmsalomon dmsalomon commented Apr 29, 2020

This implements the feature suggested in #170

The daemon exposes a dbus method called Shift, that the user can interact with to request a shift. At the moment that dbus method needs to be interacted with manually using a tool like dbus-send.

The following command requests a shift:

dbus-send --type=method_call --dest=org.mpris.MediaPlayer2.playerctld /org/mpris/MediaPlayer2 com.github.altdesktop.playerctld.Shift

This dbus command can be incorporated in playerctl, so that the user can do something like playerctl shift which will execute the above dbus method call. I tried my best to implement the dbus method, but I could not figure out how to send a proper response, so I have to use type=method_call instead of --print-reply.

Additionally, I have added some logic to first check the status of the 'new' player before setting it active. This is necessary because some players (i.e. spotify) will send dbus updates periodically, even when paused, which will automatically set it as the most recent player. The only downside is that explicitly pausing the player does not make it most recent, but the more intuitive notion of 'most recent' is the most recent player to be Playing.

This implements the feature suggested in altdesktop#170
@acrisci
Copy link
Member

acrisci commented Apr 29, 2020

Additionally, I have added some logic to first check the status of the 'new' player before setting it active.

What if we say the active player changes if we get an event where playback status changes? That solves the problem. We already cache all that.

This dbus command can be incorporated in playerctl, so that the user can do something like playerctl shift

I don't want to put anything playerctld specific in playerctl binary other than the bit to auto start the player. Put an argument parser in playerctld to handle this.

@dmsalomon
Copy link
Contributor Author

dmsalomon commented Apr 30, 2020

What if we say the active player changes if we get an event where playback status changes? That solves the problem. We already cache all that.

I tried to implement this, but it doesn't seem to work very well. It does work to control the active player, but these continuous pause messages still update the output of playerctl -p playerctld metadata --follow. Also this method doesn't work in the context of switching and pausing the old player/playing the new player. The responses from each player (which will be recorded by playercltd) are not synchronized, and its possible that the paused player will respond last, causing it to regain the active role.

The solution of only allowing a player in the Playing state to become active is more reliable and intuitive, since the user should always the currently playing player to be considered active. It would be annoying for playerctl -p playerctld play-pause to play the paused player, and not pause the playing player, at least in the most common use cases.

I don't want to put anything playerctld specific in playerctl binary other than the bit to auto start the player. Put an argument parser in playerctld to handle this.

It seems strange to place client code into the same binary that runs the daemon, but it definitely works.

I also implemented another method, that not only shifts/cycles between the various players, but also pauses the old player, and plays the new player (assuming the old player was actually playing, otherwise it is the same as the original method).

@acrisci
Copy link
Member

acrisci commented May 2, 2020

these continuous pause messages still update the output of playerctl -p playerctld metadata --follow

Sounds like it could be a bug that can be fixed there. I need to look into the exact behavior because I think there is a better solution. Intuitively I would like the active player to be the player the user has interacted with last. Maybe other things should switch the active player such as when a track changes. Let's pin down what we want and try to achieve that. What would your ideal rule be for active player selection?

It seems strange to place client code into the same binary that runs the daemon, but it definitely works.

By putting all the complexity this adds in playerctld, it allows us to accommodate more features without putting the burden of documentation on the typical user.

I also implemented another method, that not only shifts/cycles between the various players, but also pauses the old player, and plays the new player

I'm not a fan of this because it increases the scope of this feature dramatically. If I accept this, I might end up with an entire shift mpris interface which I don't want to maintain. But don't be discouraged if I don't accept a feature. The way this is designed it's very easy to fork into your own project if you have big plans.

@dmsalomon
Copy link
Contributor Author

dmsalomon commented May 4, 2020

Sounds like it could be a bug that can be fixed there. I need to look into the exact behavior because I think there is a better solution.

I think this might be a good place to start looking. (edit: that is also causing a bug, but an unrelated one 😀. This was happening because I missed a detail in the implementation).

Intuitively I would like the active player to be the player the user has interacted with last. Maybe other things should switch the active player such as when a track changes. Let's pin down what we want and try to achieve that. What would your ideal rule be for active player selection?

The issue is that playerctld is not actually monitoring what player was interacted with last. What it is monitoring is which player has emitted a signal for PropertiesChanged, which unfortunately some players (i.e. spotify) will send this at regular intervals, even when no properties have actually changed.

One approach is to never choose a paused player as active (unless there is no other choice). This makes sense to me, since I assume (I hope correctly) that the use case for playerctld is to both always pause the currently playing player, and then resume it later. So, active is really a synonym for currently playing or most recently playing. Even if a user were to manually pause a player, it was either recently playing (in which case it was active by my preferred definition), or wasn't playing, in which cause the pause was useless and should not change the state of playerctld. (I'm ignoring cases in which a user want 2 players playing simultaneously, firstly because that's rare, more importantly because playerctld can't sanely handle such an environment anyway).

The other solution is what you proposed: to manually verify that properties have changed. Perhaps not all of them, but at least the important ones like playback status, artist, and title. This will of course mean that some user interaction will be ignored, but I don't see why a user would pause a player that is already paused anyway.

I'm not a fan of this because it increases the scope of this feature dramatically. If I accept this, I might end up with an entire shift mpris interface which I don't want to maintain. But don't be discouraged if I don't accept a feature. The way this is designed it's very easy to fork into your own project if you have big plans.

I agree. I realized that this method is not actually needed, assuming that there was a way to query playerctld for the current active player. There is already a property for PlayerNames, so I think we can easily add one called ActivePlayer. Then a user could implement this functionality themselves using minimal shell scripting, with dbus-send.

@dmsalomon
Copy link
Contributor Author

dmsalomon commented May 4, 2020

Ok, I've trimmed the pull request a bit. It only has the basics to make it work. I am also using the idea you proposed to change the active player when the metadata changes. To my mind this feature works as expected.

There are 2 flaws in my opinion. It doesn't has the PlayPause feature (which is what I want out of this feature, personally), and it will switch to a player that is unable to play (i.e. CanPlay is false).

The second flaw can kind of be worked around with the following:

playerctld --shift &&
while [ "$(playerctl -p playerctld status)" = "Stopped" ]; do
    playerctld --shift
done

but its less than ideal.

However the first part is much trickier to implement properly outside of the daemon. And certainly not with minimal scripting as I originally thought, so far as I can see right now. Perhaps we can make it much easier to implement the play pause feature, by simply returning the old player and new player to the caller of the Shift method, so that a user could manually play/pause them.

GError *error = NULL;
struct Player *p;

p = context_get_active_player(ctx);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make me feel better if we had a guard here for no active player instead of relying on that guard in the method call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will put you at ease 😄

@@ -731,12 +789,13 @@ static void player_signal_proxy_callback(GDBusConnection *connection, const gcha
if (is_properties_changed) {
GVariant *interface = g_variant_get_child_value(parameters, 0);
GVariant *properties = g_variant_get_child_value(parameters, 1);
player_update_properties(player, g_variant_get_string(interface, 0), properties);
is_properties_changed =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a new variable for this. It's changed the meaning of the variable which can lead to subtle bugs.

}
}

int main(int argc, char *argv[]) {
struct PlayerctldContext ctx = {0};
GError *error = NULL;

if (argc > 2 || (argc == 2 && strcmp(argv[1], "--shift") != 0)) {
g_printerr("usage: playerctld [--shift]\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be playerctld shift as a command not a flag.

}
}

int main(int argc, char *argv[]) {
struct PlayerctldContext ctx = {0};
GError *error = NULL;

if (argc > 2 || (argc == 2 && strcmp(argv[1], "--shift") != 0)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use the glib argument parser for this. See the playerctl source code. Someone will have to do this at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried my best on the glib parser. It's a little weird since I'm using G_OPTION_REMAINING to catch the single parameter that we are interested in (i.e. shift), but I don't think there it is possible to indicate that it is optional (unless we make it a flag, --shift, which you don't want).

@@ -108,6 +110,11 @@ static void player_update_properties(struct Player *player, const char *interfac
}
GVariant *cache_value = g_variant_dict_lookup_value(&cached_properties, key, NULL);
if (cache_value != NULL) {
if (is_player_interface && g_strcmp0(key, "PlaybackStatus") == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've missed the case here where PlaybackStatus is not included in the changed properties given by the signal which is a possible case.

Copy link
Contributor Author

@dmsalomon dmsalomon May 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually important. This would break a whole bunch of tests in test_daemon.py that assume changing any metadata property will grab focus of the active player. (We could rewrite those tests entirely if we wanted to impose the new behavior). I am just assuming that we will only block those players (i.e. spotify) that send the PlaybackStatus property even when it hasn't changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break a whole bunch of tests in test_daemon.py that assume changing any metadata property will grab focus of the active player.

This means your changes are not covered by the test suite.

@acrisci
Copy link
Member

acrisci commented May 12, 2020

I'll need tests to merge this. See here:

https://github.com/altdesktop/playerctl/blob/master/test/test_daemon.py

@acrisci
Copy link
Member

acrisci commented May 12, 2020

The issue is that playerctld is not actually monitoring what player was interacted with last.

Yeah I agree with this.

One approach is to never choose a paused player as active

I've rejected this before. #160 has some reasons and was the initial inspiration for playerctld.

The main feature I need is for playerctl play-pause to always interact with the same player between calls unless the active player has changed.

the use case for playerctld is to both always pause the currently playing player, and then resume it later. So, active is really a synonym for currently playing or most recently playing.

I want all user interaction to count including seek and volume change. The tricky situation is metadata change since this can happen with or without user interaction and we can't tell the difference between the two. So if you hit the "next track" button, then that is definitely user interaction. But if the track just randomly changes maybe not.

If spotify is just spamming the same metadata over and over like you say, then that should definitely not count as an active player change. That can definitely be fixed.

@dmsalomon
Copy link
Contributor Author

Thanks for all the comments. I will get working on this. I'm dealing with finals at the moment, but I will get working on this as soon as I have a chance.

@dmsalomon
Copy link
Contributor Author

I believe I have addressed all of your code review comments. I made a comment above explaining that I'm not sure how to implement the glib parser to show that the command is optional.

The only thing left to address is exactly which properties we want to use to reject a player from becoming active. I think we can assert that a newly active player must change a critical property, such as playback status, title, volume, position (let me know what you think makes sense on this list). (Right now I am only checking for PlaybackStatus and xesam:title, and I ignore xesam:artist since it changes with xesam:title). We will need to modify the current tests which assume any metadata change makes a player active.

Finally, I will need to add tests for the Shift method itself.

@acrisci
Copy link
Member

acrisci commented May 24, 2020

I would like to merge the shift feature, but I think the active player stuff should be separated out into another PR since it still needs a lot of work.

@dmsalomon
Copy link
Contributor Author

I've separated off the active player modifications into a separate feature branch. The only thing that maybe hasn't been addressed is improving the --help output by making it clear that COMMAND is optional (although a user will never directly interact with playerctld unless they are using a command, so maybe not an issue).

g_warning("could not emit active player change: %s", error->message);
g_clear_error(&error);
}
player_update_position_sync(p, ctx, &error);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the position before emitting signals or it sends the old position.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 8ece9ef

@dmsalomon
Copy link
Contributor Author

It seems that my last commit just caused a test case to fail, and I can't see how any of the changes made should have caused that to happen. The tests run fine on my local machine, so I'm not sure what's up with that.

I'm also checking up on the status of this pull request, and what else you think needs to be done for it to be merged.

@acrisci
Copy link
Member

acrisci commented Jun 6, 2020

It passes locally so this is probably a race condition that was in the tests before your changes. Travis runs a bit slower so those can come out more easily.

@acrisci
Copy link
Member

acrisci commented Jun 6, 2020

Looks good to me. 👍

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

Successfully merging this pull request may close these issues.

2 participants