feat(tracks): remove from library behind a confirmation
The context menu's one destructive command. Its impact line says the files are not deleted, because a user who reads "remove" as "delete" and finds their music gone was failed by the copy rather than by the operation. The store patches rather than invalidates: the event carries the paths, so the tracks array — the expensive collection — is spliced in place and only the album/artist/genre summaries, whose counts really did change, are refetched. It falls back to a full invalidate when a tracks fetch is already in flight, which is the one case a patch cannot be shown to be equivalent to. Deleting an audio_files row cascades to queue_tracks, so the removal also compacts the queue — the same reload RemoveLibrary does, which unloads the player if the removed track was the one playing.
This commit is contained in:
@@ -129,6 +129,15 @@ func (l *Library) RemoveFromLibrary(filePaths []string) (*RemovalResult, error)
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Deleting an audio_files row cascades to queue_tracks, so the
|
||||||
|
// queue's in-memory copy now holds tracks the database does not —
|
||||||
|
// including, possibly, the one playing. This is the same reload
|
||||||
|
// RemoveLibrary does, and it unloads the player if the current
|
||||||
|
// track was among them.
|
||||||
|
if l.removalHooks.CompactQueue != nil {
|
||||||
|
l.removalHooks.CompactQueue()
|
||||||
|
}
|
||||||
|
|
||||||
// An album, artist or genre whose last track just went is now a row
|
// An album, artist or genre whose last track just went is now a row
|
||||||
// with nothing behind it, and the album list selects from
|
// with nothing behind it, and the album list selects from
|
||||||
// release_groups rather than from audio_files — so it would keep
|
// release_groups rather than from audio_files — so it would keep
|
||||||
|
|||||||
@@ -257,6 +257,32 @@ func TestRemoveFromLibrary_EmitsPatchablePayload(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestRemoveFromLibrary_CompactsTheQueue pins the half that is invisible
|
||||||
|
// from the track list: deleting an audio_files row cascades to
|
||||||
|
// queue_tracks, so the queue's in-memory copy — and the player, if it
|
||||||
|
// was the track playing — has to be told.
|
||||||
|
func TestRemoveFromLibrary_CompactsTheQueue(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
lib, dir, paths, _, libID := setupScanLibrary(t, 2)
|
||||||
|
|
||||||
|
lib.scanInternal(libID, "Test", dir)
|
||||||
|
|
||||||
|
compacted := 0
|
||||||
|
|
||||||
|
lib.SetRemovalHooks(RemovalHooks{
|
||||||
|
CompactQueue: func() { compacted++ },
|
||||||
|
})
|
||||||
|
|
||||||
|
if _, err := lib.RemoveFromLibrary([]string{paths[0]}); err != nil {
|
||||||
|
t.Fatalf("RemoveFromLibrary: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if compacted != 1 {
|
||||||
|
t.Errorf("CompactQueue called %d times, want 1", compacted)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// TestRemoveFromLibrary_RejectsAnEmptyRequest keeps a stray Delete on
|
// TestRemoveFromLibrary_RejectsAnEmptyRequest keeps a stray Delete on
|
||||||
// an empty selection from reaching the database at all.
|
// an empty selection from reaching the database at all.
|
||||||
func TestRemoveFromLibrary_RejectsAnEmptyRequest(t *testing.T) {
|
func TestRemoveFromLibrary_RejectsAnEmptyRequest(t *testing.T) {
|
||||||
|
|||||||
@@ -62,6 +62,9 @@ import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'
|
|||||||
import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
|
import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
|
||||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||||
import { describeError } from '@utils/describe-error';
|
import { describeError } from '@utils/describe-error';
|
||||||
|
import { notificationStore } from '@store/notification-store';
|
||||||
|
import { confirmAction } from '@components/confirm-dialog/confirm-dialog';
|
||||||
|
import { RemoveFromLibrary } from '@go/library/Library';
|
||||||
import { loadTrackDetails } from '@utils/lazy-track-details.js';
|
import { loadTrackDetails } from '@utils/lazy-track-details.js';
|
||||||
import { tracksByFilePath, tracksForPaths } from '@utils/track-index.js';
|
import { tracksByFilePath, tracksForPaths } from '@utils/track-index.js';
|
||||||
import '@components/playlist-picker/playlist-picker.js';
|
import '@components/playlist-picker/playlist-picker.js';
|
||||||
@@ -1615,12 +1618,79 @@ export class TrackList
|
|||||||
void this.openBatchTrackDetails(filePaths);
|
void this.openBatchTrackDetails(filePaths);
|
||||||
}
|
}
|
||||||
break;
|
break;
|
||||||
|
case 'remove-from-library':
|
||||||
|
// The only destructive command in this menu: it asks
|
||||||
|
// first, and it keeps the selection until the user has
|
||||||
|
// answered — the dialog names a count, and clearing the
|
||||||
|
// selection under it would make that count a claim
|
||||||
|
// about nothing.
|
||||||
|
this.ctxMenu.close();
|
||||||
|
void this.removeFromLibrary(filePaths);
|
||||||
|
|
||||||
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
this.selection.clear();
|
this.selection.clear();
|
||||||
this.ctxMenu.close();
|
this.ctxMenu.close();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* "Remove from library", behind a confirmation that says what it
|
||||||
|
* does *and* what it does not.
|
||||||
|
*
|
||||||
|
* The second half is the point. This deletes the database rows and
|
||||||
|
* stops the scanner importing those paths again; the audio files
|
||||||
|
* are left exactly where they are. A user who reads "remove" as
|
||||||
|
* "delete" and finds their music gone would have been failed by the
|
||||||
|
* copy, not by the operation — so the copy says so in the impact
|
||||||
|
* line, where the consequence of every other destructive action in
|
||||||
|
* the app is written.
|
||||||
|
*/
|
||||||
|
private async removeFromLibrary(filePaths: string[]) {
|
||||||
|
const count = filePaths.length;
|
||||||
|
const only =
|
||||||
|
count === 1
|
||||||
|
? tracksByFilePath(this.tracks).get(filePaths[0]!)
|
||||||
|
: undefined;
|
||||||
|
|
||||||
|
const ok = await confirmAction({
|
||||||
|
title:
|
||||||
|
count === 1
|
||||||
|
? `Remove “${only?.TrackName ?? filePaths[0]!}” from the library?`
|
||||||
|
: `Remove ${count.toLocaleString()} tracks from the library?`,
|
||||||
|
message:
|
||||||
|
count === 1
|
||||||
|
? 'It is removed from YellowJacket and will not be added' +
|
||||||
|
' back by a future scan.'
|
||||||
|
: 'They are removed from YellowJacket and will not be' +
|
||||||
|
' added back by a future scan.',
|
||||||
|
impact:
|
||||||
|
count === 1
|
||||||
|
? 'The file is not deleted — it stays on disk exactly' +
|
||||||
|
' where it is. A full rescan brings it back.'
|
||||||
|
: 'The files are not deleted — they stay on disk exactly' +
|
||||||
|
' where they are. A full rescan brings them back.',
|
||||||
|
confirmLabel:
|
||||||
|
count === 1
|
||||||
|
? 'Remove track'
|
||||||
|
: `Remove ${count.toLocaleString()} tracks`,
|
||||||
|
danger: true,
|
||||||
|
});
|
||||||
|
|
||||||
|
if (!ok) return;
|
||||||
|
|
||||||
|
try {
|
||||||
|
await RemoveFromLibrary(filePaths);
|
||||||
|
this.selection.clear();
|
||||||
|
} catch (error) {
|
||||||
|
console.error('Error removing tracks from library:', error);
|
||||||
|
notificationStore.persistent({
|
||||||
|
title: 'Could not remove from library',
|
||||||
|
text: `${count === 1 ? 'That track is' : `Those ${count.toLocaleString()} tracks are`} still in your library. ${describeError(error)}`,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
private onContextMenuFavoriteToggle() {
|
private onContextMenuFavoriteToggle() {
|
||||||
const filePaths =
|
const filePaths =
|
||||||
this.selection.getSelectedKeysOrdered();
|
this.selection.getSelectedKeysOrdered();
|
||||||
@@ -2081,6 +2151,20 @@ export class TrackList
|
|||||||
></wa-icon>
|
></wa-icon>
|
||||||
Track Details
|
Track Details
|
||||||
</wa-dropdown-item>
|
</wa-dropdown-item>
|
||||||
|
<wa-dropdown-item
|
||||||
|
@click=${() =>
|
||||||
|
this.onContextMenuAction(
|
||||||
|
'remove-from-library',
|
||||||
|
)}
|
||||||
|
@mouseenter=${() =>
|
||||||
|
this.ctxMenu.closePlaylistSubmenu()}
|
||||||
|
>
|
||||||
|
<wa-icon
|
||||||
|
slot="icon"
|
||||||
|
name="trash"
|
||||||
|
></wa-icon>
|
||||||
|
Remove from Library
|
||||||
|
</wa-dropdown-item>
|
||||||
</div>
|
</div>
|
||||||
`
|
`
|
||||||
: nothing}
|
: nothing}
|
||||||
|
|||||||
@@ -108,6 +108,9 @@ class LibraryStore {
|
|||||||
EventsOn(Events.TrackPlayCountChanged, (payload: unknown) => {
|
EventsOn(Events.TrackPlayCountChanged, (payload: unknown) => {
|
||||||
this.applyPlayCount(payload);
|
this.applyPlayCount(payload);
|
||||||
});
|
});
|
||||||
|
EventsOn(Events.TracksRemovedFromLibrary, (payload: unknown) => {
|
||||||
|
this.applyTracksRemoved(payload);
|
||||||
|
});
|
||||||
|
|
||||||
this.loadCoverSize();
|
this.loadCoverSize();
|
||||||
this.deferEagerFetch();
|
this.deferEagerFetch();
|
||||||
@@ -559,6 +562,71 @@ class LibraryStore {
|
|||||||
this.notify();
|
this.notify();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Splice removed tracks out in place, and refetch only the
|
||||||
|
* summaries whose counts changed.
|
||||||
|
*
|
||||||
|
* `invalidate()` would be correct and is the expensive answer: it
|
||||||
|
* nulls `tracks` and eagerly refetches it, which is ~37 MB across
|
||||||
|
* the IPC at 50 000 tracks for an operation that removed three
|
||||||
|
* rows. The event carries the paths precisely so this does not have
|
||||||
|
* to happen — the same bargain `TrackPlayCountChanged` makes.
|
||||||
|
*
|
||||||
|
* The album, artist and genre lists really do change (their track
|
||||||
|
* counts, and the row itself when its last track goes), so they are
|
||||||
|
* dropped and refetched. They are the small collections.
|
||||||
|
*/
|
||||||
|
private applyTracksRemoved(payload: unknown): void {
|
||||||
|
const p = payload as { filePaths?: string[] } | null;
|
||||||
|
const removed = p?.filePaths;
|
||||||
|
|
||||||
|
if (!removed || removed.length === 0) return;
|
||||||
|
|
||||||
|
// A tracks fetch already in flight would land holding the rows
|
||||||
|
// that were just deleted, and it captured the cache generation
|
||||||
|
// this patch is about to leave behind. There is no patch that
|
||||||
|
// is equivalent to that, so fall back.
|
||||||
|
if (this.inFlight.has('tracks')) {
|
||||||
|
this.invalidate();
|
||||||
|
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (this.tracks !== null) {
|
||||||
|
const gone = new Set(removed);
|
||||||
|
const kept = this.tracks.filter((t) => !gone.has(t.FilePath));
|
||||||
|
|
||||||
|
// A new array identity even when nothing matched would
|
||||||
|
// invalidate every memoized filter/sort cache keyed on it
|
||||||
|
// for no reason.
|
||||||
|
if (kept.length !== this.tracks.length) {
|
||||||
|
this.tracks = kept;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
this.albums = null;
|
||||||
|
this.artists = null;
|
||||||
|
this.genres = null;
|
||||||
|
// Bumping the cache generation is what stops an album fetch
|
||||||
|
// issued before the removal from committing its pre-removal
|
||||||
|
// answer. Safe for the tracks slot precisely because the guard
|
||||||
|
// above established there is nothing in flight for it.
|
||||||
|
this.cacheGen++;
|
||||||
|
this.inFlight.delete('albums');
|
||||||
|
this.inFlight.delete('artists');
|
||||||
|
this.inFlight.delete('genres');
|
||||||
|
|
||||||
|
this.changeGen++;
|
||||||
|
this.notify();
|
||||||
|
|
||||||
|
const logged = (what: string) => (err: unknown) =>
|
||||||
|
console.error(`library: could not reload ${what}`, err);
|
||||||
|
|
||||||
|
void this.getAlbums().catch(logged('albums'));
|
||||||
|
void this.getArtists().catch(logged('artists'));
|
||||||
|
void this.getGenres().catch(logged('genres'));
|
||||||
|
}
|
||||||
|
|
||||||
private invalidate(): void {
|
private invalidate(): void {
|
||||||
this.tracks = null;
|
this.tracks = null;
|
||||||
this.albums = null;
|
this.albums = null;
|
||||||
|
|||||||
@@ -175,6 +175,55 @@ describe('library store: caching', () => {
|
|||||||
expect(calls()).toEqual([]);
|
expect(calls()).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Removing tracks is the same bargain the play count makes, one
|
||||||
|
* collection wider: the event carries the paths so the tracks array
|
||||||
|
* — the expensive one — is patched rather than refetched, while the
|
||||||
|
* album/artist/genre summaries, whose counts really did change, are
|
||||||
|
* dropped and reloaded.
|
||||||
|
*/
|
||||||
|
describe('tracks removed from the library', () => {
|
||||||
|
beforeEach(async () => {
|
||||||
|
emit(Events.TracksRemovedFromLibrary, {
|
||||||
|
filePaths: ['/a.mp3'],
|
||||||
|
count: 1,
|
||||||
|
});
|
||||||
|
await flush();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not refetch the tracks', () => {
|
||||||
|
expect(calls('library.Library.GetAllTracks')).toHaveLength(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('splices the removed track out in place', () => {
|
||||||
|
expect(libraryStore.getCachedTracks()?.map((t) => t.FilePath)).toEqual([
|
||||||
|
'/b.mp3',
|
||||||
|
]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('reloads the summaries, whose counts changed', () => {
|
||||||
|
expect(
|
||||||
|
[
|
||||||
|
'library.Library.GetAllAlbums',
|
||||||
|
'library.Library.GetAllArtists',
|
||||||
|
'library.Library.GetAllGenresWithCounts',
|
||||||
|
].map((path) => calls(path).length),
|
||||||
|
).toEqual([1, 1, 1]);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it('ignores a removal naming a track it does not hold, without dropping the array', async () => {
|
||||||
|
const before = libraryStore.getCachedTracks();
|
||||||
|
|
||||||
|
emit(Events.TracksRemovedFromLibrary, {
|
||||||
|
filePaths: ['/not-in-this-library.mp3'],
|
||||||
|
count: 1,
|
||||||
|
});
|
||||||
|
await flush();
|
||||||
|
|
||||||
|
expect(libraryStore.getCachedTracks()).toBe(before);
|
||||||
|
});
|
||||||
|
|
||||||
it('resets scroll positions on invalidation, so a shorter list is not scrolled past its end', async () => {
|
it('resets scroll positions on invalidation, so a shorter list is not scrolled past its end', async () => {
|
||||||
libraryStore.setScrollPosition('albums', 4200);
|
libraryStore.setScrollPosition('albums', 4200);
|
||||||
emit(Events.LibraryScanComplete);
|
emit(Events.LibraryScanComplete);
|
||||||
|
|||||||
Reference in New Issue
Block a user