Merge pull request 'DLNA renderers, app updates told like Librarian, acquisition without Lidarr, Collection and admin rework (M502–M505, M542)' (#156) from dev into main

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

.gitea/workflows/release.yml

@@ -452,7 +452,7 @@ jobs:
           # The derivation lives in ci/version.sh, not here, so it can be
           # executed by a test on every push. Anything inline in this file is
           # unverifiable until a release is already running.
-          out="$(ci/version.sh HEAD)"
+          out="$(ci/version.sh --apk HEAD)"
           printf '%s\n' "${out}" >> "$GITHUB_OUTPUT"
 
           # The channel is a property of the LANE, not of the commit, which is

android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt

@@ -216,24 +216,56 @@ class MinstrelForwardingPlayer(
     // drops via isLoadingUpnp() = true; the OutputPickerController
     // observes the uiState.queue change and runs the resync (re-clears
     // Sonos's native queue + AddURIToQueue the new tracks + Play).
+    //
+    // The SAME tracks set again (play on the album already queued, from
+    // another track) is not a new queue: the renderer already holds it, and
+    // the uiState collector -- which keys on the track ids -- would never see
+    // a change to resync on, leaving the cast stuck "loading". It is a move
+    // within the queue, so the renderer is pointed at the new track and the
+    // play() that follows goes through.
 
     override fun setMediaItems(mediaItems: List<MediaItem>) {
+        val sameQueue = holdsSameItems(mediaItems)
         super.setMediaItems(mediaItems)
-        markPendingResyncIfRemote()
+        afterQueueSet(sameQueue)
     }
 
     override fun setMediaItems(mediaItems: List<MediaItem>, resetPosition: Boolean) {
+        val sameQueue = holdsSameItems(mediaItems)
         super.setMediaItems(mediaItems, resetPosition)
-        markPendingResyncIfRemote()
+        afterQueueSet(sameQueue)
     }
 
     override fun setMediaItems(mediaItems: List<MediaItem>, startIndex: Int, startPositionMs: Long) {
+        val sameQueue = holdsSameItems(mediaItems)
         super.setMediaItems(mediaItems, startIndex, startPositionMs)
-        markPendingResyncIfRemote()
+        afterQueueSet(sameQueue)
     }
 
-    private fun markPendingResyncIfRemote() {
-        val wasActive = holder.active.value ?: return
+    private fun holdsSameItems(mediaItems: List<MediaItem>): Boolean =
+        mediaItems.size == delegate.mediaItemCount &&
+            mediaItems.indices.all { mediaItems[it].mediaId == delegate.getMediaItemAt(it).mediaId }
+
+    private fun afterQueueSet(sameQueue: Boolean) {
+        val active = holder.active.value ?: return
+        if (!sameQueue) {
+            markPendingResync(active)
+            return
+        }
+        val index = delegate.currentMediaItemIndex
+        val positionMs = delegate.currentPosition.coerceAtLeast(0L)
+        Timber.w(
+            "setMediaItems with the queue %s already holds -- moving to track %d",
+            active.routeName,
+            index + 1,
+        )
+        when (active.queueMode) {
+            QueueMode.SINGLE_TRACK -> startSingleTrack(active, index, positionMs)
+            QueueMode.NATIVE_QUEUE -> seekNativeQueue(active, index, positionMs)
+        }
+    }
+
+    private fun markPendingResync(wasActive: ActiveUpnp) {
         Timber.w(
             "setMediaItems while UPnP active (%s) -- marking pending resync",
             wasActive.routeName,
@@ -332,11 +364,15 @@ class MinstrelForwardingPlayer(
         super.seekTo(mediaItemIndex, positionMs)
         when (active.queueMode) {
             QueueMode.SINGLE_TRACK -> playSingleTrackAt(active, mediaItemIndex, positionMs)
-            QueueMode.NATIVE_QUEUE -> sendNativeQueueCommand(active) {
-                active.avTransport.seekToTrack(mediaItemIndex + 1)
-                if (positionMs > 0L) {
-                    active.avTransport.seek(positionMs)
-                }
+            QueueMode.NATIVE_QUEUE -> seekNativeQueue(active, mediaItemIndex, positionMs)
+        }
+    }
+
+    private fun seekNativeQueue(active: ActiveUpnp, index: Int, positionMs: Long) {
+        sendNativeQueueCommand(active) {
+            active.avTransport.seekToTrack(index + 1)
+            if (positionMs > 0L) {
+                active.avTransport.seek(positionMs)
             }
         }
     }

android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt

@@ -383,23 +383,27 @@ class OutputPickerController @Inject constructor(
             resyncSingleTrack(upnpRoute, transport, newQueue, newCurrentIndex)
             return@withLock
         }
-        val handledIncrementally = runCatching {
+        // The incremental diff is for a queue changed under a cast that kept
+        // running (play next, an append, a removal): it leaves the playing
+        // track alone. A queue REPLACED through setMediaItems has already
+        // cleared active, and the listener asked for its start track and for
+        // it to play -- both of which the diff would ignore, and with active
+        // never restored the app would think it had gone back to the phone
+        // while the speaker played on. That case reloads in full below.
+        val stillActive = activeUpnpHolder.active.value?.routeId == routeId
+        val handledIncrementally = stillActive && runCatching {
             sonosQueue.tryIncrementalResync(transport, oldIds, newQueue)
         }.getOrElse { e ->
             Timber.w(e, "Sonos incremental resync errored; falling back to full reload")
             false
         }
         if (handledIncrementally) {
-            // Active was never cleared on the incremental path; clear any
-            // target that markPendingResyncIfRemote set (it didn't, for
-            // incremental cases that don't go through setMediaItems, but
-            // belt-and-suspenders).
             activeUpnpHolder.setTarget(null)
             return@withLock
         }
         // Full rebuild: ensure active is cleared so transport calls drop
-        // (markPendingResyncIfRemote may already have done this on the
-        // setMediaItems path).
+        // (the forwarding player has already done this on the setMediaItems
+        // path).
         if (activeUpnpHolder.active.value != null) {
             activeUpnpHolder.set(null)
             activeUpnpHolder.setTarget(routeId)

android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateRepository.kt

@@ -62,7 +62,7 @@ class UpdateRepository(
     private val installedCode: Long,
     private val installedName: String,
     private val offers: Boolean,
-    // Where the updater before this one kept its download, never cleaned up.
+    // A download left by older builds of the app, which nothing else removes.
     private val legacyApk: File? = null,
 ) {
     private val turns = Mutex()

ci/version.sh

@@ -3,11 +3,17 @@
 # Derives the three values a build is stamped with, and the tag that names it.
 #
 #   name=YYYY.MM.DD.HHMM   label for people, from the timestamp of the newest
-#                          commit that CHANGED SOMETHING SHIPPED (see SHIPPED)
+#                          commit that CHANGED SOMETHING SHIPPED (see SHIPPED,
+#                          or SHIPPED_APK with --apk)
 #   code=<int>             ordering key, minutes since 2020-01-01 at BUILD time
 #   tag=v<name>            what a release of this commit must be called
 #
-# Usage:  ci/version.sh [<commit-ish>]        (default HEAD)
+# Usage:  ci/version.sh [--apk] [<commit-ish>]        (default HEAD)
+#
+#   --apk  names the Android app from android/ alone. The app compares this
+#          name with the one it is running, and a different name is what
+#          offers an update — so a name that moved for a server-only commit
+#          would offer every phone a rebuild of the app it already has.
 #
 # This exists as a script rather than inline workflow YAML for one reason:
 # release.yml only runs on `main` and on tags, so anything living inside it is
@@ -30,6 +36,12 @@
 set -euo pipefail
 
 readonly EPOCH_2020=1577836800 # 2020-01-01T00:00:00Z
+
+apk=false
+if [ "${1:-}" = "--apk" ]; then
+  apk=true
+  shift
+fi
 readonly REF="${1:-HEAD}"
 
 # Both clocks are overridable so a test can pin them. Nothing but tests should
@@ -48,14 +60,17 @@ readonly REF="${1:-HEAD}"
 #   wrongly included → version moves when nothing shipped. Cosmetic noise in
 #                      a string nobody sorts.
 #
-# THIS REPO SHIPS TWO ARTIFACTS FROM ONE DERIVATION, and that is why the list
-# is shorter than it looks like it should be. The server image ships cmd/,
-# internal/, shared/, web/, config.example.yaml and client/; the APK ships
-# android/. Neither ships the other's sources — but excluding android/ here
-# would stop an Android-only commit from moving the APK's OWN version, which
-# is the dangerous direction. So this is the union: exclude only what ships in
-# NEITHER, and accept that an Android commit also nudges the server's reported
-# version. Over-inclusion across the two, which is the harmless direction.
+# THIS LIST NAMES THE SERVER IMAGE, and it is shorter than it looks like it
+# should be. The image ships cmd/, internal/, shared/, web/,
+# config.example.yaml and client/ — and client/ holds the APK, so android/
+# stays in: an Android-only commit changes what the image carries. Exclude
+# only what ships in neither artifact; over-inclusion is the harmless
+# direction here.
+#
+# The APK itself is named from SHIPPED_APK (below), where over-inclusion is
+# NOT harmless: the app offers an update whenever the name differs from its
+# own, so a name moved by a server-only commit offers every phone a rebuild
+# of the app it already has.
 #
 # The family's other repos (roundtable / roundtable-android) each keep a
 # tighter list because they are separate repos with one artifact apiece. Do
@@ -100,9 +115,24 @@ readonly SHIPPED=(
   ':!web/vitest.setup.ts'
 )
 
+# What the APK is built from: android/ and nothing else (the Gradle build
+# reads no path outside it). Still a denylist within it, for the same reason
+# as above — a new directory under android/ counts until named here.
+readonly SHIPPED_APK=(
+  android
+  ':!android/app/src/test'        # JVM unit tests
+  ':!android/*.md'
+)
+
+if [ "${apk}" = true ]; then
+  shipped=("${SHIPPED_APK[@]}")
+else
+  shipped=("${SHIPPED[@]}")
+fi
+
 commit_epoch="${MINSTREL_COMMIT_EPOCH:-}"
 if [ -z "${commit_epoch}" ]; then
-  commit_epoch="$(git log --format=%ct -1 "${REF}" -- "${SHIPPED[@]}")"
+  commit_epoch="$(git log --format=%ct -1 "${REF}" -- "${shipped[@]}")"
   # Loudly, on purpose. A silent fallback here is the landmine this whole
   # script exists to avoid: a plausible-looking version that is quietly wrong,
   # on a green run. Realistically this means a shallow clone (no commit in

internal/server/release_version_test.go

@@ -455,7 +455,7 @@ func TestImageBuild_StampsADerivedVersionAndAChannel(t *testing.T) {
 		t.Errorf("the main arm stamps the literal string \"main\" as a version; two images months apart would be indistinguishable\n%s", mainArm)
 	}
 	if !strings.Contains(yaml, "ci/version.sh HEAD | sed") {
-		t.Error("the image job no longer derives its version from ci/version.sh — the version and the APK's version can now drift apart")
+		t.Error("the image job no longer derives its version from ci/version.sh — the image's version has drifted out of test coverage")
 	}
 }
 
@@ -508,10 +508,11 @@ func commitFile(t *testing.T, dir, path, epoch string) {
 }
 
 // versionIn runs ci/version.sh inside dir, reading real git rather than the
-// pinned-clock override, so the PATHSPEC is what is under test.
-func versionIn(t *testing.T, dir string) (string, error) {
+// pinned-clock override, so the PATHSPEC is what is under test. flags go
+// before the ref, as in `--apk`.
+func versionIn(t *testing.T, dir string, flags ...string) (string, error) {
 	t.Helper()
-	cmd := exec.Command(filepath.Join(repoRoot(t), "ci", "version.sh"), "HEAD")
+	cmd := exec.Command(filepath.Join(repoRoot(t), "ci", "version.sh"), append(flags, "HEAD")...)
 	cmd.Dir = dir
 	cmd.Env = append(os.Environ(),
 		"GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null",
@@ -558,11 +559,9 @@ func TestVersionName_IgnoresCommitsThatShipNothing(t *testing.T) {
 	}
 }
 
-// android/ is deliberately NOT excluded, and that is the subtle half of the
-// list. It ships in no server image — but it is the APK's entire source, and
-// ONE script derives the version for both artifacts. Excluding it would stop
-// an Android-only commit from moving the APK's own version, which is exactly
-// the silent downgrade the versioning rework exists to prevent.
+// android/ is deliberately NOT excluded from the image's name: the image
+// bundles the APK under client/, so an Android-only commit changes what the
+// image carries, and an image that changed must not keep its old name.
 func TestVersionName_AndroidSourcesCount(t *testing.T) {
 	dir := gitRepo(t)
 	commitFile(t, dir, "internal/server/thing.go", "1757443736")
@@ -573,7 +572,71 @@ func TestVersionName_AndroidSourcesCount(t *testing.T) {
 		t.Fatalf("version.sh failed: %v\n%s", err, out)
 	}
 	if !strings.Contains(out, "name=2026.09.10.0042") {
-		t.Errorf("an Android commit did not move the version; the APK would ship new code under its old version name\n%s", out)
+		t.Errorf("an Android commit did not move the image's version; it carries a new APK under its old name\n%s", out)
+	}
+}
+
+// The app offers an update whenever the server's APK has a different name
+// from its own, so the APK's name must move for Android commits and ONLY for
+// them. A name taken from the whole repo would offer every phone a rebuild of
+// the app it already has on each server-only deploy.
+func TestApkName_MovesOnlyForAndroidCommits(t *testing.T) {
+	const (
+		app    = "1757443736" // 2025.09.09.1848
+		server = "1789000920" // 2026.09.10.0042, later
+		again  = "1789600000" // 2026.09.16.2306, later still
+	)
+	dir := gitRepo(t)
+	commitFile(t, dir, "android/app/src/main/Thing.kt", app)
+	commitFile(t, dir, "internal/server/thing.go", server)
+	commitFile(t, dir, "web/src/routes/page.svelte", server)
+
+	out, err := versionIn(t, dir, "--apk")
+	if err != nil {
+		t.Fatalf("version.sh --apk failed: %v\n%s", err, out)
+	}
+	if !strings.Contains(out, "name=2025.09.09.1848") {
+		t.Errorf("a server-only commit moved the APK's name; every phone would be offered the app it already has\n%s", out)
+	}
+	// The image's name still follows the server commit, so the test above
+	// is not passing because the flag is ignored and everything is stale.
+	out, err = versionIn(t, dir)
+	if err != nil {
+		t.Fatalf("version.sh failed: %v\n%s", err, out)
+	}
+	if !strings.Contains(out, "name=2026.09.10.0042") {
+		t.Errorf("without --apk the name should follow the server commit\n%s", out)
+	}
+
+	// A test-only Android commit ships nothing either.
+	commitFile(t, dir, "android/app/src/test/java/A.kt", again)
+	out, err = versionIn(t, dir, "--apk")
+	if err != nil {
+		t.Fatalf("version.sh --apk failed: %v\n%s", err, out)
+	}
+	if !strings.Contains(out, "name=2025.09.09.1848") {
+		t.Errorf("an Android test-only commit moved the APK's name\n%s", out)
+	}
+
+	// ...and a real Android commit must move it, or new app code would ship
+	// under the name of the old and never be offered.
+	commitFile(t, dir, "android/app/src/main/Thing.kt", again)
+	out, err = versionIn(t, dir, "--apk")
+	if err != nil {
+		t.Fatalf("version.sh --apk failed: %v\n%s", err, out)
+	}
+	if !strings.Contains(out, "name=2026.09.16.2306") {
+		t.Errorf("an Android commit did not move the APK's name; the new app would never be offered\n%s", out)
+	}
+}
+
+// The APK lane must ask for the APK's name. Without the flag it would still
+// build, sign and publish, and quietly go back to offering an update on every
+// server deploy.
+func TestReleaseWorkflow_NamesTheApkFromAndroidAlone(t *testing.T) {
+	step := stepBody(t, releaseYAML(t), "Compute release version")
+	if !strings.Contains(step, "ci/version.sh --apk HEAD") {
+		t.Errorf("the APK lane does not derive its version with --apk\n%s", step)
 	}
 }