command/QueueCommands: reimplement relative "move"/"moveid" offsets

The existing implementation has been utterly broken forever; I cannot
explain what it actually does, but it doesn't do what the
documentation says.

NEWS

@@ -4,6 +4,7 @@ ver 0.23 (not yet released)
   - show the audio format in "playlistinfo"
   - support "listfiles" with arbitrary storage plugins
   - support relative positions in "addid"
+  - fix relative positions in "move" and "moveid"
 * database
   - proxy: require MPD 0.20 or later
   - proxy: require libmpdclient 2.11 or later

doc/protocol.rst

@@ -739,14 +739,22 @@ Whenever possible, ids should be used.
     at ``START:END`` [#since_0_15]_ to ``TO``
     in the playlist.
 
+    If ``TO`` starts with ``+`` or ``-``, then it is relative to the
+    current song; e.g. ``+0`` moves to right after the current song
+    and ``-0`` moves to right before the current song (i.e. zero songs
+    between the current song and the moved range).
+
 .. _command_moveid:
 
 :command:`moveid {FROM} {TO}`
     Moves the song with ``FROM`` (songid) to
     ``TO`` (playlist index) in the
-    playlist.  If ``TO`` is negative, it
+    playlist.
-    is relative to the current song in the playlist (if
+
-    there is one).
+    If ``TO`` starts with ``+`` or ``-``, then it is relative to the
+    current song; e.g. ``+0`` moves to right after the current song
+    and ``-0`` moves to right before the current song (i.e. zero songs
+    between the current song and the moved song).
 
 .. _command_playlist:
 

src/Partition.hxx

@@ -147,14 +147,10 @@ struct Partition final : QueueListener, PlayerListener, MixerListener {
 		playlist.Shuffle(pc, range);
 	}
 
-	void MoveRange(RangeArg range, int to) {
+	void MoveRange(RangeArg range, unsigned to) {
 		playlist.MoveRange(pc, range, to);
 	}
 
-	void MoveId(unsigned id, int to) {
-		playlist.MoveId(pc, id, to);
-	}
-
 	void SwapPositions(unsigned song1, unsigned song2) {
 		playlist.SwapPositions(pc, song1, song2);
 	}

src/command/QueueCommands.cxx

@@ -363,8 +363,62 @@ handle_prioid(Client &client, Request args, [[maybe_unused]] Response &r)
 	return CommandResult::OK;
 }
 
+static unsigned
+ParseMoveDestination(const char *s, const RangeArg range,
+		     const playlist &p)
+{
+	assert(!range.IsEmpty());
+	assert(!range.IsOpenEnded());
+
+	const unsigned queue_length = p.queue.GetLength();
+
+	if (*s == '+') {
+		/* after the current song */
+
+		unsigned current = RequireCurrentPosition(p);
+		assert(current < queue_length);
+		if (range.Contains(current))
+			/* no-op */
+			return range.start;
+
+		if (current >= range.end)
+			current -= range.Count();
+
+		return current + 1 +
+			ParseCommandArgUnsigned(s + 1,
+						queue_length - current - range.Count());
+	} else if (*s == '-') {
+		/* before the current song */
+
+		unsigned current = RequireCurrentPosition(p);
+		assert(current < queue_length);
+		if (range.Contains(current))
+			/* no-op */
+			return range.start;
+
+		if (current >= range.end)
+			current -= range.Count();
+
+		return current -
+			ParseCommandArgUnsigned(s + 1,
+						queue_length - current - range.Count());
+	} else
+		/* absolute position */
+		return ParseCommandArgUnsigned(s,
+					       queue_length - range.Count());
+}
+
+static CommandResult
+handle_move(Partition &partition, RangeArg range, const char *to)
+{
+	partition.MoveRange(range,
+			    ParseMoveDestination(to, range,
+						 partition.playlist));
+	return CommandResult::OK;
+}
+
 CommandResult
-handle_move(Client &client, Request args, [[maybe_unused]] Response &r)
+handle_move(Client &client, Request args, Response &r)
 {
 	RangeArg range = args.ParseRange(0);
 	if (range.IsOpenEnded()) {
@@ -372,18 +426,22 @@ handle_move(Client &client, Request args, [[maybe_unused]] Response &r)
 		return CommandResult::ERROR;
 	}
 
-	int to = args.ParseInt(1);
+	return handle_move(client.GetPartition(), range, args[1]);
-	client.GetPartition().MoveRange(range, to);
-	return CommandResult::OK;
 }
 
 CommandResult
 handle_moveid(Client &client, Request args, [[maybe_unused]] Response &r)
 {
-	unsigned id = args.ParseUnsigned(0);
+	auto &partition = client.GetPartition();
-	int to = args.ParseInt(1);
+	const auto &queue = partition.playlist.queue;
-	client.GetPartition().MoveId(id, to);
+
-	return CommandResult::OK;
+	const int position = queue.IdToPosition(args.ParseUnsigned(0));
+	if (position < 0) {
+		r.Error(ACK_ERROR_NO_EXIST, "No such song");
+		return CommandResult::ERROR;
+	}
+
+	return handle_move(partition, RangeArg::Single(position), args[1]);
 }
 
 CommandResult

src/queue/Playlist.hxx

@@ -258,9 +258,7 @@ public:
 
 	void Shuffle(PlayerControl &pc, RangeArg range);
 
-	void MoveRange(PlayerControl &pc, RangeArg range, int to);
+	void MoveRange(PlayerControl &pc, RangeArg range, unsigned to);
-
-	void MoveId(PlayerControl &pc, unsigned id, int to);
 
 	void SwapPositions(PlayerControl &pc, unsigned song1, unsigned song2);
 

src/queue/PlaylistEdit.cxx

@@ -315,51 +315,31 @@ playlist::StaleSong(PlayerControl &pc, const char *uri) noexcept
 }
 
 void
-playlist::MoveRange(PlayerControl &pc, RangeArg range, int to)
+playlist::MoveRange(PlayerControl &pc, RangeArg range, unsigned to)
 {
 	if (!queue.IsValidPosition(range.start) ||
 	    !queue.IsValidPosition(range.end - 1))
 		throw PlaylistError::BadRange();
 
-	if ((to >= 0 && to + range.Count() - 1 >= GetLength()) ||
+	if (to + range.Count() - 1 >= GetLength())
-	    (to < 0 && unsigned(std::abs(to)) > GetLength()))
 		throw PlaylistError::BadRange();
 
-	if ((int)range.start == to)
+	if (range.start == to)
 		/* nothing happens */
 		return;
 
 	const DetachedSong *const queued_song = GetQueuedSong();
 
-	/*
-	 * (to < 0) => move to offset from current song
-	 * (-playlist.length == to) => move to position BEFORE current song
-	 */
-	const int currentSong = GetCurrentPosition();
-	if (to < 0) {
-		if (currentSong < 0)
-			/* can't move relative to current song,
-			   because there is no current song */
-			throw PlaylistError::BadRange();
-
-		if (range.Contains(currentSong))
-			/* no-op, can't be moved to offset of itself */
-			return;
-		to = (currentSong + std::abs(to)) % GetLength();
-		if (range.start < (unsigned)to)
-			to -= range.Count();
-	}
-
 	queue.MoveRange(range.start, range.end, to);
 
 	if (!queue.random && current >= 0) {
 		/* update current */
 		if (range.Contains(current))
-			current += unsigned(to) - range.start;
+			current += to - range.start;
 		else if (unsigned(current) >= range.end &&
-			 unsigned(current) <= unsigned(to))
+			 unsigned(current) <= to)
 			current -= range.Count();
-		else if (unsigned(current) >= unsigned(to) &&
+		else if (unsigned(current) >= to &&
 			 unsigned(current) < range.start)
 			current += range.Count();
 	}
@@ -368,16 +348,6 @@ playlist::MoveRange(PlayerControl &pc, RangeArg range, int to)
 	OnModified();
 }
 
-void
-playlist::MoveId(PlayerControl &pc, unsigned id1, int to)
-{
-	int song = queue.IdToPosition(id1);
-	if (song < 0)
-		throw PlaylistError::NoSuchSong();
-
-	MoveRange(pc, RangeArg::Single(song), to);
-}
-
 void
 playlist::Shuffle(PlayerControl &pc, RangeArg range)
 {