Missing playerstatus notifications for sync command when players are off

[timbell]

I'm writing a client app and trying to drive all updates from the status/playerstatus notifications. Things are generally working well but not when syncs are performed with players turned off. A simple case is as follows:

Using CometD to subscribe to playerstatus notifications, and with two players A and B. If the players are on then playerstatus notifications are received for both players when syncing/unsyncing. If the players are off then only one of the two players gets a playerstatus notification.

This is due to this code in statusQuery_filter (Queries.pm):

	# Bug 10064: playlist notifications get sent to everyone in the sync-group
	if ($request->isCommand([['playlist', 'newmetadata']]) && (my $client = $request->client)) {
		return 0 if !grep($_->id eq $myclientid, $client->syncGroupActiveMembers());
	} else {
		return 0 if $clientid ne $myclientid;
	}

It could be fixed by changing to syncGroupActiveMembers to syncGroupAllMembers (a new method referencing allPlayers) but that would probably lead to too many notifications in other cases. A better alternative might be to explicitly test for sync commands and never filter them so that all players are always notified when a sync command is issued. Something like this:

	# Bug 10064: playlist notifications get sent to everyone in the sync-group
	if ($request->isCommand([['playlist', 'newmetadata']]) && (my $client = $request->client)) {
		return 0 if !grep($_->id eq $myclientid, $client->syncGroupActiveMembers());
	elsif (!$request->isCommand([['sync']])) {
		return 0 if $clientid ne $myclientid;
	}

It might be possible to limit the notifications to only players directly affected by the sync command but it would require knowing the sync groups' memberships before the command was issued so probably pretty hard to do and maybe not worth it.

[michaelherger]

TBH: I'm not too familiar with this part of the code. But shouldn't the check be part of the first condition?

Would you mind testing this thoroughly, then submit a pull request for 8.2?

[timbell]

Sure, I'll do a pull request. I've been running the following change in 8.2 and 7.9.1 (my "prod") without any issues. I think it expresses the intent more clearly.

	# Bug 10064: playlist notifications get sent to everyone in the sync-group
	if ($request->isCommand([['playlist', 'newmetadata']]) && (my $client = $request->client)) {
		return 0 if !grep($_->id eq $myclientid, $client->syncGroupActiveMembers());
	} elsif ($request->isCommand([['sync']])) {
		# Don't filter out notifications for sync commands. All players are notified
		# of the change in sync state regardless of whether they are "on" or "off".
	} else {
		return 0 if $clientid ne $myclientid;
	}