From 7d165d3b9b5ff7d8fb3209a0820b8eea62fc31f9 Mon Sep 17 00:00:00 2001
From: Michael Pyne <mpyne@kde.org>
Date: Sun, 1 Jul 2018 22:18:05 -0400
Subject: [PATCH] Remove slight bias in random track selection.

Adjust the way we randomly pick the next track to play to remove a
potential bias towards tracks that are earlier in the list.

On GNU the bias is extremely slight since RAND_MAX is so high, but it's
not that much harder to do it right.
---
 tracksequenceiterator.cpp | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/tracksequenceiterator.cpp b/tracksequenceiterator.cpp
index 0fa95ee3..42f6b6ec 100644
--- a/tracksequenceiterator.cpp
+++ b/tracksequenceiterator.cpp
@@ -69,6 +69,24 @@ DefaultSequenceIterator::~DefaultSequenceIterator()
 {
 }
 
+// Helper function to return a random number up to (but not including) a given max with
+// a truly equal probability of each integer in [0, max) being selected.
+// When Qt 5.10 can be required we can use QRandomGenerator for this, but for now need to
+// fixup KRandom.
+// See https://twitter.com/colmmacc/status/1012723779708088320
+static int boundedRandom(const int upperBound)
+{
+    while (1) {
+        const int candidate = KRandom::random();
+
+        // this check excludes integers above the highest multiple of
+        // upperBound that is still below RAND_MAX to remove bias
+        if (candidate < (RAND_MAX - (RAND_MAX % upperBound))) {
+            return candidate % upperBound;
+        }
+    }
+}
+
 void DefaultSequenceIterator::advance()
 {
     if(!current())
@@ -79,6 +97,9 @@ void DefaultSequenceIterator::advance()
     bool albumRandom = action("albumRandomPlay") && action<KToggleAction>("albumRandomPlay")->isChecked();
 
     if(isRandom || albumRandom) {
+        // TODO: This should probably use KRandomSequence's ability to shuffle
+        // items instead of making a new random choice each time through.
+
         if(m_randomItems.isEmpty() && loop) {
 
             // Since refillRandomList will remove the currently playing item,
@@ -100,7 +121,7 @@ void DefaultSequenceIterator::advance()
 
         if(albumRandom) {
             if(m_albumSearch.isNull() || m_albumSearch.matchedItems().isEmpty()) {
-                item = m_randomItems[KRandom::random() % m_randomItems.count()];
+                item = m_randomItems[boundedRandom(m_randomItems.count())];
                 initAlbumSearch(item);
             }
 
@@ -135,7 +156,7 @@ void DefaultSequenceIterator::advance()
                 qCCritical(JUK_LOG) << "Unable to perform album random play on " << *item;
         }
         else
-            item = m_randomItems[KRandom::random() % m_randomItems.count()];
+            item = m_randomItems[boundedRandom(m_randomItems.count())];
 
         setCurrent(item);
         m_randomItems.removeAll(item);
-- 
GitLab