Support offset values for adding tracks relative to current track

[djmattyg007]

With this change, you can add tracks directly before or after the currently playing track without first having to retrieve the position of the currently playing track value yourself. When used over Mopidy's HTTP interfaces, this reduces the chance that the tracks you're adding will accidentally be skipped over between your two requests.

The syntax for the offset strings matches MPD's support that was implemented as part of 0.23.

I noticed there were no existing tests for the at_position argument, so I added some at the same time to handle other existing cases in addition to the new cases.

[djmattyg007]

I was in a hurry and didn't actually run the tests locally, figuring it would be fine. It appears I've got some more work to do on understanding how the tracklist works 😊

[adamcik]

Two points:

1. If we go with fixing this via a magic value it should be AFTER_CURRENT = ... or something like that. Not a bare magic value. Ideally this would also be a constant in mopidy.js

2. Second option would be adding a new keyword argument for this case. Downside here would be a new client with an old mopidy might break.

Are there other cases we should have "shortcuts" for? Start of tracklist, end and after current are the obvious ones that come to mind.

[djmattyg007]

If we go with fixing this via a magic value it should be AFTER_CURRENT = ... or something like that

I added a constant for this.

Ideally this would also be a constant in mopidy.js

I'll add that once this PR gets merged.

Are there other cases we should have "shortcuts" for? Start of tracklist, end and after current are the obvious ones that come to mind.

Start of tracklist is easy: just supply 0. End of tracklist is even easier and also already supported - don't supply any value at all.

[kingosticks]

This is out of scope for this PR and a bit niche but what happens (and what should happen) when you use this, or the old 2-call manual method, at a point in time near the end of the current track's playback? Will our gapless stuff have already loaded the old next track so you won't get the new one you just inserted?

[djmattyg007]

what happens (and what should happen) when you use this, or the old 2-call manual method, at a point in time near the end of the current track's playback?

Right now, there's a small window of time (even smaller with this new approach) where if the current track's playback ends after the index has been retrieved but before the first new track is added, you won't hear the tracks you've just added. As long as the first new track gets added before playback of the current track ends, you'll hear all of the tracks you added.

As for what should happen, I have no idea. I'm not sure there is a good solution.

I think a more pressing problem is "play next" functionality when random is turned on. Right now, the track you've just selected to play next won't actually play next (regardless of whether or not this new approach is used). Has any thought been given to this in the past?

[kingosticks]

If random is on then you cannot control what is played next without an excplicit call to play passing a track or tracklist ID. I don't think we need any exceptions to that rule. This PR is about adding to the tracklist next, not playing next.

EDIT:

Will our gapless stuff have already loaded the old next track so you won't get the new one you just inserted?

And for the record I couldn't trigger this happening with a few casual attempts. The time period between about-to-finish firing and the track finishing playing must be smaller than I remember.

[djmattyg007]

I suspect it could happen, but that you'd never be able to reliably reproduce it.

[djmattyg007]

I've updated this PR to utilise an enum for the new behaviour. This also allowed me to more easily add "constants" for the start and end of the tracklist, as @adamcik suggested.

The new system shouldn't have any issues with backwards-compatibility, and I've added a test to confirm that negative integer position values work.

[jodal]

This could be a string Enum:

Suggested change

	class AddAtPosition(enum.Enum):
	START = enum.auto()
	AFTER_CURRENT = enum.auto()
	END = enum.auto()
	class AddAtPosition(str, enum.Enum):
	START = "START"
	AFTER_CURRENT = "AFTER_CURRENT"
	END = "END"

Then you could accept strings to the at_position argument and check if they're valid with simply at_position in AddAtPosition, and the strings could be converted to enum values with just AddAtPosition(at_position).

[djmattyg007]

I deliberately didn't want to use a string Enum because I feel like you lose the benefits of using an Enum from a type checking point of view. I've seen them used before and they've always felt like a kludge.

[djmattyg007]

I've updated the PR. I also added a few more tests while I was there.

[kingosticks]

MPD now also supports this thanks to @djmattyg007's feature request. Perhaps we should also implement complete support for relative insertion indexes here.

[kingosticks]

@djmattyg007 are you happy with this as is or do you want to support the relative insertion with +/-OFFSET ?

[djmattyg007]

I'd prefer to re-work this to instead use the MPD offset syntax.

[djmattyg007]

Similarly to #2007 I'm going to work on updating this so we can utilise support for it in a mopidy-mpd update, which probably requires it to fulfil the new protocol version.

[djmattyg007]

@jodal @kingosticks This is finally ready for review once more :)

Flake8 is complaining about line length because of a couple of long strings, but it's not something black is complaining about. What are your thoughts? What's the preferred approach for breaking long strings over multiple lines?

[djmattyg007]

Oh, I noticed that offset values are now supported in MPD for a couple of other operations. I'll take a deeper look at this tomorrow 👍

[kingosticks]

Yes, it's totally random which methods support it. It appears to be done on request.

[djmattyg007]

I think they’re all related to adding tracks to the tracklist though, so hopefully this is the only new functionality required in Mopidy’s core to make it all possible.

[jodal]

Rebased and passed CI on main! 🎉