clone song bug + add one method

[philippe44]

Two changes

1- small one is adding a method as a provision for 8.5 when a track has started, we call the PH in case it wants to do something for each player. Not important but might be useful one day and as 8.5 will probably be widely used, better do that now.

2- The other one, which lead me to try 1- as a workaround drove me nuts... In the Deezer plugin, I could not get metadata changes to happen at the time (was way in advance, as in current plugin). After a lot of swearing and sweating, I realized that the $song->pluginData of the next track was in fact pointing to current track's pluginData. So when changing some of it in the next, it was of course being modified in the current one. It happens because when cloning song, it's a simple copy of attributes, including hashrefs.
There is a user-level workaround to initialize the new $song's pluginData object with an empty hash before setting anything in it. Still, I don't think that was the intent when cloning a song so I think we should do a proper clone of all its attribute, unless you think it was purposely done a shallow copy.

[mherger]

Please don't get me wrong... but when there's a change needed in a method which has "worked as expected" for 15 years, I'm always a bit hesitant. You know this place better than I do. Yet... why did we never need that clone tweak before? Can you point me at your code which would require this change?

Clone does not seem to be a core module. I'm not sure we can expect it to be available always. I see that they claim it was faster than Storable::dclone, but I'd still prefer the latter, as it's what we're using all over the place, and which has proven to be working on all supported platforms.

[philippe44]

I don't 😄, it's a very legitimate question

The issue is probably not obvious and people might have made some workaround with time. For example, in existing Deezer, ProtocolHandler::getNextTrack, line 330 there is a tentative of reset of the pluginData fields which I think does not work BTW because if you look at the code of Song::PluginData

sub pluginData {
	my ( $self, $key, $value ) = @_;

	my $ret;

	if ( !defined $self->_pluginData() ) {
		$self->_pluginData({});
	}

	if ( !defined $key ) {
		return $self->_pluginData();
	}

	if ( ref $key eq 'HASH' ) {
		# Assign an entire hash to pluginData
		$ret = $self->_pluginData($key);
	}
	else {
		if ( defined $value ) {
			$self->_pluginData()->{$key} = $value;
		}

		$ret = $self->_pluginData()->{$key};
	}

	return $ret;
}

Setting a key to 'undef', AFAIU, does not do anything. In fact I had this issue a long time ago in the Groups plugin, but that's another "story".

The issue is in Player::StreamingController::_getNextTrack. When the current track is a playlist, we make a clone of it and then call $song->getNextSong on that clone and getNextSong ultimately calls handler->getNextTrack with the clone (which will then be put in the controller's queue list that contains streaming and playing songs)

But so if you modify the pluginData in PH's getNextTrack, you in fact modify the pluginData of the $client->playingSong which I can't believe is the expected effect. You want to modify your own $song pluginData. For example, it does not end well if getMetadataFor uses $client->playingSong's pluginData for anything (that was my issue).

The workaround is simple, just do a $song->pluginData({}) in getNextTrack but again, I can't see that as the desired effect and I was thinking that 8.5 could be the opportunity for such cleaning. Regarding dclone vs clone, I have to preference.

If you want to leave things as they are, it's up to you of course. I think others will be bit by that thing which a bit vicious and unexpected. I was really not thinking that the song I receive in getNextTrack is "mangled" with the currently playing one.