feat(shortcuts): bind tracklist.delete to the confirmation
The binding has been in the defaults and in Settings since it was written, with nothing on the other end of it, because "remove from library" did not exist. It does now — and Delete only *opens* the dialog, never performs the removal, which is the only version defensible one keystroke from a focused row. The e2e case asserts the two things that matter and neither is the row count: the file is still on disk, and a real scan of the real directory does not bring the row back. It watches a control path survive the same scan, because a guard that excluded everything would pass the negative assertion for free — and it restores the database it spends.
This commit is contained in:
@@ -33,15 +33,16 @@ func DefaultBindings() map[string]string {
|
||||
"app.selectAll": "Ctrl+A",
|
||||
"app.shortcuts": "?",
|
||||
|
||||
// Panel-specific (track list). There is no `tracklist.delete`:
|
||||
// it was bound to Delete and advertised in Settings as
|
||||
// configurable while nothing listened for it, because "remove
|
||||
// from library" does not exist and it is not clear what it would
|
||||
// remove — the row (which the next scan puts back unless the path
|
||||
// is also excluded) or the file (a delete-your-music button one
|
||||
// keystroke from a focused row). Advertise it again when it does
|
||||
// something.
|
||||
"tracklist.play": "Enter",
|
||||
// Panel-specific (track list). `tracklist.delete` spent six
|
||||
// phases advertised in Settings with nothing on the other end of
|
||||
// it, because "remove from library" did not exist and it was not
|
||||
// clear what it would remove. It now removes the row and
|
||||
// excludes the path from future scans, and leaves the file on
|
||||
// disk — and the key only *opens the confirmation*, never
|
||||
// performs the removal, which is the only version defensible one
|
||||
// keystroke from a focused row.
|
||||
"tracklist.play": "Enter",
|
||||
"tracklist.delete": "Delete",
|
||||
|
||||
// Panel-specific (autotag review). These are the keys the
|
||||
// autotag page used to bind on its own document listener, which
|
||||
|
||||
@@ -0,0 +1,149 @@
|
||||
import { existsSync } from 'node:fs';
|
||||
|
||||
import { test, expect, resetEvents, callBinding, eventNames } from '../support/fixtures.js';
|
||||
|
||||
/**
|
||||
* "Remove from library" removes the row and leaves the file.
|
||||
*
|
||||
* Two assertions carry this spec and neither is about the row count.
|
||||
* The first is that the **file is still on disk** — that is the promise
|
||||
* the confirmation copy makes, and the only thing standing between this
|
||||
* feature and a user's music. The second is that a **real scan does not
|
||||
* bring the row back**: without the exclusion the operation undoes
|
||||
* itself on the next scan, which is worse than not having it.
|
||||
*
|
||||
* The suite shares one backend process in file order, so this restores
|
||||
* the database it spent.
|
||||
*/
|
||||
const SNAPSHOT = 'e2e-pre-remove';
|
||||
|
||||
/** The file paths of the first n rows, in list order. */
|
||||
const firstPaths = (n: number): string[] => Array.from(
|
||||
document.querySelector('track-list')
|
||||
?.shadowRoot?.querySelectorAll('[data-file-path]') ?? [],
|
||||
).map((r) => r.getAttribute('data-file-path') ?? '').slice(0, n);
|
||||
|
||||
test.describe('remove from library', () => {
|
||||
test.beforeAll(async ({ baseURL }) => {
|
||||
// VACUUM INTO copies the whole file and the restore copies every row
|
||||
// back, which is well over the 30 s a hook gets by default once
|
||||
// earlier specs have staged an explore catalog.
|
||||
test.setTimeout(180_000);
|
||||
|
||||
const res = await fetch(`${baseURL}/__test/db/snapshot?name=${SNAPSHOT}`, {
|
||||
method: 'POST',
|
||||
signal: AbortSignal.timeout(120_000),
|
||||
});
|
||||
|
||||
expect(res.ok, 'could not snapshot the database before spending it').toBe(true);
|
||||
});
|
||||
|
||||
test.afterAll(async ({ baseURL }) => {
|
||||
test.setTimeout(180_000);
|
||||
|
||||
const res = await fetch(`${baseURL}/__test/db/restore?name=${SNAPSHOT}`, {
|
||||
method: 'POST',
|
||||
signal: AbortSignal.timeout(120_000),
|
||||
});
|
||||
|
||||
expect(res.ok, 'could not restore the database this spec spent').toBe(true);
|
||||
});
|
||||
|
||||
test.beforeEach(async ({ app }) => {
|
||||
await app.getByTestId('nav-tracks').click();
|
||||
await expect(app.getByTestId('track-row').first()).toBeVisible();
|
||||
});
|
||||
|
||||
/**
|
||||
* Delete is bound to *opening* the confirmation and to nothing else.
|
||||
* A key that asks is defensible one row from the user's music; a key
|
||||
* that acts is not.
|
||||
*/
|
||||
test('Delete asks, and cancelling is a true no-op', async ({ app }) => {
|
||||
const before = await app.getByTestId('track-row').count();
|
||||
|
||||
await resetEvents(app);
|
||||
await app.evaluate(() => {
|
||||
const rows = document.querySelector('track-list')
|
||||
?.shadowRoot?.querySelectorAll('[data-testid="track-row"]');
|
||||
|
||||
rows?.[2]?.dispatchEvent(new MouseEvent('click', {
|
||||
bubbles: true, composed: true,
|
||||
}));
|
||||
});
|
||||
|
||||
await app.keyboard.press('Delete');
|
||||
|
||||
const dialog = app.getByRole('dialog', { name: /from the library\?/ });
|
||||
|
||||
await expect(dialog).toBeVisible();
|
||||
// The copy is the user's only protection, so it is asserted rather
|
||||
// than assumed: it has to say the file is not deleted.
|
||||
await expect(app.getByTestId('confirm-dialog')).toContainText(
|
||||
/not deleted/,
|
||||
);
|
||||
|
||||
await app.getByTestId('confirm-cancel').click();
|
||||
await expect(dialog).toBeHidden();
|
||||
|
||||
expect(await app.getByTestId('track-row').count()).toBe(before);
|
||||
expect((await eventNames(app))['TracksRemovedFromLibrary'] ?? 0).toBe(0);
|
||||
});
|
||||
|
||||
test('confirming removes the row, keeps the file, and survives a scan', async ({
|
||||
app,
|
||||
}) => {
|
||||
test.setTimeout(120_000);
|
||||
|
||||
const [target, control] = await app.evaluate(firstPaths, 2);
|
||||
|
||||
expect(target, 'no tracks in the library to remove').toBeTruthy();
|
||||
expect(existsSync(target!), 'fixture file missing before the test').toBe(true);
|
||||
|
||||
const before = await app.getByTestId('track-row').count();
|
||||
|
||||
await resetEvents(app);
|
||||
await app.getByTestId('track-row').first().click({ button: 'right' });
|
||||
await app.getByRole('menuitem', { name: 'Remove from Library' }).click();
|
||||
await app.getByTestId('confirm-accept').click();
|
||||
|
||||
const removed = await app.evaluate(
|
||||
() => window.__yjEvents.wait('TracksRemovedFromLibrary', {
|
||||
timeoutMs: 15_000,
|
||||
}),
|
||||
);
|
||||
|
||||
expect((removed.data as Array<Record<string, unknown>>)[0]).toMatchObject({
|
||||
filePaths: [target],
|
||||
count: 1,
|
||||
});
|
||||
|
||||
await expect(app.getByTestId('track-row')).toHaveCount(before - 1);
|
||||
|
||||
// The promise the copy makes.
|
||||
expect(existsSync(target!), 'the file was deleted from disk').toBe(true);
|
||||
|
||||
// And the half that makes the rest true: a real scan of the real
|
||||
// directory must not import it again.
|
||||
await resetEvents(app);
|
||||
await callBinding(app, 'library.Library.ScanAllLibraries', []);
|
||||
await app.evaluate(
|
||||
() => window.__yjEvents.wait('LibraryScanComplete', { timeoutMs: 90_000 }),
|
||||
);
|
||||
await app.waitForTimeout(1000);
|
||||
|
||||
const paths = await app.evaluate(
|
||||
() => Array.from(
|
||||
document.querySelector('track-list')
|
||||
?.shadowRoot?.querySelectorAll('[data-file-path]') ?? [],
|
||||
).map((r) => r.getAttribute('data-file-path') ?? ''),
|
||||
);
|
||||
|
||||
expect(paths, 'the excluded path came back on the next scan')
|
||||
.not.toContain(target);
|
||||
// The positive half: a guard that excluded everything would pass
|
||||
// the assertion above for free.
|
||||
expect(paths, 'the scan lost a path nobody excluded').toContain(control);
|
||||
expect(existsSync(target!), 'the file was deleted from disk').toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -1191,8 +1191,29 @@ export class TrackList
|
||||
'shortcut:tracklist-play',
|
||||
this.handleShortcutPlay,
|
||||
);
|
||||
this.listenWhileActive(
|
||||
document,
|
||||
'shortcut:tracklist-delete',
|
||||
this.handleShortcutDelete,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete opens the confirmation and does nothing else.
|
||||
*
|
||||
* That is the whole design of the binding: one keystroke from a
|
||||
* focused row, a key that *asks* is defensible and a key that
|
||||
* *acts* is not — so this is the same dialog the menu command
|
||||
* opens, reached by a different route.
|
||||
*/
|
||||
private handleShortcutDelete = (): void => {
|
||||
const filePaths = this.selection.getSelectedKeysOrdered();
|
||||
|
||||
if (filePaths.length === 0) return;
|
||||
|
||||
void this.removeFromLibrary(filePaths);
|
||||
};
|
||||
|
||||
/** Enter plays the selection — the `tracklist.play` binding, which
|
||||
* has existed in the defaults and in Settings since it was written
|
||||
* and has never had anything on the other end of it. */
|
||||
|
||||
@@ -438,9 +438,14 @@ async function dispatch(action: string): Promise<void> {
|
||||
);
|
||||
break;
|
||||
|
||||
// No `tracklist.delete`: it dispatched an event nothing
|
||||
// listened for, from a binding Settings advertised as
|
||||
// configurable. See backend/shortcuts/config.go.
|
||||
// `tracklist.delete` opens the confirmation and nothing else:
|
||||
// the key is a request, not an action. See
|
||||
// backend/shortcuts/config.go.
|
||||
case 'tracklist.delete':
|
||||
document.dispatchEvent(
|
||||
new CustomEvent('shortcut:tracklist-delete'),
|
||||
);
|
||||
break;
|
||||
|
||||
// Panel-specific: autotag review. The view listens for these
|
||||
// while it is the view on screen, and for nothing while it is
|
||||
|
||||
@@ -121,6 +121,12 @@ export const SHORTCUT_META: Record<string, ShortcutMeta> = {
|
||||
scope: 'panel:track-list',
|
||||
defaultKey: 'Enter',
|
||||
},
|
||||
'tracklist.delete': {
|
||||
label: 'Remove from Library',
|
||||
category: 'Navigation',
|
||||
scope: 'panel:track-list',
|
||||
defaultKey: 'Delete',
|
||||
},
|
||||
'autotag.apply': {
|
||||
label: 'Apply Match',
|
||||
category: 'Autotag',
|
||||
|
||||
@@ -383,6 +383,35 @@ describe('shortcut dispatch: scope', () => {
|
||||
|
||||
expect(fired).toBe(1);
|
||||
});
|
||||
|
||||
/**
|
||||
* `tracklist.delete` was advertised in Settings for six phases with
|
||||
* nothing dispatching for it. What it dispatches now only *opens* a
|
||||
* confirmation, which is what makes a destructive action defensible
|
||||
* on an unmodified key one row from the user's music.
|
||||
*/
|
||||
it('dispatches for tracklist.delete, which had nothing on the other end', () => {
|
||||
bindings({ 'tracklist.delete': 'Delete' });
|
||||
|
||||
const panel = mount(document.createElement('div'));
|
||||
const row = document.createElement('div');
|
||||
|
||||
row.tabIndex = 0;
|
||||
panel.dataset['shortcutScope'] = 'tracklist';
|
||||
panel.append(row);
|
||||
row.focus();
|
||||
|
||||
let fired = 0;
|
||||
const listener = (): void => {
|
||||
fired += 1;
|
||||
};
|
||||
|
||||
document.addEventListener('shortcut:tracklist-delete', listener);
|
||||
press('Delete');
|
||||
document.removeEventListener('shortcut:tracklist-delete', listener);
|
||||
|
||||
expect(fired).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
// ===================================================================
|
||||
|
||||
Reference in New Issue
Block a user