feat: notice an album I already have before reviewing it
The destination path is not enough to answer "do I already have this": a re-download whose artist folder is spelled differently lands somewhere else entirely, and the user only finds out after reviewing the cover of an album they already own. An album is now matched against the journal on the folded artist and album names -- case, spacing and punctuation removed -- as well as against the disk on the destination path. Matching on tags catches the re-download; matching on the path catches an album copied in by hand, which may have no record at all. The finding is shown on the metadata step, before the review starts, with three choices one keypress each: - keep both (the default, because it is the only choice that cannot lose an album) files the new copy beside the old one as "<album> (2)"; - replace removes the album already there once the new one has landed; - skip leaves this album alone and goes back to the list. Those keys take priority over scrolling on that step, because 'k' would otherwise scroll the screen, which is exactly the kind of small collision that makes somebody press the wrong thing at the wrong moment. Closes 6dd3e6a
diff --git a/README.md b/README.md
@@ -24,6 +24,8 @@ produces a clean, consistently tagged, consistently named library: lossless mast
- Transcodes a streaming copy from the masters and optimises its cover
- Remembers artist folder decisions in `artists.toml` and the journal, so the same
artist always lands in the same folder
+- Notices when an album is already in the library — by artist and name, not just by path —
+ before the review starts, and offers keep both, replace or skip
- `b` on the album list files everything ready in one unattended pass, parking whatever
needs a decision and naming it at the end; `n` jumps straight to the next parked album
- `J` on the album list opens the journal: every record, with what is actually on disk
diff --git a/docs/DESIGN.md b/docs/DESIGN.md
@@ -193,6 +193,19 @@ the masters back to staging and removes the library copy, which is derived. Un-f
moves rather than copies, because after filing the archive holds the only copy of that
audio.
+## Already having it
+
+The destination path is not enough to answer "do I already have this": a re-download whose
+artist folder is spelled differently lands somewhere else entirely, and the user finds out
+after reviewing the cover of an album they already own. An album is matched to the journal
+on the folded artist and album names, and to the disk on the destination path, and the
+finding is shown before the review starts rather than at the end.
+
+Keeping both is the default, because it is the only choice that cannot lose an album. It
+files the new copy beside the old one as `<album> (2)`, and everything after that — the
+summary, the journal and the library built by the same run — follows the album to where it
+actually went.
+
## Filing unattended
`batch` files every ready album without stopping for a decision. An album that needs a
diff --git a/internal/cli/cli.go b/internal/cli/cli.go
@@ -37,6 +37,9 @@ type App struct {
ImagePreview bool
Logf func(string)
Quiet bool
+ // Collision overrides what happens when the destination already exists:
+ // "replace", "merge", "both" or "" to work it out.
+ Collision string
}
func New(cfg config.Config, out, errw io.Writer, in io.Reader) *App {
@@ -1444,6 +1447,76 @@ func humanBytes(n int64) string {
return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGT"[exp])
}
+// Existing describes an album that is already in the archive, found before the
+// album in front of the user reaches the point of being written.
+type Existing struct {
+ ArchiveDir string
+ LibraryDir string
+ AlbumArtist string
+ Album string
+ Tracks int
+ // SamePath is true when the destination is exactly the one in the plan, so
+ // filing would land on top of it.
+ SamePath bool
+}
+
+// LookupExisting answers "do I already have this?", which the destination path
+// alone cannot: a re-download whose artist folder is spelled differently ends up
+// somewhere else entirely, and the user only finds out after reviewing the cover
+// of an album they already own.
+func (a *App) LookupExisting(p plan.Plan) (Existing, bool) {
+ j, err := a.journal()
+ if err != nil {
+ return Existing{}, false
+ }
+ if p.ArchiveDir != "" {
+ if _, err := os.Stat(p.ArchiveDir); err == nil {
+ return Existing{
+ ArchiveDir: p.ArchiveDir,
+ LibraryDir: p.LibraryDir,
+ AlbumArtist: p.AlbumArt,
+ Album: p.AlbumName,
+ Tracks: len(p.Tracks),
+ SamePath: true,
+ }, true
+ }
+ }
+ wantArtist := foldCredit(p.AlbumArt)
+ wantAlbum := foldCredit(p.AlbumName)
+ if wantArtist == "" || wantAlbum == "" {
+ return Existing{}, false
+ }
+ for _, rec := range j.Albums {
+ if rec.Phase != state.PhaseArchived {
+ continue
+ }
+ if foldCredit(rec.AlbumArtist) != wantArtist || foldCredit(rec.Album) != wantAlbum {
+ continue
+ }
+ return Existing{
+ ArchiveDir: rec.ArchiveDir,
+ LibraryDir: rec.LibraryDir,
+ AlbumArtist: rec.AlbumArtist,
+ Album: rec.Album,
+ Tracks: rec.Tracks,
+ SamePath: strings.EqualFold(filepath.Clean(rec.ArchiveDir), filepath.Clean(p.ArchiveDir)),
+ }, true
+ }
+ return Existing{}, false
+}
+
+// foldCredit makes two spellings of the same credit comparable: case, spacing
+// and punctuation are the things that differ between rips of one album.
+func foldCredit(s string) string {
+ var b strings.Builder
+ for _, r := range strings.ToLower(s) {
+ if r >= 'a' && r <= 'z' || r >= '0' && r <= '9' {
+ b.WriteRune(r)
+ }
+ }
+ return b.String()
+}
+
// BatchReport is what an unattended run did.
type BatchReport struct {
Filed []string
diff --git a/internal/cli/file.go b/internal/cli/file.go
@@ -261,10 +261,12 @@ func (a *App) FileAlbum(ctx context.Context, dir string, logf func(string, ...an
}
}
- collision := ""
- if _, statErr := os.Stat(p.ArchiveDir); statErr == nil {
- collision = "both"
- a.logf("destination already exists; keeping both copies")
+ collision := a.Collision
+ if collision == "" {
+ if _, statErr := os.Stat(p.ArchiveDir); statErr == nil {
+ collision = "both"
+ a.logf("destination already exists; keeping both copies")
+ }
}
return a.FilePlan(ctx, p, cover, collision)
}
@@ -320,10 +322,12 @@ func (a *App) FileAlbumWithCover(ctx context.Context, dir, coverPath string, pla
}
}
- collision := ""
- if _, statErr := os.Stat(p.ArchiveDir); statErr == nil {
- collision = "both"
- a.logf("destination already exists; keeping both copies")
+ collision := a.Collision
+ if collision == "" {
+ if _, statErr := os.Stat(p.ArchiveDir); statErr == nil {
+ collision = "both"
+ a.logf("destination already exists; keeping both copies")
+ }
}
return a.FilePlan(ctx, p, cover, collision)
}
diff --git a/internal/cli/journal_test.go b/internal/cli/journal_test.go
@@ -462,3 +462,60 @@ func TestBatchParksWhatNeedsYouAndCarriesOn(t *testing.T) {
t.Errorf("a second batch failed on %v", rep2.Failed)
}
}
+
+// The destination path alone does not answer "do I already have this": a
+// re-download whose artist folder is spelled differently lands somewhere else
+// entirely, and the user only finds out after reviewing the cover of an album
+// they already own.
+func TestLookupExistingFindsTheSameAlbumUnderAnotherSpelling(t *testing.T) {
+ root := t.TempDir()
+ a, _ := cliApp(t, root)
+
+ owned := filepath.Join(root, "archive", "boards-of-canada", "music-has-the-right")
+ seed(t, a, state.Record{
+ Source: "/staging/old", Phase: state.PhaseArchived,
+ AlbumArtist: "Boards of Canada", Album: "Music Has the Right to Children",
+ ArchiveDir: owned, Tracks: 18,
+ })
+
+ p := plan.Plan{
+ AlbumName: "music has the right to children",
+ AlbumArt: "boards of canada",
+ ArchiveDir: filepath.Join(root, "archive", "boards-of-canada", "music-has-the-right-to-children"),
+ }
+ got, ok := a.LookupExisting(p)
+ if !ok {
+ t.Fatal("an album with the same artist and name under another folder was not found")
+ }
+ if got.ArchiveDir != owned {
+ t.Errorf("found %q, want %q", got.ArchiveDir, owned)
+ }
+ if got.SamePath {
+ t.Error("the paths differ, so SamePath should be false")
+ }
+
+ // A different album is not a match.
+ other := plan.Plan{AlbumName: "Geogaddi", AlbumArt: "Boards of Canada"}
+ if _, ok := a.LookupExisting(other); ok {
+ t.Error("a different album matched")
+ }
+}
+
+// An album already sitting exactly where the plan would write it is found even
+// with no journal record at all, which is what happens to anything copied in by
+// hand.
+func TestLookupExistingFindsAnUnrecordedAlbumAtTheSamePath(t *testing.T) {
+ root := t.TempDir()
+ a, _ := cliApp(t, root)
+ dir := filepath.Join(root, "archive", "an-artist", "an-album")
+ if err := os.MkdirAll(dir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ got, ok := a.LookupExisting(plan.Plan{ArchiveDir: dir})
+ if !ok {
+ t.Fatal("an album already at the destination was not found")
+ }
+ if !got.SamePath {
+ t.Error("SamePath should be true")
+ }
+}
diff --git a/internal/tui/runner.go b/internal/tui/runner.go
@@ -18,12 +18,17 @@ import (
)
type Runner struct {
- Cfg config.Config
- choices map[string]CoverOption
+ Cfg config.Config
+ choices map[string]CoverOption
+ collisions map[string]string
}
func NewRunner(cfg config.Config) *Runner {
- return &Runner{Cfg: cfg, choices: map[string]CoverOption{}}
+ return &Runner{
+ Cfg: cfg,
+ choices: map[string]CoverOption{},
+ collisions: map[string]string{},
+ }
}
func (r *Runner) Prepare(ctx context.Context, dir string) (plan.Plan, error) {
@@ -85,6 +90,27 @@ func (r *Runner) Journal(ctx context.Context) ([]JournalEntry, error) {
return out, nil
}
+func (r *Runner) Existing(ctx context.Context, p plan.Plan) (Existing, bool) {
+ e, ok := r.app(nil).LookupExisting(p)
+ if !ok {
+ return Existing{}, false
+ }
+ return Existing{
+ ArchiveDir: e.ArchiveDir,
+ LibraryDir: e.LibraryDir,
+ AlbumArtist: e.AlbumArtist,
+ Album: e.Album,
+ Tracks: e.Tracks,
+ SamePath: e.SamePath,
+ }, true
+}
+
+// Collide records what to do when the destination already exists, for the album
+// the user is looking at right now.
+func (r *Runner) Collide(albumDir, mode string) {
+ r.collisions[albumDir] = mode
+}
+
// Attention maps each staging directory an unattended run parked to the reason.
func (r *Runner) Attention(ctx context.Context) (map[string]string, error) {
app := r.app(nil)
@@ -146,8 +172,12 @@ func (r *Runner) Run(ctx context.Context, album scan.Album, progress func(string
return outcomeOf(res), nil
}
+ if mode := r.collisions[album.Dir]; mode != "" {
+ app.Collision = mode
+ }
res, err := app.FileAlbumWithCover(ctx, album.Dir, choice.Path, choice.Placeholder, nil)
delete(r.choices, album.Dir)
+ delete(r.collisions, album.Dir)
if err != nil {
return Outcome{}, err
}
@@ -190,6 +220,8 @@ func Run(cfg config.Config, out io.Writer, in io.Reader) error {
CoverOptions: r.CoverOptions,
ApplyCover: r.ApplyCover,
Attention: r.Attention,
+ Existing: r.Existing,
+ Collide: r.Collide,
Batch: r.Batch,
Journal: r.Journal,
ForgetSource: r.ForgetSource,
diff --git a/internal/tui/tui.go b/internal/tui/tui.go
@@ -3,6 +3,7 @@ package tui
import (
"context"
"fmt"
+ "path/filepath"
"sort"
"strings"
"time"
@@ -64,6 +65,18 @@ type Options struct {
UnfileSource func(source string, progress func(string)) error
Batch func(ctx context.Context, progress func(string)) (BatchReport, error)
Attention func(ctx context.Context) (map[string]string, error)
+ Existing func(ctx context.Context, p plan.Plan) (Existing, bool)
+ Collide func(albumDir, mode string)
+}
+
+// Existing is an album intake already has, found before this one is filed.
+type Existing struct {
+ ArchiveDir string
+ LibraryDir string
+ AlbumArtist string
+ Album string
+ Tracks int
+ SamePath bool
}
// BatchReport is what an unattended run did, for the summary at the end.
@@ -143,6 +156,8 @@ type Module struct {
placed *kittyPlacement
attention map[string]string
+ existing *Existing
+ collision string
batch *BatchReport
journal []JournalEntry
journalCursor int
@@ -207,6 +222,19 @@ func (m Module) journalCmd() tea.Cmd {
}
}
+type existingMsg struct {
+ existing Existing
+ found bool
+}
+
+func (m Module) existingCmd(p plan.Plan) tea.Cmd {
+ get := m.opts.Existing
+ return func() tea.Msg {
+ e, ok := get(context.Background(), p)
+ return existingMsg{existing: e, found: ok}
+ }
+}
+
type attentionMsg struct {
reasons map[string]string
err error
@@ -367,6 +395,9 @@ func (m Module) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
m.current, m.pl = msg.album, msg.plan
m.screen, m.step, m.scroll = ScreenReview, StepMetadata, 0
m.status = fmt.Sprintf("%d track%s read", len(msg.plan.Tracks), plural(len(msg.plan.Tracks)))
+ if m.opts.Existing != nil {
+ return m, tea.Batch(clear, m.existingCmd(msg.plan))
+ }
return m, clear
case coversMsg:
@@ -396,6 +427,19 @@ func (m Module) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
m.renderCoverPreview(m.coverPreviewWidth(), m.coverPreviewHeight())
return m, tea.Batch(m.placeCover(), replantTick())
+ case existingMsg:
+ if msg.found {
+ e := msg.existing
+ m.existing = &e
+ // Keeping both is the default, because nothing else can lose an
+ // album, and the other two are one keypress away.
+ m.collision = "both"
+ m.status = "you already have this album"
+ return m, nil
+ }
+ m.existing, m.collision = nil, ""
+ return m, nil
+
case attentionMsg:
if msg.err == nil {
m.attention = msg.reasons
@@ -656,6 +700,25 @@ func (m Module) jumpToAttention() (tea.Model, tea.Cmd) {
}
func (m Module) keyReview(key string) (tea.Model, tea.Cmd) {
+ // An album that is already in the library takes the keys first: 'k' would
+ // otherwise scroll, which is exactly the kind of small collision that makes
+ // somebody press the wrong thing at the wrong moment.
+ if m.existing != nil && m.step == StepMetadata {
+ switch key {
+ case "k":
+ m.collision = "both"
+ m.status = "filing beside " + filepath.Base(m.existing.ArchiveDir)
+ return m, nil
+ case "r":
+ m.collision = "replace"
+ m.status = "will replace " + filepath.Base(m.existing.ArchiveDir)
+ return m, nil
+ case "s":
+ m.screen, m.existing, m.collision = ScreenBrowse, nil, ""
+ m.status = "left it alone; you already have it"
+ return m, m.scanCmd()
+ }
+ }
switch key {
case "up", "k":
if m.scroll > 0 {
@@ -729,6 +792,9 @@ func (m Module) nextStep() (tea.Model, tea.Cmd) {
m.screen, m.scroll = ScreenCoverPicker, 0
return m, nil
case StepConfirm:
+ if m.opts.Collide != nil && m.collision != "" {
+ m.opts.Collide(m.current.Dir, m.collision)
+ }
ctx, cancel := context.WithCancel(context.Background())
m.cancel = cancel
m.events = make(chan tea.Msg, 64)
@@ -821,7 +887,7 @@ func (m *Module) resetAlbum() {
m.placed = nil
m.sheetPath, m.sheetBefore = "", nil
m.journalMenu, m.journalAction, m.journalConfirm = -1, 0, false
- m.batch = nil
+ m.batch, m.existing, m.collision = nil, nil, ""
m.log, m.outcome = nil, Outcome{}
m.err, m.status = nil, ""
m.scroll, m.offset = 0, 0
@@ -962,7 +1028,11 @@ func (m Module) renderFooter() string {
hints = []string{"↑↓", "move", "enter", "actions", "r", "refresh", "esc", "back"}
}
case ScreenReview:
- hints = []string{"↑↓", "scroll", "enter", "continue", "e", "edit album", "esc", "back", "q", "quit"}
+ if m.existing != nil && m.step == StepMetadata {
+ hints = []string{"k", "keep both", "r", "replace it", "s", "skip"}
+ } else {
+ hints = []string{"↑↓", "scroll", "enter", "continue", "e", "edit album", "esc", "back", "q", "quit"}
+ }
case ScreenCoverPicker:
hints = []string{"↑↓", "choose", "enter", "use this cover", "e", "edit album", "esc", "cancel"}
case ScreenWorking:
@@ -1295,7 +1365,7 @@ func (m Module) keyJournalMenu(keyp string) (tea.Model, tea.Cmd) {
switch keyp {
case "esc", "q":
m.journalMenu, m.journalAction, m.journalConfirm = -1, 0, false
- m.batch = nil
+ m.batch, m.existing, m.collision = nil, nil, ""
return m, nil
case "up", "k":
if m.journalAction > 0 {
@@ -1325,6 +1395,6 @@ func (m Module) keyJournalMenu(keyp string) (tea.Model, tea.Cmd) {
func (m Module) runJournalAction(a journalAction) (tea.Model, tea.Cmd) {
entry := m.journal[m.journalCursor]
m.journalMenu, m.journalAction, m.journalConfirm = -1, 0, false
- m.batch = nil
+ m.batch, m.existing, m.collision = nil, nil, ""
return m, a.run(&m, entry)
}
diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go
@@ -795,3 +795,59 @@ func TestTheAlbumListMarksWhatNeedsYou(t *testing.T) {
t.Errorf("a ready album was marked: %q", got)
}
}
+
+// Being told before the review starts is the point: otherwise the cover of an
+// album already in the library gets reviewed in full.
+func TestAlreadyHavingTheAlbumIsSaidBeforeTheReview(t *testing.T) {
+ m := testModule(120, 40)
+ m.pl = readyPlan()
+ m.opts.Existing = func(context.Context, plan.Plan) (Existing, bool) {
+ return Existing{ArchiveDir: "/archive/an-artist/an-album", AlbumArtist: "An Artist", Album: "An Album"}, true
+ }
+ m.ready = true
+ m.current = scan.Album{Name: "boards", Dir: m.pl.Album.Dir, Status: scan.StatusReady}
+ m.screen, m.step = ScreenReview, StepMetadata
+ gotModel, _ := m.Update(existingMsg{existing: Existing{ArchiveDir: "/archive/an-artist/an-album"}, found: true})
+ m = gotModel.(Module)
+ if m.existing == nil {
+ t.Fatal("the existing album was not remembered")
+ }
+ if m.collision != "both" {
+ t.Errorf("collision = %q; keeping both is the only choice that cannot lose an album", m.collision)
+ }
+ out := m.View().Content
+ for _, want := range []string{"already have", "keep both", "/archive/an-artist/an-album"} {
+ if !strings.Contains(out, want) {
+ t.Errorf("the metadata screen does not say %q:\n%s", want, out)
+ }
+ }
+}
+
+// The three choices are one keypress each, and skipping leaves the album alone.
+func TestTheAlreadyHaveChoices(t *testing.T) {
+ m := testModule(120, 40)
+ m.pl = readyPlan()
+ m.current = scan.Album{Name: "boards", Dir: m.pl.Album.Dir, Status: scan.StatusReady}
+ m.screen, m.step = ScreenReview, StepMetadata
+ e := Existing{ArchiveDir: "/archive/an-artist/an-album", AlbumArtist: "A", Album: "B"}
+ m.existing, m.collision = &e, "both"
+
+ got, _ := m.handleKey(key("r"))
+ m = got.(Module)
+ if m.collision != "replace" {
+ t.Errorf("collision after 'r' = %q", m.collision)
+ }
+ got, _ = m.handleKey(key("k"))
+ m = got.(Module)
+ if m.collision != "both" {
+ t.Errorf("collision after 'k' = %q", m.collision)
+ }
+ got, cmd := m.handleKey(key("s"))
+ m = got.(Module)
+ if cmd == nil || m.screen != ScreenBrowse {
+ t.Error("'s' should leave the album alone and go back to the list")
+ }
+ if m.existing != nil {
+ t.Error("the existing album should be forgotten when the album is skipped")
+ }
+}
diff --git a/internal/tui/view.go b/internal/tui/view.go
@@ -2,6 +2,7 @@ package tui
import (
"fmt"
+ "os"
"path/filepath"
"strings"
@@ -62,6 +63,28 @@ func (m Module) stepMetadata() string {
b.WriteString(panelWith(m.st, "", meta, m.width, true))
b.WriteString("\n\n")
+ if m.existing != nil {
+ e := m.existing
+ what := "you already have this album"
+ if host, err := os.Hostname(); err == nil && host == "" {
+ _ = host
+ }
+ if !e.SamePath {
+ what = "you already have an album with this artist and name"
+ }
+ where := fmt.Sprintf("%s - %s\n%s", e.AlbumArtist, e.Album, e.ArchiveDir)
+ b.WriteString(" " + m.st.Danger.Render("⚠ "+what) + "\n")
+ b.WriteString(" " + m.st.Muted.Render(truncateTo(where, m.width-6)) + "\n")
+ choice := "keep both: file this one beside it as \"... (2)\""
+ switch m.collision {
+ case "replace":
+ choice = "replace it: the album already there is removed once this one has landed"
+ case "both":
+ }
+ b.WriteString(" " + m.st.Warning.Render(choice) + "\n")
+ b.WriteString(" " + m.st.Muted.Render("k keep both · r replace · s skip this album") + "\n\n")
+ }
+
if m.pl.VariousArt {
b.WriteString(" " + m.st.Warning.Render("compilation") + " " +
m.st.Muted.Render("per-track artists differ, so this files under various-artists/") + "\n\n")