From e45c126a3f266d4e3883e70648d752443882b312 Mon Sep 17 00:00:00 2001 From: jeffvli Date: Tue, 18 Nov 2025 03:33:18 -0800 Subject: [PATCH] fix lyrics components --- package.json | 2 +- pnpm-lock.yaml | 6 +- src/renderer/features/lyrics/lyrics.tsx | 20 +-- .../features/lyrics/synchronized-lyrics.tsx | 154 ++++++------------ 4 files changed, 61 insertions(+), 121 deletions(-) diff --git a/package.json b/package.json index f025f7b27..d8c4d626f 100644 --- a/package.json +++ b/package.json @@ -78,7 +78,6 @@ "@tanstack/react-query-devtools": "^5.90.2", "@tanstack/react-query-persist-client": "^5.90.11", "@ts-rest/core": "^3.52.1", - "@types/react-window": "^1.8.8", "@wavesurfer/react": "^1.0.11", "@xhayper/discord-rpc": "^1.3.0", "audiomotion-analyzer": "^4.5.1", @@ -142,6 +141,7 @@ "@types/node": "^24.10.1", "@types/react": "^19.2.5", "@types/react-dom": "^19.2.3", + "@types/react-window": "^1.8.8", "@types/source-map-support": "^0.5.10", "@types/ws": "^8.18.1", "@vitejs/plugin-react": "^5.1.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4d5971368..1c09fc89a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -62,9 +62,6 @@ importers: '@ts-rest/core': specifier: ^3.52.1 version: 3.52.1(@types/node@24.10.1)(zod@3.25.76) - '@types/react-window': - specifier: ^1.8.8 - version: 1.8.8 '@wavesurfer/react': specifier: ^1.0.11 version: 1.0.11(react@19.1.0)(wavesurfer.js@7.11.1) @@ -249,6 +246,9 @@ importers: '@types/react-dom': specifier: ^19.2.3 version: 19.2.3(@types/react@19.2.5) + '@types/react-window': + specifier: ^1.8.8 + version: 1.8.8 '@types/source-map-support': specifier: ^0.5.10 version: 0.5.10 diff --git a/src/renderer/features/lyrics/lyrics.tsx b/src/renderer/features/lyrics/lyrics.tsx index 1758becd1..f83f83548 100644 --- a/src/renderer/features/lyrics/lyrics.tsx +++ b/src/renderer/features/lyrics/lyrics.tsx @@ -19,8 +19,9 @@ import { UnsynchronizedLyrics, UnsynchronizedLyricsProps, } from '/@/renderer/features/lyrics/unsynchronized-lyrics'; +import { usePlayerEvents } from '/@/renderer/features/player/audio-player/hooks/use-player-events'; import { queryClient } from '/@/renderer/lib/react-query'; -import { usePlayerSong, useLyricsSettings, usePlayerStore } from '/@/renderer/store'; +import { useLyricsSettings, usePlayerSong } from '/@/renderer/store'; import { Center } from '/@/shared/components/center/center'; import { Group } from '/@/shared/components/group/group'; import { Icon } from '/@/shared/components/icon/icon'; @@ -130,22 +131,17 @@ export const Lyrics = () => { }), ); - useEffect(() => { - const unsubSongChange = usePlayerStore.subscribe( - (state) => state.current.song, - () => { + usePlayerEvents( + { + onCurrentSongChange: () => { setOverride(undefined); setIndex(0); setShowTranslation(false); setTranslatedLyrics(null); }, - { equalityFn: (a, b) => a?.id === b?.id }, - ); - - return () => { - unsubSongChange(); - }; - }, []); + }, + [], + ); useEffect(() => { if (lyrics && !translatedLyrics && enableAutoTranslation) { diff --git a/src/renderer/features/lyrics/synchronized-lyrics.tsx b/src/renderer/features/lyrics/synchronized-lyrics.tsx index b86d8204a..7ff146998 100644 --- a/src/renderer/features/lyrics/synchronized-lyrics.tsx +++ b/src/renderer/features/lyrics/synchronized-lyrics.tsx @@ -1,21 +1,13 @@ import clsx from 'clsx'; import isElectron from 'is-electron'; -import { useCallback, useEffect, useRef } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; import styles from './synchronized-lyrics.module.css'; import { LyricLine } from '/@/renderer/features/lyrics/lyric-line'; +import { usePlayerEvents } from '/@/renderer/features/player/audio-player/hooks/use-player-events'; import { useScrobble } from '/@/renderer/features/player/hooks/use-scrobble'; -import { PlayersRef } from '/@/renderer/features/player/ref/players-ref'; -import { - useLyricsSettings, - usePlaybackType, - usePlayerActions, - usePlayerData, - usePlayerNum, - usePlayerStatus, - usePlayerTimestamp, -} from '/@/renderer/store'; +import { useLyricsSettings, usePlaybackType, usePlayerActions } from '/@/renderer/store'; import { FullLyricsMetadata, SynchronizedLyricsArray } from '/@/shared/types/domain-types'; import { PlayerStatus, PlayerType } from '/@/shared/types/types'; @@ -36,31 +28,26 @@ export const SynchronizedLyrics = ({ source, translatedLyrics, }: SynchronizedLyricsProps) => { - const playersRef = PlayersRef; - const status = usePlayerStatus(); const playbackType = usePlaybackType(); - const playerData = usePlayerData(); - const now = usePlayerTimestamp(); const settings = useLyricsSettings(); - const currentPlayer = usePlayerNum(); - const currentPlayerRef = - currentPlayer === 1 ? playersRef.current?.player1 : playersRef.current?.player2; - + const { mediaSeekToTimestamp } = usePlayerActions(); const { handleScrobbleFromSeek } = useScrobble(); + // State for player status and timestamp from events + const [status, setStatus] = useState(PlayerStatus.PAUSED); + const [timestamp, setTimestamp] = useState(0); + const handleSeek = useCallback( (time: number) => { if (playbackType === PlayerType.LOCAL && mpvPlayer) { mpvPlayer.seekTo(time); - // setCurrentTime(time, true); } else { - // setCurrentTime(time, true); handleScrobbleFromSeek(time); mpris?.updateSeek(time); - currentPlayerRef?.seekTo(time); + mediaSeekToTimestamp(time); } }, - [currentPlayerRef, handleScrobbleFromSeek, playbackType], + [handleScrobbleFromSeek, mediaSeekToTimestamp, playbackType], ); // const seeked = useSeeked(); @@ -95,30 +82,9 @@ export const SynchronizedLyrics = ({ return -1; }; - const getCurrentTime = useCallback(async () => { - if (isElectron() && playbackType !== PlayerType.WEB) { - if (mpvPlayer) { - return mpvPlayer.getCurrentTime(); - } - return 0; - } - - if (playersRef.current === undefined) { - return 0; - } - - const player = - playerData.player.playerNum === 1 - ? playersRef.current.player1 - : playersRef.current.player2; - const underlying = player?.getInternalPlayer(); - - // If it is null, this probably means we added a new song while the lyrics tab is open - // and the queue was previously empty - if (!underlying) return 0; - - return underlying.currentTime; - }, [playbackType, playersRef, playerData]); + const setCurrentLyricRef = useRef< + (timeInMs: number, epoch?: number, targetIndex?: number) => void + >(() => {}); const setCurrentLyric = useCallback( (timeInMs: number, epoch?: number, targetIndex?: number) => { @@ -177,7 +143,7 @@ export const SynchronizedLyrics = ({ lyricTimer.current = setTimeout( () => { - setCurrentLyric(nextTime, nextEpoch, index + 1); + setCurrentLyricRef.current(nextTime, nextEpoch, index + 1); }, nextTime - timeInMs - elapsed, ); @@ -186,6 +152,28 @@ export const SynchronizedLyrics = ({ [], ); + // Store the callback in a ref so it can be called recursively + useEffect(() => { + setCurrentLyricRef.current = setCurrentLyric; + }, [setCurrentLyric]); + + // Subscribe to player events + usePlayerEvents( + { + onPlayerProgress: (properties) => { + setTimestamp(properties.timestamp); + }, + onPlayerSeekToTimestamp: (properties) => { + // When seeking, update timestamp immediately + setTimestamp(properties.timestamp); + }, + onPlayerStatus: (properties) => { + setStatus(properties.status); + }, + }, + [], + ); + useEffect(() => { // Copy the follow settings into a ref that can be accessed in the timeout followRef.current = settings.follow; @@ -194,40 +182,21 @@ export const SynchronizedLyrics = ({ useEffect(() => { // This handler is used to handle when lyrics change. It is in some sense the // 'primary' handler for parsing lyrics, as unlike the other callbacks, it will - // ALSO remove listeners on close. Use the promisified getCurrentTime(), because - // we don't want to be dependent on npw, which may not be precise + // ALSO remove listeners on close. lyricRef.current = lyrics; if (status === PlayerStatus.PLAYING) { - let rejected = false; - - getCurrentTime() - .then((timeInSec: number) => { - if (rejected) { - return false; - } - - setCurrentLyric(timeInSec * 1000 - delayMsRef.current); - - return true; - }) - .catch(console.error); + // Use the current timestamp from player events + setCurrentLyric(timestamp * 1000 - delayMsRef.current); return () => { - // Case 1: cleanup happens before we hear back from - // the main process. In this case, when the promise resolves, ignore the result - rejected = true; - - // Case 2: Cleanup happens after we hear back from main process but - // (potentially) before the next lyric. In this case, clear the timer. - // Do NOT do this for other cleanup functions, as it should only be done - // when switching to a new song (or an empty one) + // Cleanup: clear the timer when lyrics change or component unmounts if (lyricTimer.current) clearTimeout(lyricTimer.current); }; } return () => {}; - }, [getCurrentTime, lyrics, playbackType, setCurrentLyric, status]); + }, [lyrics, setCurrentLyric, status, timestamp]); useEffect(() => { // This handler is used to deal with changes to the current delay. If the offset @@ -236,39 +205,22 @@ export const SynchronizedLyrics = ({ const changed = delayMsRef.current !== settings.delayMs; if (!changed) { - return () => {}; + return; } if (lyricTimer.current) { clearTimeout(lyricTimer.current); } - let rejected = false; - delayMsRef.current = settings.delayMs; - getCurrentTime() - .then((timeInSec: number) => { - if (rejected) { - return false; - } - - setCurrentLyric(timeInSec * 1000 - delayMsRef.current); - - return true; - }) - .catch(console.error); - - return () => { - // In the event this ends earlier, just kill the promise. Cleanup of - // timeouts is otherwise handled by another handler - rejected = true; - }; - }, [getCurrentTime, setCurrentLyric, settings.delayMs]); + // Use the current timestamp from player events + setCurrentLyric(timestamp * 1000 - delayMsRef.current); + }, [setCurrentLyric, settings.delayMs, timestamp]); useEffect(() => { - // This handler is used specifically for dealing with seeking. In this case, - // we assume that now is the accurate time + // This handler is used specifically for dealing with seeking and progress updates. + // When the timestamp changes, update the current lyric position. if (status !== PlayerStatus.PLAYING) { if (lyricTimer.current) { clearTimeout(lyricTimer.current); @@ -277,20 +229,12 @@ export const SynchronizedLyrics = ({ return; } - // If the time goes back to 0 and we are still playing, this suggests that - // we may be playing the same track (repeat one). In this case, we also - // need to restart playback - const restarted = status === PlayerStatus.PLAYING && now === 0; - // if (!seeked && !restarted) { - // return; - // } - if (lyricTimer.current) { clearTimeout(lyricTimer.current); } - setCurrentLyric(now * 1000 - delayMsRef.current); - }, [now, setCurrentLyric, status]); + setCurrentLyric(timestamp * 1000 - delayMsRef.current); + }, [timestamp, setCurrentLyric, status]); useEffect(() => { // Guaranteed cleanup; stop the timer, and just in case also increment