From 91254e9211f6fa9864dd8f2d2bb7b43cccf7cb77 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 17 Oct 2017 19:44:16 +0200 Subject: [PATCH] queue/PlaylistControl: keep order list consistency in MoveOrderToCurrent() Our previous use of Queue::SwapOrders() could cause surprising results: - sometimes, the old "current" song would be played again (if the newly selected song had not been played already) - sometimes, the old "current" song would not be played again (if the newly selected song had already been played) This is inconsistent, because it should not depend on whether the newly selected song had already been played. So instead of Queue::SwapOrders() we now use Queue::MoveOrderAfter() and Queue::MoveOrderBefore(), which is more expensive, but also more consistent. It attempts to retain as much from the previous order list as possible, and only moves the newly selected song around. --- NEWS | 1 + src/queue/PlaylistControl.cxx | 24 +++++++++++++++--------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/NEWS b/NEWS index 3ab0b453b..25bf749f5 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,7 @@ ver 0.20.11 (not yet released) * decoder - ffmpeg: more reliable song duration - gme: fix track numbering +* improve random song order when switching songs manually * fix case insensitive search without libicu * fix endless loop when accessing malformed file names in ZIP files diff --git a/src/queue/PlaylistControl.cxx b/src/queue/PlaylistControl.cxx index c2284b60c..2217756dd 100644 --- a/src/queue/PlaylistControl.cxx +++ b/src/queue/PlaylistControl.cxx @@ -63,15 +63,21 @@ playlist::MoveOrderToCurrent(unsigned old_order) /* no-op because there is no order list */ return old_order; - const unsigned destination_order = playing - ? (unsigned)current - : 0; - - /* swap the new song with the previous "current" one, so - playback continues as planned */ - queue.SwapOrders(old_order, destination_order); - - return destination_order; + if (playing) { + /* already playing: move the specified song after the + current one (because the current one has already + been playing and shall not be played again) */ + return queue.MoveOrderAfter(old_order, current); + } else if (current >= 0) { + /* not playing: move the specified song before the + current one, so it will be played eventually */ + return queue.MoveOrderBefore(old_order, current); + } else { + /* not playing anything: move the specified song to + the front */ + queue.SwapOrders(old_order, 0); + return 0; + } } void