diff --git a/CLAUDE.md b/CLAUDE.md index abe00156..4213df8d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -291,6 +291,16 @@ notes**, which is the one place somebody wrote down what happened in a clip. Every non-trivial operation is a class in `src/main/actions/` extending `BaseAction` with a single `execute(input)`. **Add new business logic as an Action**, not inline in a route. +**Anything that sends a file to the Recycle Bin goes through `MoveFileToTrashAction`**, which +normalises the path. Rows store forward slashes (fast-glob's spelling) and `shell.trashItem` refuses +those with "Failed to parse path"; compressing a clip called it directly and failed, after the encode, +on every clip in a real library. `compress-check.mjs`'s shell stub now refuses a forward slash too. + +**A second row for one file is retired by the scan when it holds nothing of its own** +(`services/duplicateRows.ts`). Older builds indexed some files under both slash spellings; the scan +stopped making new ones but never removed an old one, so the clip showed twice. Only the row goes, +never the file, and a duplicate that carries anything its twin lacks is left and logged. + `ScanAndSyncClipsAction` has a **prune guard**: it refuses to delete rows when the videos folder looks empty or when one scan would remove more than half the library. A clip row carries the only copy of its tags, notes, display name, collections and stars, and an unmounted drive makes every file look diff --git a/scripts/compress-check.mjs b/scripts/compress-check.mjs index 334cbcb2..4ede12f9 100644 --- a/scripts/compress-check.mjs +++ b/scripts/compress-check.mjs @@ -109,14 +109,17 @@ const isGreen = ({ rgb: [r, g, b] }) => g > 60 && g - r > 40 && g - b > 40; /* ------------------------------------------------------------ the source */ announceRoot(ROOT); -const dir = join(ROOT, game); +// A file instead of a game folder runs the bench on that exact recording, +// which is how a clip somebody reports as not compressing gets reproduced. +const givenFile = VIDEO.test(game) && existsSync(game) ? game : null; +const dir = givenFile ? join(givenFile, '..') : join(ROOT, game); if (!existsSync(dir)) { console.error(`no such folder: ${dir}`); process.exit(2); } -let source = null; -for (const name of readdirSync(dir).filter((f) => VIDEO.test(f) && !f.startsWith('.'))) { +let source = givenFile; +for (const name of givenFile ? [] : readdirSync(dir).filter((f) => VIDEO.test(f) && !f.startsWith('.'))) { const candidate = join(dir, name); if ((await seconds(candidate)) >= MIN_SOURCE_SEC) { source = candidate; @@ -133,10 +136,11 @@ mkdirSync(TMP, { recursive: true }); const library = join(tmpdir(), `goodbit-compress-check-${Date.now()}`); const profile = join(library, '.profile'); -mkdirSync(join(library, game), { recursive: true }); +const folder = givenFile ? 'Clips' : game; +mkdirSync(join(library, folder), { recursive: true }); mkdirSync(profile, { recursive: true }); -const working = join(library, game, source.split(/[\\/]/).pop()); +const working = join(library, folder, source.split(/[\\/]/).pop()); copyFileSync(source, working); /* @@ -176,7 +180,9 @@ writeFileSync( // The real one moves a file to the Recycle Bin. There is no bin to check // here and leaving the original in place would make the rename fail, so it // is deleted, and the bench asserts the sequence rather than the bin. - 'export const shell = { trashItem: (p) => rm(p, { force: true }) };', + // Refuses a forward-slash path exactly as the real one does on Windows, which is + // how every compression in a real library failed while this bench passed. + 'export const shell = { trashItem: async (p) => { if (process.platform === "win32" && p.includes("/")) throw new Error("Failed to parse path"); await rm(p, { force: true }); } };', "export const screen = { getPrimaryDisplay: () => ({ id: 0, label: '' }), getAllDisplays: () => [] };", 'export default { app, shell, screen };', '', @@ -220,11 +226,12 @@ const before = { const clip = await repo.save( repo.create({ - filePath: working, + // Stored the way a real library stores it: fast-glob hands back forward slashes. + filePath: working.split(String.fromCharCode(92)).join("/"), relPath: working.slice(library.length + 1), filename: working.split(/[\\/]/).pop(), extension: 'mp4', - game, + game: folder, sizeBytes: before.bytes, durationSec: before.seconds, fileModifiedAt: before.mtime, @@ -309,7 +316,7 @@ ok('and the picture has colour in it', middle.saturation > 3, middle.saturation. /* ---------------------------------------- a clip that is already small */ console.log('\na clip already smaller than the preset aims for'); -const small = join(library, game, 'already-small.mp4'); +const small = join(library, folder, 'already-small.mp4'); await execFileAsync(ffmpeg, [ '-v', 'error', '-y', '-f', 'lavfi', '-i', 'testsrc=size=640x360:rate=30:duration=4', @@ -324,7 +331,7 @@ const smallRow = await repo.save( relPath: small.slice(library.length + 1), filename: 'already-small.mp4', extension: 'mp4', - game, + game: folder, sizeBytes: statSync(small).size, durationSec: 4, fileModifiedAt: statSync(small).mtime, diff --git a/src/main/actions/CompressClipAction.ts b/src/main/actions/CompressClipAction.ts index f75bbc64..6027f9f8 100644 --- a/src/main/actions/CompressClipAction.ts +++ b/src/main/actions/CompressClipAction.ts @@ -1,6 +1,6 @@ +import { MoveFileToTrashAction } from './MoveFileToTrashAction.js'; import path from 'node:path'; import fsPromises from 'node:fs/promises'; -import { shell } from 'electron'; import { BaseAction } from './BaseAction.js'; import { AppDataSource } from '../data-source.js'; import { Clip } from '../entity/Clip.js'; @@ -160,8 +160,11 @@ export class CompressClipAction extends BaseAction { } } + await this.retireDuplicateRows(); + const allClips = await clipRepo.find(); const missing = allClips.filter((clip) => !nowOnDisk.has(samePath(clip.filePath))); @@ -253,6 +256,45 @@ export class ScanAndSyncClipsAction extends BaseAction { return { added, updated, removed, total, hidden: Number(hiddenRow?.n ?? 0), pruneSkipped }; } -} + /** + * Remove a second row for a file that already has one, when it holds nothing + * of its own. See `services/duplicateRows.ts` for the rule; this is the + * reading and the deleting. Rows only: the file is never touched. + */ + private async retireDuplicateRows(): Promise { + const rows: Array> = await AppDataSource.query(` + SELECT c.id, c.filePath, c.displayName, c.notes, c.starred, c.published, c.openCount, + (SELECT COUNT(*) FROM clip_tags_tag t WHERE t.clipId = c.id) AS tags, + (SELECT COUNT(*) FROM good_bit g WHERE g.clipId = c.id) AS marks, + (SELECT COUNT(*) FROM collection_clips_clip k WHERE k.clipId = c.id) AS collections + FROM clip c + WHERE REPLACE(LOWER(c.filePath), char(92), '/') IN ( + SELECT REPLACE(LOWER(filePath), char(92), '/') FROM clip + GROUP BY REPLACE(LOWER(filePath), char(92), '/') HAVING COUNT(*) > 1 + )`); + if (!rows.length) return; + const plan = planDuplicateRows( + rows.map((row) => ({ + id: Number(row.id), + filePath: String(row.filePath), + displayName: (row.displayName as string | null) ?? null, + notes: (row.notes as string | null) ?? null, + starred: Boolean(row.starred), + published: Boolean(row.published), + openCount: Number(row.openCount ?? 0), + tags: Number(row.tags), + marks: Number(row.marks), + collections: Number(row.collections), + })), + ); + for (const group of plan.kept) { + console.warn(`[scan] ${group.path} has ${group.ids.length} rows that each hold something; left as they are`); + } + if (plan.remove.length) { + await AppDataSource.getRepository(Clip).delete(plan.remove); + console.log(`[scan] removed ${plan.remove.length} duplicate row(s) for files that already had one: ${plan.remove.join(', ')}`); + } + } +} diff --git a/src/main/routes/clips.ts b/src/main/routes/clips.ts index 028cbb67..7c559902 100644 --- a/src/main/routes/clips.ts +++ b/src/main/routes/clips.ts @@ -800,8 +800,20 @@ clipsRouter.post('/:id/compress', asyncHandler(async (req, res) => { // The row is the answer worth waiting for: the tile has to redraw with // the new size, and a refusal has to redraw with the old one. const after = await AppDataSource.getRepository(Clip).findOneBy({ id: clipId }); + // A refusal is an answer, not a failure, and the job says which in words + // the toast can show: before this it was a log line, so a clip that was + // left alone looked exactly like one that had been compressed. if (!result.replaced) { console.log(`[compress] ${clip.filename}: ${result.reason}, left alone`); + setJobProgress( + jobId, + 100, + result.reason === 'not-smaller' + ? 'Already about as small as a compression would make it, so it was left alone.' + : 'The compressed copy came out the wrong length, so the original was kept.', + ); + } else { + setJobProgress(jobId, 100, 'Compressed'); } completeJob(jobId, after ? ClipDTO.fromEntity(after) : undefined); }) diff --git a/src/main/services/duplicateRows.ts b/src/main/services/duplicateRows.ts new file mode 100644 index 00000000..f7bb33bc --- /dev/null +++ b/src/main/services/duplicateRows.ts @@ -0,0 +1,86 @@ +/** + * Two rows for one file, and which of them may go. + * + * Before the scan learned that `D:/Clips/x.mp4` and `D:\Clips\x.mp4` are the + * same file, a clip indexed by one path spelling and found again by the other + * got a second row. The scan stops making new ones, but it never removed an + * old one: the file exists, so neither row looks orphaned, and the library + * showed the clip twice, once with its publish state and once without. + * + * **Only a row that carries nothing is removed.** A clip row is the only copy + * of its tags, notes, name, stars, marks and collections, so a duplicate is + * retired only when everything it holds is already on the row that stays. + * Anything else is left, twice, and logged: a clip shown twice is an + * annoyance, and a clip whose notes were thrown away is a loss. + * + * The row only. The file is the same file either way and is never touched. + */ +export interface DuplicateCandidate { + id: number; + filePath: string; + displayName: string | null; + notes: string | null; + starred: boolean; + published: boolean; + openCount: number; + tags: number; + marks: number; + collections: number; +} + +export interface DuplicatePlan { + /** Rows safe to delete, because their twin holds everything they do. */ + remove: number[]; + /** Groups left alone because more than one row carries something, for the log. */ + kept: Array<{ path: string; ids: number[] }>; +} + +const BACKSLASH = String.fromCharCode(92); +const samePath = (value: string): string => value.split(BACKSLASH).join('/').toLowerCase(); + +/** How much a row holds that nothing else does. Higher stays. */ +function weight(row: DuplicateCandidate): number { + return ( + row.tags * 1000 + + row.marks * 1000 + + row.collections * 1000 + + (row.notes?.trim() ? 1000 : 0) + + (row.starred ? 100 : 0) + + (row.published ? 100 : 0) + + row.openCount + ); +} + +/** Whether `extra` holds anything `keeper` does not. */ +function carriesMore(extra: DuplicateCandidate, keeper: DuplicateCandidate): boolean { + if (extra.tags > 0 || extra.marks > 0 || extra.collections > 0) return true; + if (extra.notes?.trim() && extra.notes !== keeper.notes) return true; + if (extra.starred && !keeper.starred) return true; + if (extra.published && !keeper.published) return true; + const name = extra.displayName?.trim(); + if (name && name !== keeper.displayName?.trim()) return true; + return false; +} + +export function planDuplicateRows(rows: readonly DuplicateCandidate[]): DuplicatePlan { + const groups = new Map(); + for (const row of rows) { + const key = samePath(row.filePath); + const group = groups.get(key); + if (group) group.push(row); + else groups.set(key, [row]); + } + + const plan: DuplicatePlan = { remove: [], kept: [] }; + for (const [path, group] of groups) { + if (group.length < 2) continue; + // The richest row stays; the oldest breaks a tie, since it is the one + // anything outside the database is most likely to know about. + const ordered = [...group].sort((a, b) => weight(b) - weight(a) || a.id - b.id); + const [keeper, ...rest] = ordered; + const removable = rest.filter((row) => !carriesMore(row, keeper)); + plan.remove.push(...removable.map((row) => row.id)); + if (removable.length < rest.length) plan.kept.push({ path, ids: ordered.map((row) => row.id) }); + } + return plan; +} diff --git a/src/renderer/src/App.vue b/src/renderer/src/App.vue index e80abde9..44b39a37 100644 --- a/src/renderer/src/App.vue +++ b/src/renderer/src/App.vue @@ -43,6 +43,7 @@ onUnmounted(() => { :confirm-label="pending?.confirmLabel" :tone="pending?.tone" :icon="pending?.icon" + :emphasis="pending?.emphasis" @confirm="accept" @cancel="cancel" /> diff --git a/src/renderer/src/components/Base/BaseConfirmDialog.vue b/src/renderer/src/components/Base/BaseConfirmDialog.vue index 47338d64..20c6e7e1 100644 --- a/src/renderer/src/components/Base/BaseConfirmDialog.vue +++ b/src/renderer/src/components/Base/BaseConfirmDialog.vue @@ -1,5 +1,5 @@