From f00cd0abcdb42bc9b777f9860c75c68528420475 Mon Sep 17 00:00:00 2001 From: Boof2015 <75185879+Boof2015@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:08:15 -0400 Subject: [PATCH] fix now playing swipe softlock --- .../data/CatalogMigrationTest.kt | 2 +- .../astralibraryscanner/data/ArtistCredits.kt | 14 +- .../data/ArtistCreditsTest.kt | 5 +- src/components/player/NowPlayingOverlay.tsx | 258 +++++++++++++----- .../player/nowPlayingDismiss.test.mts | 26 ++ src/components/player/nowPlayingDismiss.ts | 39 +++ .../player/nowPlayingPreferences.test.mts | 30 +- src/shared/library/artistCredits.ts | 57 ++-- 8 files changed, 328 insertions(+), 103 deletions(-) diff --git a/modules/astra-library-scanner/android/src/androidTest/java/expo/modules/astralibraryscanner/data/CatalogMigrationTest.kt b/modules/astra-library-scanner/android/src/androidTest/java/expo/modules/astralibraryscanner/data/CatalogMigrationTest.kt index 5951cba..efb5bf0 100644 --- a/modules/astra-library-scanner/android/src/androidTest/java/expo/modules/astralibraryscanner/data/CatalogMigrationTest.kt +++ b/modules/astra-library-scanner/android/src/androidTest/java/expo/modules/astralibraryscanner/data/CatalogMigrationTest.kt @@ -44,7 +44,7 @@ class CatalogMigrationTest { ).use { cursor -> assertTrue(cursor.moveToFirst()) assertEquals("jellyfin:2", cursor.getString(0)) - assertEquals(CURRENT_ARTIST_CREDIT_VERSION, cursor.getInt(1)) + assertEquals(INITIAL_MULTI_ARTIST_CREDIT_VERSION, cursor.getInt(1)) assertTrue(cursor.moveToNext()) assertEquals("local:1", cursor.getString(0)) assertEquals(LEGACY_ARTIST_CREDIT_VERSION, cursor.getInt(1)) diff --git a/modules/astra-library-scanner/android/src/main/java/expo/modules/astralibraryscanner/data/ArtistCredits.kt b/modules/astra-library-scanner/android/src/main/java/expo/modules/astralibraryscanner/data/ArtistCredits.kt index 3b4572e..48242e5 100644 --- a/modules/astra-library-scanner/android/src/main/java/expo/modules/astralibraryscanner/data/ArtistCredits.kt +++ b/modules/astra-library-scanner/android/src/main/java/expo/modules/astralibraryscanner/data/ArtistCredits.kt @@ -10,8 +10,12 @@ import com.google.android.exoplayer2.metadata.id3.TextInformationFrame import java.util.concurrent.TimeUnit import org.json.JSONArray -internal const val CURRENT_ARTIST_CREDIT_VERSION = 2 internal const val LEGACY_ARTIST_CREDIT_VERSION = 1 +internal const val INITIAL_MULTI_ARTIST_CREDIT_VERSION = 2 +// Version 3 removes synthesized ampersands from stored display strings. +// Keeping this independent of the Room schema version lets existing local +// version-2 catalogs use the normal stale-source full-reindex path. +internal const val CURRENT_ARTIST_CREDIT_VERSION = 3 internal data class ArtistCreditNames( val artists: List = emptyList(), @@ -33,13 +37,7 @@ internal fun normalizeArtistNames(values: Iterable): List { } internal fun formatArtistNames(values: Iterable): String { - val names = normalizeArtistNames(values) - return when (names.size) { - 0 -> "" - 1 -> names[0] - 2 -> "${names[0]} & ${names[1]}" - else -> "${names.dropLast(1).joinToString(", ")} & ${names.last()}" - } + return normalizeArtistNames(values).joinToString(", ") } internal fun serializeArtistNames(values: Iterable): String? { diff --git a/modules/astra-library-scanner/android/src/test/java/expo/modules/astralibraryscanner/data/ArtistCreditsTest.kt b/modules/astra-library-scanner/android/src/test/java/expo/modules/astralibraryscanner/data/ArtistCreditsTest.kt index 970c4ad..064f42d 100644 --- a/modules/astra-library-scanner/android/src/test/java/expo/modules/astralibraryscanner/data/ArtistCreditsTest.kt +++ b/modules/astra-library-scanner/android/src/test/java/expo/modules/astralibraryscanner/data/ArtistCreditsTest.kt @@ -16,8 +16,9 @@ class ArtistCreditsTest { assertEquals(listOf("Earth, Wind & Fire", "The Emotions"), credits.artists) assertEquals(listOf("Curator One", "Curator Two"), credits.albumArtists) - assertEquals("Earth, Wind & Fire & The Emotions", formatArtistNames(credits.artists)) - assertEquals("Curator One & Curator Two", formatArtistNames(credits.albumArtists)) + assertEquals("Earth, Wind & Fire, The Emotions", formatArtistNames(credits.artists)) + assertEquals("Curator One, Curator Two", formatArtistNames(credits.albumArtists)) + assertEquals("1, 2, 3", formatArtistNames(listOf("1", "2", "3"))) } @Test diff --git a/src/components/player/NowPlayingOverlay.tsx b/src/components/player/NowPlayingOverlay.tsx index b8c6ecb..22d29a3 100644 --- a/src/components/player/NowPlayingOverlay.tsx +++ b/src/components/player/NowPlayingOverlay.tsx @@ -1,3 +1,4 @@ +/* eslint-disable react-hooks/immutability, react-hooks/preserve-manual-memoization -- Reanimated gesture state is intentionally mutable, and the pan recognizer must retain identity across renders. */ import { Fragment, useCallback, useEffect, useMemo, useState } from 'react'; import { BackHandler, @@ -42,8 +43,10 @@ import { NowPlayingCompanionPane } from '@/components/player/NowPlayingCompanion import { PlayerStateIcon } from '@/components/player/PlayerStateIcon'; import { CachedLyricPeek } from '@/components/player/CachedLyricPeek'; import { + resolveNowPlayingPanRelease, resolveNowPlayingDismissSpring, shouldEnableNowPlayingPan, + shouldStartNowPlayingPan, } from '@/components/player/nowPlayingDismiss'; import { useDelayedUnmountPresence } from '@/components/delayedPresence'; import { @@ -73,8 +76,11 @@ import { NOW_PLAYING_WIDE_PANE_GAP, } from '@/components/player/nowPlayingLayout'; import { useReturnToTabs } from '@/navigation/returnToTabs'; -import { resolveNavigationArtist, splitCollaborators } from '@/library/artistGrouping'; -import { buildArtistNameTokens } from '@/shared/library/artistCredits'; +import { resolveNavigationArtist } from '@/library/artistGrouping'; +import { + buildArtistNameTokens, + parseArtistMetadata, +} from '@/shared/library/artistCredits'; import { artworkThumbFromSource, playerBackdropArtworkSource, @@ -107,9 +113,6 @@ import { } from '@/playback/playbackTargetPresentation'; import { formatSleepTimerStatus } from '@/audio/sleepTimerState'; -const DISMISS_DISTANCE = 140; -const DISMISS_VELOCITY = 1000; - const HEADER_HEIGHT = NOW_PLAYING_HEADER_HEIGHT; const CONTENT_TOP_PADDING = NOW_PLAYING_CONTENT_TOP_PADDING; const CONTENT_BOTTOM_PADDING = NOW_PLAYING_CONTENT_BOTTOM_PADDING; @@ -123,6 +126,7 @@ const MENU_ANIMATION_IN_MS = 130; const MENU_ANIMATION_OUT_MS = 100; const MENU_ENTER_OFFSET_Y = -8; const NOW_PLAYING_SPECTRUM_SMOOTHING = 0.85; +const PAN_DISMISS_HANDOFF_BACKSTOP_MS = 120; interface NowPlayingMenuItem { key: string; @@ -151,8 +155,6 @@ export function NowPlayingOverlay() { const surfacesLive = playerOpen && foreground; const [queueOpen, setQueueOpen] = useState(false); const [lyricsBodySwitching, setLyricsBodySwitching] = useState(false); - // Stable identity: QueueTray is memo'd, so a fresh arrow here would defeat it. - const closeQueue = useCallback(() => setQueueOpen(false), []); const [menuOpen, setMenuOpen] = useState(false); const [targetPickerOpen, setTargetPickerOpen] = useState(false); const [sleepTimerOpen, setSleepTimerOpen] = useState(false); @@ -256,6 +258,13 @@ export function NowPlayingOverlay() { insets.right + contentPadding + Math.max(0, (effectiveWidth - contentPadding * 2 - shellWidth) / 2); + const shellLeft = + insets.left + + contentPadding + + Math.max(0, (effectiveWidth - contentPadding * 2 - shellWidth) / 2); + const companionStartX = tabletCompanionLayout + ? shellLeft + shellWidth - tabletCompanionLayout.companionWidth + : Number.POSITIVE_INFINITY; const menuTop = insets.top + CONTENT_TOP_PADDING + HEADER_HEIGHT + spacing.xs; const libraryTrack = useMemo( () => (track ? libraryTracks.find((entry) => entry.path === track.path) ?? null : null), @@ -274,14 +283,41 @@ export function NowPlayingOverlay() { : ''; const artistCreditTokens = useMemo(() => { if (!track) return []; - const collaborators = - track.artistNames && track.artistNames.length > 0 - ? track.artistNames - : splitCollaborators(track.artist); - return buildArtistNameTokens(collaborators.length > 0 ? collaborators : [track.artist]); + if (track.artistNames && track.artistNames.length > 0) { + return buildArtistNameTokens(track.artistNames); + } + return parseArtistMetadata(track.artist); }, [track]); const albumKey = track?.albumIdentityKey ?? libraryTrack?.album_identity_key; + // The overlay stays mounted; open/close is this one shared value sliding the + // sheet on the UI thread. The gesture itself is memoized below, so changing + // child/body state updates these gates rather than replacing the recognizer. + const translateY = useSharedValue(windowHeight); + const panEnabled = useSharedValue( + shouldEnableNowPlayingPan(playerOpen, queueOpen, lyricsBodySwitching) + ); + const panDismissRequested = useSharedValue(false); + const panExitPending = useSharedValue(false); + const panReleaseVelocity = useSharedValue(0); + const panReleaseRemainingDistance = useSharedValue(windowHeight); + const screenHeight = useSharedValue(windowHeight); + const companionTouchStartX = useSharedValue(companionStartX); + const menuProgress = useSharedValue(0); + const trackProgress = useSharedValue(1); + // ∿ engagement, shared by both scope styles: rail = art shrink + strip fade, + // rack = art face crossfading to the instrument rack. The presence gates keep + // both faces for the 220 ms transition, then release the invisible surface. + const stageProgress = useSharedValue(effectiveScopeStageVisible ? 1 : 0); + + const suspendPanForChildTransition = () => { + panEnabled.value = false; + panDismissRequested.value = false; + panExitPending.value = false; + cancelAnimation(translateY); + translateY.value = 0; + }; + useEffect(() => { if (!hasTabletCompanion || isDesktopTarget) return; if (queueOpen) { @@ -309,6 +345,7 @@ export function NowPlayingOverlay() { } return; } + suspendPanForChildTransition(); setQueueOpen(true); }; @@ -406,16 +443,6 @@ export function NowPlayingOverlay() { }); } - // The overlay stays mounted; open/close is this one shared value sliding the - // sheet on the UI thread. Starts off-screen so a pre-warmed mount never flashes. - const translateY = useSharedValue(windowHeight); - const menuProgress = useSharedValue(0); - const trackProgress = useSharedValue(1); - // ∿ engagement, shared by both scope styles: rail = art shrink + strip fade, - // rack = art face crossfading to the instrument rack. The presence gates keep - // both faces for the 220 ms transition, then release the invisible surface. - const stageProgress = useSharedValue(effectiveScopeStageVisible ? 1 : 0); - /** * Swapping the phone player body unmounts every normal control underneath the * parent pan detector. Suspend that pan for the commit frame and synchronously @@ -424,8 +451,7 @@ export function NowPlayingOverlay() { */ const setPhoneLyricsVisible = (visible: boolean) => { if (!playerOpen) return; - cancelAnimation(translateY); - translateY.value = 0; + suspendPanForChildTransition(); setLyricsBodySwitching(true); void setLyricsVisible(visible); }; @@ -444,6 +470,22 @@ export function NowPlayingOverlay() { return () => cancelAnimationFrame(frame); }, [lyricsBodySwitching, lyricsMode]); + useEffect(() => { + panEnabled.value = shouldEnableNowPlayingPan( + playerOpen, + queueOpen, + lyricsBodySwitching + ); + }, [lyricsBodySwitching, panEnabled, playerOpen, queueOpen]); + + useEffect(() => { + screenHeight.value = windowHeight; + }, [screenHeight, windowHeight]); + + useEffect(() => { + companionTouchStartX.value = companionStartX; + }, [companionStartX, companionTouchStartX]); + useEffect(() => { if (!transitionTrackKey) return; trackProgress.value = 0; @@ -508,49 +550,6 @@ export function NowPlayingOverlay() { ); }; - const pan = Gesture.Pan() - // A child sheet owns vertical gestures while it is visible. Replacing this - // gesture during the queue-button touch used to cancel a partially active - // pan and leave translateY off-screen while phase still said "open". - .enabled(shouldEnableNowPlayingPan(playerOpen, queueOpen, lyricsBodySwitching)) - .activeOffsetY(14) // engage only on a downward drag - .failOffsetY(-14) - .failOffsetX([-24, 24]) // let the horizontal seek drag through - .onUpdate((e) => { - translateY.value = e.translationY > 0 ? e.translationY : 0; - }) - .onEnd((e) => { - if (e.translationY > DISMISS_DISTANCE || e.velocityY > DISMISS_VELOCITY) { - const releaseSpring = resolveNowPlayingDismissSpring( - e.velocityY, - windowHeight - translateY.value - ); - // Same commit-first contract as dismissSheet, with the release spring's - // velocity-matched shaping preserved. - runOnJS(beginDismiss)(); - translateY.value = withSpring( - windowHeight, - { - damping: releaseSpring.damping, - stiffness: releaseSpring.stiffness, - velocity: releaseSpring.velocity, - overshootClamping: true, - energyThreshold: 1e-4, - }, - (finished) => { - if (finished) runOnJS(commitClosed)(); - } - ); - } else { - translateY.value = withTiming(0, motion.snap); - } - }) - .onFinalize((_event, success) => { - // RNGH does not call onEnd for a cancelled gesture. Never leave the - // overlay at its last partial translation in that path. - if (!success) translateY.value = withTiming(0, motion.snap); - }); - // Enter animation. Keyed on `openRequest` as well as the phase, so asking for // a player that already believes it is open still re-runs the slide-in — that // is the recovery path for a sheet stranded off-screen by an interrupted @@ -566,14 +565,46 @@ export function NowPlayingOverlay() { // The gesture and button paths own the exit animation, including its // velocity-matched spring shaping. Only animate here when `closing` // arrived from a direct closePlayer() with no animation attached. - if (!exitAnimated) { + if (exitAnimated && panExitPending.value) { + const releaseSpring = resolveNowPlayingDismissSpring( + panReleaseVelocity.value, + panReleaseRemainingDistance.value + ); + panExitPending.value = false; + cancelAnimation(translateY); + translateY.value = withSpring( + screenHeight.value, + { + damping: releaseSpring.damping, + stiffness: releaseSpring.stiffness, + velocity: releaseSpring.velocity, + overshootClamping: true, + energyThreshold: 1e-4, + }, + (finished) => { + if (finished) runOnJS(commitClosed)(); + } + ); + } else if (!exitAnimated) { translateY.value = withTiming(windowHeight, { duration: 200 }); } return; } translateY.value = withTiming(0, { duration: 240 }); // eslint-disable-next-line react-hooks/exhaustive-deps -- windowHeight excluded on purpose (see above) - }, [phase, openRequest, exitAnimated, queueOpen, lyricsMode, translateY]); + }, [ + phase, + openRequest, + exitAnimated, + queueOpen, + lyricsMode, + commitClosed, + panExitPending, + panReleaseRemainingDistance, + panReleaseVelocity, + screenHeight, + translateY, + ]); // `closing` → `closed`, and `opening` → `open`. Both are timers rather than // animation callbacks, so a cancelled animation can never strand the phase. @@ -599,6 +630,92 @@ export function NowPlayingOverlay() { return undefined; }, [phase, openRequest]); + const pan = useMemo( + () => Gesture.Pan() + .activeOffsetY(14) // engage only on a downward drag + .failOffsetY(-14) + .failOffsetX([-24, 24]) // let the horizontal seek drag through + .onTouchesDown((event, stateManager) => { + const touchX = event.allTouches[0]?.absoluteX ?? Number.NaN; + if ( + !shouldStartNowPlayingPan( + panEnabled.value, + touchX, + companionTouchStartX.value + ) + ) { + stateManager.fail(); + } + }) + .onTouchesMove((_event, stateManager) => { + if (!panEnabled.value) stateManager.fail(); + }) + .onStart(() => { + panDismissRequested.value = false; + panExitPending.value = false; + cancelAnimation(translateY); + }) + .onUpdate((event) => { + if (!panEnabled.value) { + translateY.value = 0; + return; + } + translateY.value = event.translationY > 0 ? event.translationY : 0; + }) + .onEnd((event, success) => { + const release = resolveNowPlayingPanRelease( + event.translationY, + event.velocityY, + success && panEnabled.value + ); + if (release === 'dismiss') { + panDismissRequested.value = true; + panExitPending.value = true; + panReleaseVelocity.value = event.velocityY; + panReleaseRemainingDistance.value = + screenHeight.value - translateY.value; + // If the RN handoff is ever dropped, restore the partial drag instead + // of leaving an open player stranded. The phase effect cancels this + // delayed animation only after Zustand has entered `closing`. + translateY.value = withDelay( + PAN_DISMISS_HANDOFF_BACKSTOP_MS, + withTiming(0, motion.snap) + ); + runOnJS(beginDismiss)(); + return; + } + panDismissRequested.value = false; + panExitPending.value = false; + translateY.value = withTiming(0, motion.snap); + }) + .onFinalize(() => { + // Successful dismissals retain the short handoff backstop above. Every + // other terminal path, including cancellation, re-anchors immediately. + if (!panDismissRequested.value) { + translateY.value = withTiming(0, motion.snap); + } + }), + [ + beginDismiss, + companionTouchStartX, + panDismissRequested, + panEnabled, + panExitPending, + panReleaseRemainingDistance, + panReleaseVelocity, + screenHeight, + translateY, + ] + ); + + // Stable identity: QueueTray is memo'd, so a fresh arrow here would defeat it. + const closeQueue = useCallback(() => { + suspendPanForChildTransition(); + setQueueOpen(false); + // Shared values and the state setter remain stable for this overlay mount. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + // Hardware back, innermost layer first: menu → queue tray → player. Registered // only while open, so it sits above the focused screen's own handlers (LIFO) // — e.g. the library-detail back interceptor underneath. @@ -622,7 +739,7 @@ export function NowPlayingOverlay() { return true; } if (queueOpen) { - setQueueOpen(false); + closeQueue(); return true; } dismissSheet(); @@ -637,6 +754,7 @@ export function NowPlayingOverlay() { sleepTimerOpen, playlistActionTrack, queueOpen, + closeQueue, windowHeight, ]); diff --git a/src/components/player/nowPlayingDismiss.test.mts b/src/components/player/nowPlayingDismiss.test.mts index ab05666..fe009c3 100644 --- a/src/components/player/nowPlayingDismiss.test.mts +++ b/src/components/player/nowPlayingDismiss.test.mts @@ -2,8 +2,10 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { + resolveNowPlayingPanRelease, resolveNowPlayingDismissSpring, shouldEnableNowPlayingPan, + shouldStartNowPlayingPan, } from './nowPlayingDismiss.ts'; test('player pan yields to child sheets and body replacements', () => { @@ -13,6 +15,30 @@ test('player pan yields to child sheets and body replacements', () => { assert.equal(shouldEnableNowPlayingPan(false, false, false), false); }); +test('player pan starts outside child-owned companion scrolling', () => { + assert.equal( + shouldStartNowPlayingPan(true, 900, Number.POSITIVE_INFINITY), + true, + 'phones have no companion boundary' + ); + assert.equal(shouldStartNowPlayingPan(true, 719, 720), true); + assert.equal(shouldStartNowPlayingPan(true, 720, 720), false); + assert.equal(shouldStartNowPlayingPan(true, 900, 720), false); + assert.equal(shouldStartNowPlayingPan(false, 100, 720), false); +}); + +test('only a successfully ended threshold release dismisses the player', () => { + assert.equal(resolveNowPlayingPanRelease(141, 0, true), 'dismiss'); + assert.equal(resolveNowPlayingPanRelease(0, 1001, true), 'dismiss'); + assert.equal(resolveNowPlayingPanRelease(140, 1000, true), 'restore'); + assert.equal(resolveNowPlayingPanRelease(-200, -2000, true), 'restore'); + assert.equal(resolveNowPlayingPanRelease(500, 5000, false), 'restore'); + assert.equal( + resolveNowPlayingPanRelease(Number.POSITIVE_INFINITY, Number.NaN, true), + 'restore' + ); +}); + test('preserves ordinary downward release velocity and spring', () => { for (const velocity of [0, 1, 500, 999, 1000]) { assert.deepEqual(resolveNowPlayingDismissSpring(velocity, 760), { diff --git a/src/components/player/nowPlayingDismiss.ts b/src/components/player/nowPlayingDismiss.ts index a22b406..82852bb 100644 --- a/src/components/player/nowPlayingDismiss.ts +++ b/src/components/player/nowPlayingDismiss.ts @@ -6,6 +6,8 @@ const BASE_DAMPING = 28; const MAX_DAMPING = 60; const MIN_REMAINING_DISTANCE = 120; const VELOCITY_TRAVEL_DAMPING = 1.35; +const DISMISS_DISTANCE = 140; +const DISMISS_VELOCITY = 1000; interface NowPlayingDismissSpring { velocity: number; @@ -13,6 +15,8 @@ interface NowPlayingDismissSpring { damping: number; } +export type NowPlayingPanRelease = 'dismiss' | 'restore'; + /** * The overlay pan must briefly yield when its body is being replaced. Otherwise * RNGH can cancel the old child tree after it has already written a partial @@ -26,6 +30,41 @@ export function shouldEnableNowPlayingPan( return playerOpen && !childSheetOpen && !bodySwitching; } +/** + * Queue/lyrics companion rails own their scroll gestures. A non-finite boundary + * means there is no companion rail, so the whole phone player remains eligible. + */ +export function shouldStartNowPlayingPan( + panEnabled: boolean, + absoluteX: number, + companionStartX: number +): boolean { + 'worklet'; + + if (!panEnabled) return false; + if (!Number.isFinite(companionStartX)) return true; + return Number.isFinite(absoluteX) && absoluteX < companionStartX; +} + +/** + * Cancellation is authoritative even if the last event carried a large + * translation or velocity. Only a successfully ended gesture may dismiss. + */ +export function resolveNowPlayingPanRelease( + translationY: number, + velocityY: number, + success: boolean +): NowPlayingPanRelease { + 'worklet'; + + if (!success) return 'restore'; + const distance = Number.isFinite(translationY) ? Math.max(0, translationY) : 0; + const velocity = Number.isFinite(velocityY) ? Math.max(0, velocityY) : 0; + return distance > DISMISS_DISTANCE || velocity > DISMISS_VELOCITY + ? 'dismiss' + : 'restore'; +} + /** * Preserve the exact release velocity, then make the spring progressively * stronger and more damped for hard flicks so it sheds speed after handoff. diff --git a/src/components/player/nowPlayingPreferences.test.mts b/src/components/player/nowPlayingPreferences.test.mts index 97ea6a6..2a062c5 100644 --- a/src/components/player/nowPlayingPreferences.test.mts +++ b/src/components/player/nowPlayingPreferences.test.mts @@ -1,8 +1,11 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { parseNowPlayingCompanion } from './nowPlayingPreferences.ts'; -import { splitCollaborators } from '../../shared/library/albumGrouping.ts'; -import { buildArtistNameTokens } from '../../shared/library/artistCredits.ts'; +import { + buildArtistNameTokens, + formatArtistNames, + parseArtistMetadata, +} from '../../shared/library/artistCredits.ts'; test('defaults missing and invalid companion preferences to queue', () => { assert.equal(parseNowPlayingCompanion(null), 'queue'); @@ -16,18 +19,33 @@ test('restores persisted queue and lyrics companion preferences', () => { }); test('builds separate clickable credits for collaborative track artists', () => { - const artists = splitCollaborators('Dazbee feat. 9Lana & ValkyR'); - assert.deepEqual(artists, ['Dazbee', '9Lana', 'ValkyR']); + const artists = ['Dazbee', '9Lana', 'ValkyR']; + assert.equal(formatArtistNames(artists), 'Dazbee, 9Lana, ValkyR'); assert.deepEqual(buildArtistNameTokens(artists), [ { artist: 'Dazbee', separator: ', ' }, - { artist: '9Lana', separator: ' & ' }, + { artist: '9Lana', separator: ', ' }, { artist: 'ValkyR', separator: null }, ]); }); test('structured credits preserve commas and ampersands inside one artist name', () => { assert.deepEqual(buildArtistNameTokens(['Earth, Wind & Fire', 'The Emotions']), [ - { artist: 'Earth, Wind & Fire', separator: ' & ' }, + { artist: 'Earth, Wind & Fire', separator: ', ' }, { artist: 'The Emotions', separator: null }, ]); }); + +test('legacy display credits keep their literal separators', () => { + assert.deepEqual(parseArtistMetadata('1, 2, 3'), [ + { artist: '1', separator: ', ' }, + { artist: '2', separator: ', ' }, + { artist: '3', separator: null }, + ]); + assert.deepEqual(parseArtistMetadata('Simon & Garfunkel'), [ + { artist: 'Simon & Garfunkel', separator: null }, + ]); + assert.deepEqual(parseArtistMetadata('One; Two'), [ + { artist: 'One', separator: '; ' }, + { artist: 'Two', separator: null }, + ]); +}); diff --git a/src/shared/library/artistCredits.ts b/src/shared/library/artistCredits.ts index 30d4cbb..3237598 100644 --- a/src/shared/library/artistCredits.ts +++ b/src/shared/library/artistCredits.ts @@ -1,5 +1,6 @@ -// Port of desktop astra/src/shared/library/artistCredits.ts — keep semantically -// identical so the album-grouping algorithm stays in sync across apps. +// Shared credit normalization. Structured arrays retain exact artist +// boundaries; display punctuation must come from the tags or use neutral +// comma separators rather than inventing collaboration punctuation. export interface ArtistNameToken { artist: string; @@ -44,22 +45,46 @@ export function deserializeArtistNames(value: unknown): string[] { } export function formatArtistNames(names: readonly unknown[] | null | undefined): string { - const normalized = normalizeArtistNames(names); - if (normalized.length === 0) return ''; - if (normalized.length === 1) return normalized[0]; - if (normalized.length === 2) return `${normalized[0]} & ${normalized[1]}`; - return `${normalized.slice(0, -1).join(', ')} & ${normalized[normalized.length - 1]}`; + return normalizeArtistNames(names).join(', '); } export function buildArtistNameTokens(names: readonly unknown[] | null | undefined): ArtistNameToken[] { const normalized = normalizeArtistNames(names); - return normalized.map((artist, index) => { - let separator: string | null = null; - if (index < normalized.length - 2) { - separator = ', '; - } else if (index === normalized.length - 2) { - separator = ' & '; - } - return { artist, separator }; - }); + return normalized.map((artist, index) => ({ + artist, + separator: index < normalized.length - 1 ? ', ' : null, + })); +} + +const LITERAL_SEPARATOR_PATTERN = /([,;])/; + +/** + * Build clickable fallback credits from a legacy display string while keeping + * its literal punctuation. Ampersands are intentionally not split: without + * structured metadata, they may be part of an artist's actual name or tag. + */ +export function parseArtistMetadata(artistText: string): ArtistNameToken[] { + const normalized = normalizeArtistName(artistText); + if (!normalized) return []; + + const tokens: ArtistNameToken[] = []; + let pendingSeparator: ',' | ';' | null = null; + + for (const part of normalized.split(LITERAL_SEPARATOR_PATTERN)) { + if (part === ',' || part === ';') { + pendingSeparator = part; + continue; + } + + const artist = normalizeArtistName(part); + if (!artist) continue; + + if (pendingSeparator && tokens.length > 0) { + tokens[tokens.length - 1].separator = `${pendingSeparator} `; + } + tokens.push({ artist, separator: null }); + pendingSeparator = null; + } + + return tokens.length > 0 ? tokens : [{ artist: normalized, separator: null }]; }