From 8913380f812d71ebfdf9ca6e38fe735eee62282e Mon Sep 17 00:00:00 2001 From: void-function865 Date: Sat, 27 Jun 2026 20:43:39 +0200 Subject: [PATCH 1/2] feat(lightbox): add rating keyboard shortcuts for images MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement the `r` + digit rating shortcuts (issue #5616) while viewing images in the lightbox, honoring the configured Stars/Decimal system. The lightbox pauses the global Mousetrap singleton while open, so it now owns a separate (non-paused) Mousetrap instance — mousetrap-pause tracks `paused` per instance — and reuses the existing useRatingKeybinds hook (now accepting an optional instance) rather than duplicating its logic. The hand-rolled `d d` delete double-tap is replaced with the standard Mousetrap "d d" sequence binding on that instance. Also fix a pre-existing bug in useRatingKeybinds: each `r` scheduled an independent unbind timer, so a quick second sequence (e.g. "r 3" then "r 4") could have its digits unbound mid-way by the earlier timer and the keypress dropped. The unbind timer is now cancelled and rescheduled per sequence (and the decimal buffer is reset at the start of each sequence). Document the lightbox rating shortcuts in the keyboard shortcuts manual. --- .../src/docs/en/Manual/KeyboardShortcuts.md | 5 + ui/v2.5/src/hooks/Lightbox/Lightbox.tsx | 91 +++++++++++++------ ui/v2.5/src/hooks/keybinds.ts | 44 ++++++--- 3 files changed, 99 insertions(+), 41 deletions(-) diff --git a/ui/v2.5/src/docs/en/Manual/KeyboardShortcuts.md b/ui/v2.5/src/docs/en/Manual/KeyboardShortcuts.md index c561319541..134280683b 100644 --- a/ui/v2.5/src/docs/en/Manual/KeyboardShortcuts.md +++ b/ui/v2.5/src/docs/en/Manual/KeyboardShortcuts.md @@ -142,6 +142,11 @@ | `→` | Next image | | `Escape` | Close lightbox | | `d d` | Delete current image | +| Ratings || +| `r {1-5}` | Set rating (stars) | +| `r 0` | Unset rating (stars) | +| `r {0-9} {0-9}` | Set rating (decimal - `00` for `10.0`) | +| ``r ` `` | Unset rating (decimal) | ## Groups page shortcuts diff --git a/ui/v2.5/src/hooks/Lightbox/Lightbox.tsx b/ui/v2.5/src/hooks/Lightbox/Lightbox.tsx index d2f79c28ea..7bb975fccf 100644 --- a/ui/v2.5/src/hooks/Lightbox/Lightbox.tsx +++ b/ui/v2.5/src/hooks/Lightbox/Lightbox.tsx @@ -1,4 +1,10 @@ -import React, { useCallback, useEffect, useRef, useState } from "react"; +import React, { + useCallback, + useEffect, + useMemo, + useRef, + useState, +} from "react"; import { Button, Col, @@ -15,6 +21,7 @@ import Mousetrap from "mousetrap"; import { Icon } from "src/components/Shared/Icon"; import { LoadingIndicator } from "src/components/Shared/LoadingIndicator"; import useInterval from "../Interval"; +import { useRatingKeybinds } from "../keybinds"; import usePageVisibility from "../PageVisibility"; import { useToast } from "../Toast"; import { FormattedMessage, useIntl } from "react-intl"; @@ -146,7 +153,6 @@ export const LightboxComponent: React.FC = ({ // image the index has since moved to (e.g. a page-switch settle landing // while the dialog is open). const [deleteTarget, setDeleteTarget] = useState(null); - const lastDKeyTime = useRef(0); const [navOffset, setNavOffset] = useState(); // An in-flight page switch's intended landing, set synchronously by @@ -187,6 +193,17 @@ export const LightboxComponent: React.FC = ({ [images, page, pageCallback, setSwitching] ); + // The lightbox pauses the global Mousetrap singleton while open, so it owns a + // separate (non-paused) instance for its own sequence shortcuts (ratings, + // "d d"). The mousetrap-pause plugin tracks `paused` per-instance, so this + // instance keeps firing while global shortcuts stay suppressed. + const mousetrap = useMemo(() => new Mousetrap(), []); + useEffect(() => { + return () => { + mousetrap.reset(); + }; + }, [mousetrap]); + const [zoom, setZoom] = useState(1); function updateZoom(v: number) { @@ -536,20 +553,8 @@ export const LightboxComponent: React.FC = ({ if (e.key === "ArrowLeft") handleLeft(); else if (e.key === "ArrowRight") handleRight(); else if (e.key === "Escape") close(); - else if (e.key === "d") { - // Not while a page switch is in flight: the index is parked at 0 then, - // so the shortcut would target an image the user isn't viewing. - const image = images[index ?? initialIndex]; - if (!isSwitchingPageRef.current && image?.id !== undefined) { - const now = Date.now(); - if (now - lastDKeyTime.current < 1000) { - setDeleteTarget(image); - } - lastDKeyTime.current = now; - } - } }, - [setInstant, handleLeft, handleRight, close, images, index, initialIndex] + [setInstant, handleLeft, handleRight, close] ); const [clearCallback, resetCallback] = useInterval( @@ -637,6 +642,49 @@ export const LightboxComponent: React.FC = ({ }; const currentIndex = index === null ? initialIndex : index; + const currentImageId = images[currentIndex]?.id; + + function setRating(v: number | null) { + if (currentImageId) { + updateImage({ + variables: { + input: { + id: currentImageId, + rating100: v, + }, + }, + }); + } + } + + // Rating shortcuts ("r" then digit(s)) via the lightbox-scoped Mousetrap + // instance, reusing the same hook as the scene/image detail pages. + useRatingKeybinds( + isVisible, + config?.ui.ratingSystemOptions?.type, + (v) => setRating(Number.isNaN(v) ? null : v), + mousetrap + ); + + // "d d" delete shortcut, using Mousetrap's native sequence binding (matching + // the rest of the app) on the lightbox-scoped instance. Rebinding on + // currentImageId change resets Mousetrap's own sequence tracking, so a "d" + // press on one image can't combine with a second "d" press after + // navigating to another. + useEffect(() => { + if (!isVisible || currentImageId === undefined) return; + + mousetrap.bind("d d", () => { + // Not while a page switch is in flight: the index is parked at 0 then, + // so the shortcut would target an image the user isn't viewing. + if (isSwitchingPageRef.current) return; + const image = images[currentIndex]; + if (image?.id !== undefined) setDeleteTarget(image); + }); + return () => { + mousetrap.unbind("d d"); + }; + }, [isVisible, currentImageId, images, currentIndex, mousetrap]); useEffect(() => { // Don't auto-close while images are still loading. Some entry points open @@ -862,19 +910,6 @@ export const LightboxComponent: React.FC = ({ const currentImage: ILightboxImage | undefined = images[currentIndex]; const title = currentImage ? imageTitle(currentImage) : undefined; - function setRating(v: number | null) { - if (currentImage?.id) { - updateImage({ - variables: { - input: { - id: currentImage.id, - rating100: v, - }, - }, - }); - } - } - async function onIncrementClick() { if (currentImage?.id === undefined) return; try { diff --git a/ui/v2.5/src/hooks/keybinds.ts b/ui/v2.5/src/hooks/keybinds.ts index c863bbffd0..06cb7df2cd 100644 --- a/ui/v2.5/src/hooks/keybinds.ts +++ b/ui/v2.5/src/hooks/keybinds.ts @@ -5,9 +5,23 @@ import { RatingSystemType } from "src/utils/rating"; export function useRatingKeybinds( isVisible: boolean, ratingSystem: RatingSystemType | undefined, - setRating: (v: number) => void + setRating: (v: number) => void, + mousetrap: Pick = Mousetrap ) { const firstChar = useRef(undefined); + const ratingTimeout = useRef>(); + + // (Re)start the 1s window after each "r"-initiated sequence, cancelling any + // pending unbind. Without this, pressing "r" again before the window elapses + // leaves the earlier timeout scheduled, which then unbinds the digit keys + // mid-sequence and drops the next keypress (e.g. a quick "r 3" then "r 4"). + function restartRatingTimeout(unbind: () => void) { + if (ratingTimeout.current) clearTimeout(ratingTimeout.current); + ratingTimeout.current = setTimeout(() => { + ratingTimeout.current = undefined; + unbind(); + }, 1000); + } const starRatingShortcuts: { [char: string]: number } = { "0": NaN, @@ -20,23 +34,27 @@ export function useRatingKeybinds( function handleStarRatingKeybinds() { for (const key in starRatingShortcuts) { - Mousetrap.bind(key, () => setRating(starRatingShortcuts[key])); + mousetrap.bind(key, () => setRating(starRatingShortcuts[key])); } - setTimeout(() => { + restartRatingTimeout(() => { for (const key in starRatingShortcuts) { - Mousetrap.unbind(key); + mousetrap.unbind(key); } - }, 1000); + }); } function handleDecimalKeybinds() { - Mousetrap.bind("`", () => { + // start each sequence fresh so a new "r" doesn't combine with a digit left + // buffered from a previous, abandoned sequence + firstChar.current = undefined; + + mousetrap.bind("`", () => { setRating(NaN); }); for (let i = 0; i <= 9; ++i) { - Mousetrap.bind(i.toString(), () => { + mousetrap.bind(i.toString(), () => { if (firstChar.current !== undefined) { let combined = parseInt(firstChar.current + i.toString(), 10); if (combined === 0) { @@ -51,20 +69,20 @@ export function useRatingKeybinds( }); } - setTimeout(() => { + restartRatingTimeout(() => { firstChar.current = undefined; - Mousetrap.unbind("`"); + mousetrap.unbind("`"); for (let i = 0; i <= 9; ++i) { - Mousetrap.unbind(i.toString()); + mousetrap.unbind(i.toString()); } - }, 1000); + }); } useEffect(() => { if (!isVisible) return; - Mousetrap.bind("r", () => { + mousetrap.bind("r", () => { // numeric keypresses get caught by jwplayer, so blur the element // if the rating sequence is started if (document.activeElement instanceof HTMLElement) { @@ -79,7 +97,7 @@ export function useRatingKeybinds( }); return () => { - Mousetrap.unbind("r"); + mousetrap.unbind("r"); }; }); } From d1257d867ff42dd2eef467cc081446a058c88d8e Mon Sep 17 00:00:00 2001 From: void-function865 Date: Sun, 19 Jul 2026 12:53:35 +0200 Subject: [PATCH 2/2] fix(lightbox): rewrite rating keybind sequencing as a state machine Addresses review feedback from Gykes on #7088. The previous fix cancelled the pending unbind timer in the "r" effect's cleanup, but that cleanup ran on every render (the effect's deps included useCallback-wrapped handlers that depend on setRating, which every caller recreates each render). Cleanup running on every render meant it could flush the pending unbind mid-sequence on any unrelated re-render, unbinding the digit/backtick keys before the 1s window elapsed and dropping keystrokes -- a variant of the same class of bug this PR originally fixed. Root cause: the hook coupled the digit/backtick bindings' lifetime to a dynamic bind/unbind cycle tied to render-driven effect churn. Rewritten so "r", the digits, and "`" are bound once for isVisible's lifetime, and a ref-tracked sequence state (idle/star/decimal) plus a single timeout gate their behavior instead. setRating/ratingSystem are read through refs updated every render, so the bind effect only depends on isVisible/mousetrap and no longer reruns on every render. --- ui/v2.5/src/hooks/keybinds.ts | 146 ++++++++++++++++++---------------- 1 file changed, 77 insertions(+), 69 deletions(-) diff --git a/ui/v2.5/src/hooks/keybinds.ts b/ui/v2.5/src/hooks/keybinds.ts index 06cb7df2cd..eb13842ef0 100644 --- a/ui/v2.5/src/hooks/keybinds.ts +++ b/ui/v2.5/src/hooks/keybinds.ts @@ -2,102 +2,110 @@ import Mousetrap from "mousetrap"; import { useEffect, useRef } from "react"; import { RatingSystemType } from "src/utils/rating"; +const starRatingShortcuts: { [char: string]: number } = { + "0": NaN, + "1": 20, + "2": 40, + "3": 60, + "4": 80, + "5": 100, +}; + +type RatingSequenceMode = "idle" | "star" | "decimal"; + export function useRatingKeybinds( isVisible: boolean, ratingSystem: RatingSystemType | undefined, setRating: (v: number) => void, mousetrap: Pick = Mousetrap ) { - const firstChar = useRef(undefined); - const ratingTimeout = useRef>(); + // setRating/ratingSystem are recreated every render by every caller (they + // close over the currently displayed entity). Reading them through refs, + // updated unconditionally on each render, lets the bind effect below key + // only off isVisible/mousetrap while still always acting on the latest + // values -- rebinding "r" itself isn't needed to pick up a fresh setRating. + const setRatingRef = useRef(setRating); + setRatingRef.current = setRating; + const ratingSystemRef = useRef(ratingSystem); + ratingSystemRef.current = ratingSystem; - // (Re)start the 1s window after each "r"-initiated sequence, cancelling any - // pending unbind. Without this, pressing "r" again before the window elapses - // leaves the earlier timeout scheduled, which then unbinds the digit keys - // mid-sequence and drops the next keypress (e.g. a quick "r 3" then "r 4"). - function restartRatingTimeout(unbind: () => void) { - if (ratingTimeout.current) clearTimeout(ratingTimeout.current); - ratingTimeout.current = setTimeout(() => { - ratingTimeout.current = undefined; - unbind(); - }, 1000); - } + const mode = useRef("idle"); + const firstChar = useRef(undefined); + const sequenceTimeout = useRef>(); - const starRatingShortcuts: { [char: string]: number } = { - "0": NaN, - "1": 20, - "2": 40, - "3": 60, - "4": 80, - "5": 100, - }; + useEffect(() => { + if (!isVisible) return; - function handleStarRatingKeybinds() { - for (const key in starRatingShortcuts) { - mousetrap.bind(key, () => setRating(starRatingShortcuts[key])); + function endSequence() { + mode.current = "idle"; + firstChar.current = undefined; + if (sequenceTimeout.current) { + clearTimeout(sequenceTimeout.current); + sequenceTimeout.current = undefined; + } } - restartRatingTimeout(() => { - for (const key in starRatingShortcuts) { - mousetrap.unbind(key); + // "r", the digits and "`" are bound unconditionally for isVisible's + // lifetime, and gate their behaviour on `mode` instead of being bound + // and unbound per sequence. Callers pass a setRating closure they don't + // memoize, so this effect only depends on isVisible/mousetrap -- if it + // depended on setRating too, an unrelated re-render could tear down and + // rebind this effect mid-sequence, unbinding the digit keys before the + // 1s window elapses. + mousetrap.bind("r", () => { + // numeric keypresses get caught by jwplayer, so blur the element + // if the rating sequence is started + if (document.activeElement instanceof HTMLElement) { + document.activeElement.blur(); } - }); - } - function handleDecimalKeybinds() { - // start each sequence fresh so a new "r" doesn't combine with a digit left - // buffered from a previous, abandoned sequence - firstChar.current = undefined; + mode.current = + !ratingSystemRef.current || + ratingSystemRef.current === RatingSystemType.Stars + ? "star" + : "decimal"; + firstChar.current = undefined; + + if (sequenceTimeout.current) clearTimeout(sequenceTimeout.current); + sequenceTimeout.current = setTimeout(endSequence, 1000); + }); mousetrap.bind("`", () => { - setRating(NaN); + if (mode.current !== "decimal") return; + setRatingRef.current(NaN); + endSequence(); }); for (let i = 0; i <= 9; ++i) { mousetrap.bind(i.toString(), () => { - if (firstChar.current !== undefined) { - let combined = parseInt(firstChar.current + i.toString(), 10); - if (combined === 0) { - combined = 100; - } + if (mode.current === "star") { + const value = starRatingShortcuts[i.toString()]; + if (value === undefined) return; + setRatingRef.current(value); + endSequence(); + } else if (mode.current === "decimal") { + if (firstChar.current !== undefined) { + let combined = parseInt(firstChar.current + i.toString(), 10); + if (combined === 0) { + combined = 100; + } - setRating(combined); - firstChar.current = undefined; - } else { - firstChar.current = i.toString(); + setRatingRef.current(combined); + endSequence(); + } else { + firstChar.current = i.toString(); + } } }); } - restartRatingTimeout(() => { - firstChar.current = undefined; - + return () => { + mousetrap.unbind("r"); mousetrap.unbind("`"); for (let i = 0; i <= 9; ++i) { mousetrap.unbind(i.toString()); } - }); - } - - useEffect(() => { - if (!isVisible) return; - - mousetrap.bind("r", () => { - // numeric keypresses get caught by jwplayer, so blur the element - // if the rating sequence is started - if (document.activeElement instanceof HTMLElement) { - document.activeElement.blur(); - } - - if (!ratingSystem || ratingSystem === RatingSystemType.Stars) { - return handleStarRatingKeybinds(); - } else { - return handleDecimalKeybinds(); - } - }); - - return () => { - mousetrap.unbind("r"); + endSequence(); }; - }); + }, [isVisible, mousetrap]); }