From a2cb20851499e9c00bd3bf642680a9ce3148cae8 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Thu, 17 Sep 2026 10:15:45 +0200 Subject: gui: name the other copy of a duplicate, and offer to keep this one instead --- CHANGELOG.md | 4 +++ docs/gui-checklist.md | 5 ++++ gui/internal/model/plan.go | 65 +++++++++++++++++++++++++++++++++++------ gui/internal/model/plan_test.go | 55 ++++++++++++++++++++++++++++++++++ gui/internal/ui/plan.go | 58 ++++++++++++++++++++++++++++++++---- internal/engine/facts.go | 12 ++++++++ internal/engine/match.go | 8 +++++ internal/engine/match_test.go | 34 +++++++++++++++++++++ man/krino-gui.1 | 12 ++++++-- 9 files changed, 237 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47af5e9..a981b39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +- `FileMatch.DuplicateOf`: the file a `(duplicate)` test matched, absolute, + so a front end can say where the other copy is and act on it. The reason + text is unchanged. + ## 0.0.10 — 2026-09-16 A window, `krino-gui`, over the same engine: review and apply a plan, look diff --git a/docs/gui-checklist.md b/docs/gui-checklist.md index b7292b9..5a7d08a 100644 --- a/docs/gui-checklist.md +++ b/docs/gui-checklist.md @@ -136,3 +136,8 @@ A dialog is its own window: take it by its own id, not the main window's. 44. The sort picker lists the plan as scanned, by name, size, age, action or rule; Settings holds the one a fresh window starts with, and the picker keeps what is chosen by hand until the window closes. +45. A duplicate's explanation names the other copy in full, so it is clear + whether it is in this directory or elsewhere. +46. "Keep this copy, replace the other" appears only for a file with + another copy, asks first, naming both, and after Apply the kept file is + in the other's place with the other in the Trash. diff --git a/gui/internal/model/plan.go b/gui/internal/model/plan.go index 51a86d8..7c24991 100644 --- a/gui/internal/model/plan.go +++ b/gui/internal/model/plan.go @@ -20,12 +20,16 @@ import ( // Row is one line of the Plan tab: a file krino would act on, or one it // could not decide about. type Row struct { - Rel string // the file, relative to the directory's root - Size int64 - ModTime time.Time - Steps []plan.Step - Rule string // the rule that matched first, for the Rule column - Warnings []string + Rel string // the file, relative to the directory's root + Path string // absolute, for acting on the file itself + Size int64 + ModTime time.Time + // DuplicateOf is the copy a (duplicate) test matched this file + // against, absolute; "" when none did. + DuplicateOf string + Steps []plan.Step + Rule string // the rule that matched first, for the Rule column + Warnings []string // Selected is the checkbox. A row that cannot be applied - every step // skipped, or nothing but warnings - is never selected and has no box. Selected bool @@ -109,9 +113,13 @@ func (t *PlanTab) fill() { t.Warnings = append([]string(nil), r.Warnings...) warnings := map[string][]string{} files := map[string]scan.File{} + dupes := map[string]string{} for _, fms := range [][]engine.FileMatch{r.Matched, r.Unmatched} { for _, fm := range fms { files[fm.File.Rel] = fm.File + if fm.DuplicateOf != "" { + dupes[fm.File.Rel] = fm.DuplicateOf + } if len(fm.Warnings) > 0 { warnings[fm.File.Rel] = fm.Warnings } @@ -125,7 +133,8 @@ func (t *PlanTab) fill() { } t.Rows = nil for _, c := range t.dp.Chains { - row := Row{Rel: c.File.Rel, Size: c.File.Size, ModTime: c.File.ModTime, + row := Row{Rel: c.File.Rel, Path: c.File.Path, Size: c.File.Size, + ModTime: c.File.ModTime, DuplicateOf: dupes[c.File.Rel], Steps: c.Steps, Warnings: warnings[c.File.Rel]} for _, s := range c.Steps { if s.Skip == "" { @@ -150,8 +159,8 @@ func (t *PlanTab) fill() { sort.Strings(rest) for _, rel := range rest { f := files[rel] - t.Rows = append(t.Rows, Row{Rel: rel, Size: f.Size, ModTime: f.ModTime, - Warnings: warnings[rel]}) + t.Rows = append(t.Rows, Row{Rel: rel, Path: f.Path, Size: f.Size, + ModTime: f.ModTime, DuplicateOf: dupes[rel], Warnings: warnings[rel]}) } t.Counts = Counts{ Scanned: len(r.Matched) + len(r.Unmatched) + len(r.Skipped), @@ -247,6 +256,44 @@ func (t *PlanTab) ReplaceSelected(kind plan.Kind) (int, error) { return n, nil } +// KeepThisCopy is the answer to "I want this one, not the one already +// filed": the file takes the other copy's place, and the other copy goes to +// the Trash, where krino undo can still reach it. It is a review decision, +// like trashing a file by hand, so the rule that a duplicate is never +// deleted - which binds rules, not the person reading the plan - does not +// stand in its way (his request, 2026-09-17). +func (t *PlanTab) KeepThisCopy(i int) error { + if i < 0 || i >= len(t.Rows) { + return fmt.Errorf("model: no row %d", i) + } + row := t.Rows[i] + if row.DuplicateOf == "" { + return fmt.Errorf("model: %s is not a duplicate of anything krino looked at", row.Rel) + } + if row.Path == "" { + return fmt.Errorf("model: %s has no path", row.Rel) + } + for j, c := range t.dp.Chains { + if c.File.Rel != row.Rel { + continue + } + t.dp.Chains[j].Steps = []plan.Step{{ + Kind: plan.Move, + Rule: "(review)", + Src: row.Path, + Dst: row.DuplicateOf, + Displaces: row.DuplicateOf, + Reason: "chosen in review: this copy replaces the one already there", + }} + t.Rows[i].Steps = t.dp.Chains[j].Steps + t.Rows[i].Rule = "(review)" + t.Rows[i].Actable = true + t.Rows[i].Selected = true + return nil + } + return fmt.Errorf("model: %s is not in this plan", row.Rel) +} + // Apply acts on the selected files and logs the rest as declined, exactly // as choosing per file in the terminal does. Each row then carries its // outcome. diff --git a/gui/internal/model/plan_test.go b/gui/internal/model/plan_test.go index a54768c..0aa0361 100644 --- a/gui/internal/model/plan_test.go +++ b/gui/internal/model/plan_test.go @@ -408,3 +408,58 @@ func TestAgeText(t *testing.T) { t.Errorf("a file from the future = %q, want 0m", got) } } + +// TestKeepThisCopy: choosing the downloaded copy over the filed one puts it +// in the other's place and sends the other to the Trash, where undo can +// still reach it (his request, 2026-09-17). +func TestKeepThisCopy(t *testing.T) { + conf := "(path \"~/dl\")\n(rule \"dupes\" (when (duplicate \"~/docs\")) (move \"Dupes\"))\n" + e, h := sandboxDir(t, conf, map[string]string{"report.pdf": "the same bytes"}) + filed := filepath.Join(h, "docs", "work", "report.pdf") + if err := os.MkdirAll(filepath.Dir(filed), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filed, []byte("the same bytes"), 0o644); err != nil { + t.Fatal(err) + } + tab := planTab(t, e) + if len(tab.Rows) != 1 || tab.Rows[0].DuplicateOf != filed { + t.Fatalf("row = %+v, want it a duplicate of %s", tab.Rows[0], filed) + } + if err := tab.KeepThisCopy(0); err != nil { + t.Fatal(err) + } + step := tab.Rows[0].Steps[0] + if step.Kind != plan.Move || step.Dst != filed || step.Displaces != filed { + t.Fatalf("step = %+v", step) + } + if _, err := tab.Apply(context.Background()); err != nil { + t.Fatal(err) + } + // The download is now the filed copy, with its own bytes. + if body, err := os.ReadFile(filed); err != nil || string(body) != "the same bytes" { + t.Errorf("the kept copy is not in place: %v", err) + } + if _, err := os.Stat(filepath.Join(h, "dl", "report.pdf")); !os.IsNotExist(err) { + t.Errorf("the download is still where it was: %v", err) + } + // The copy it replaced went to the Trash, not to oblivion. + entries, _ := os.ReadDir(filepath.Join(h, ".local", "share", "Trash", "files")) + if len(entries) != 1 { + t.Errorf("the Trash holds %d files, want the replaced one", len(entries)) + } +} + +// TestKeepThisCopyRefusesANonDuplicate: the choice only means something for +// a file krino found another copy of. +func TestKeepThisCopyRefusesANonDuplicate(t *testing.T) { + conf := "(path \"~/dl\")\n(rule \"all\" (move \"Out\"))\n" + e, _ := sandboxDir(t, conf, map[string]string{"a.pdf": "one"}) + tab := planTab(t, e) + if err := tab.KeepThisCopy(0); err == nil { + t.Error("a file that duplicates nothing was accepted") + } + if err := tab.KeepThisCopy(9); err == nil { + t.Error("a row that does not exist was accepted") + } +} diff --git a/gui/internal/ui/plan.go b/gui/internal/ui/plan.go index 8b6a394..0a9b718 100644 --- a/gui/internal/ui/plan.go +++ b/gui/internal/ui/plan.go @@ -64,6 +64,7 @@ type planView struct { sortFollowsPrefs bool startSelected bool menu *gtk.Popover + keep *gtk.Button menuRow int tab *model.PlanTab @@ -317,21 +318,61 @@ func (p *planView) confirmDeleteChecked() { d.Show() } +// confirmKeepThisCopy asks before one copy replaces another, naming both. +func (p *planView) confirmKeepThisCopy() { + if p.tab == nil || p.menuRow < 0 || p.menuRow >= len(p.tab.Rows) { + return + } + r := p.tab.Rows[p.menuRow] + if r.DuplicateOf == "" { + return + } + d := gtk.NewMessageDialog(&p.w.win.Window, gtk.DialogModal|gtk.DialogDestroyWithParent, + gtk.MessageQuestion, gtk.ButtonsNone) + d.SetObjectProperty("text", "Keep "+escape(r.Rel)+" and replace the other copy?") + d.SetObjectProperty("secondary-text", "This file takes the place of\n"+ + escape(xdg.Abbrev(r.DuplicateOf))+ + "\n\nThat copy goes to the Trash, and krino undo can bring it back. Nothing happens until you press Apply.") + d.AddButton("Cancel", int(gtk.ResponseCancel)) + d.AddButton("Keep this copy", int(gtk.ResponseAccept)) + d.ConnectResponse(func(response int) { + d.Destroy() + if response != int(gtk.ResponseAccept) { + return + } + if err := p.tab.KeepThisCopy(p.menuRow); err != nil { + p.w.setStatus("%v", err) + return + } + p.fillList() + p.showDetails(p.menuRow) + p.w.setStatus("%s will replace %s when you press Apply; nothing has moved yet", + escape(r.Rel), escape(xdg.Abbrev(r.DuplicateOf))) + }) + d.Show() +} + // newMenu builds the row menu: what to do with a file instead of what the // rules decided. func (p *planView) newMenu() *gtk.Popover { box := gtk.NewBox(gtk.OrientationVertical, 0) + pop := gtk.NewPopover() + pop.SetChild(box) + pop.SetParent(p.list) + pop.SetHasArrow(false) trash := gtk.NewButtonWithLabel("Trash instead") perm := gtk.NewButtonWithLabel("Delete permanently instead...") - for _, b := range []*gtk.Button{trash, perm} { + p.keep = gtk.NewButtonWithLabel("Keep this copy, replace the other...") + p.keep.SetTooltipText("put this file where the copy it duplicates is, and send that one to the Trash") + for _, b := range []*gtk.Button{trash, perm, p.keep} { b.SetHasFrame(false) b.SetHAlign(gtk.AlignFill) box.Append(b) } - pop := gtk.NewPopover() - pop.SetChild(box) - pop.SetParent(p.list) - pop.SetHasArrow(false) + p.keep.ConnectClicked(func() { + pop.Popdown() + p.confirmKeepThisCopy() + }) trash.ConnectClicked(func() { pop.Popdown() p.replace(plan.Trash) @@ -367,6 +408,7 @@ func (p *planView) onRightClick(x, y float64) { if p.menuRow < 0 { return } + p.keep.SetVisible(p.tab.Rows[p.menuRow].DuplicateOf != "") at := gdk.NewRectangle(int(x), int(y), 1, 1) p.menu.SetPointingTo(&at) p.menu.Popup() @@ -869,6 +911,12 @@ func (p *planView) showDetails(i int) { if what := describeFile(r); what != "" { p.details.Append(detailLine(what, "dim-label")) } + // Where the other copy is, in full: the reason names it relative to the + // directory when it is inside it, which reads as no place at all (his + // report, 2026-09-17). + if r.DuplicateOf != "" { + p.details.Append(detailLine("the same bytes as "+escape(xdg.Abbrev(r.DuplicateOf)), "dim-label")) + } for _, s := range r.Steps { row := gtk.NewBox(gtk.OrientationHorizontal, 8) diff --git a/internal/engine/facts.go b/internal/engine/facts.go index 641f1ef..ce05df4 100644 --- a/internal/engine/facts.go +++ b/internal/engine/facts.go @@ -158,6 +158,10 @@ type facts struct { // could not read), so (matched) is unknown while none has matched. undecided bool + // dupOriginal is the file a (duplicate) test last matched against, + // absolute, for a front end that offers to act on the other copy. + dupOriginal string + contentDone bool // extraction was attempted contentErr error // why it failed partialErr error // extract.ErrPartial: a keyword not found may be in the unread part @@ -290,9 +294,17 @@ func (f *facts) Duplicate(dirs []string) (string, bool, error) { if !isDup { return "", false, nil } + // The absolute path is kept for a front end that has to act on the + // other copy - the window offers to keep this one instead - while the + // reason text stays as it reads best (plan 21). + f.dupOriginal = orig return displayOriginal(orig, root), true, nil } +// DuplicateOriginal is the file the last (duplicate) test matched against, +// absolute; "" when none did. +func (f *facts) DuplicateOriginal() string { return f.dupOriginal } + // displayOriginal reports orig relative to root when it lies inside root, // else home-abbreviated (xdg.Abbrev), as every other user-visible path is. func displayOriginal(orig, root string) string { diff --git a/internal/engine/match.go b/internal/engine/match.go index 59cea42..25c05e5 100644 --- a/internal/engine/match.go +++ b/internal/engine/match.go @@ -46,6 +46,13 @@ type FileMatch struct { // directory's rules use, or that check failed. Only set for a file some // matching rule would delete. NoDelete string + + // DuplicateOf is the file a (duplicate) test matched this one against, + // absolute; "" when none did. The reason text says the same thing the + // way it reads best - relative to the directory when it is inside it - + // which leaves a front end no way to act on the other copy, or even to + // say where it is (his report, 2026-09-17). + DuplicateOf string } // Result is everything Match found in one directory. @@ -190,6 +197,7 @@ func evalFile(run *matchRun, file scan.File) FileMatch { if len(run.d.DupScopes) > 0 && deletes(fm.Rules) { fm.NoDelete = noDelete(f, run.d.DupScopes) } + fm.DuplicateOf = f.DuplicateOriginal() return fm } diff --git a/internal/engine/match_test.go b/internal/engine/match_test.go index 63bb54a..224f881 100644 --- a/internal/engine/match_test.go +++ b/internal/engine/match_test.go @@ -460,3 +460,37 @@ func TestRuleDuplicateWarningShortensThePath(t *testing.T) { t.Errorf("no shortened rule warning for a.txt: %+v %+v", r.Matched, r.Unmatched) } } + +// TestFileMatchNamesTheDuplicate: a file a (duplicate) test matched comes +// back with the other copy's absolute path, so a front end can say where it +// is and act on it - the reason text alone says "duplicate of NAME" for a +// copy inside the directory, which reads as no place at all (plan 21). +func TestFileMatchNamesTheDuplicate(t *testing.T) { + e, d, _ := fixture(t) + res, err := e.Match(context.Background(), d) + if err != nil { + t.Fatal(err) + } + found := false + for _, fm := range res.Matched { + if fm.File.Rel != "report (1).pdf" && fm.File.Rel != "report.pdf" { + continue + } + if fm.DuplicateOf == "" { + continue + } + found = true + if !filepath.IsAbs(fm.DuplicateOf) { + t.Errorf("%s: DuplicateOf = %q, want an absolute path", fm.File.Rel, fm.DuplicateOf) + } + if fm.DuplicateOf == fm.File.Path { + t.Errorf("%s: reported as a duplicate of itself", fm.File.Rel) + } + if _, err := os.Stat(fm.DuplicateOf); err != nil { + t.Errorf("%s: DuplicateOf does not exist: %v", fm.File.Rel, err) + } + } + if !found { + t.Fatal("neither copy came back with the file it duplicates") + } +} diff --git a/man/krino-gui.1 b/man/krino-gui.1 index 87c9860..10b6da6 100644 --- a/man/krino-gui.1 +++ b/man/krino-gui.1 @@ -65,7 +65,9 @@ once a plan has been applied, and size, age and rule can each be turned off under .Cm Settings . Selecting a row explains it beside the list: the name, how big and how old -the file is, and a line per step with its action, rule and reason. A file krino could not +the file is, the copy it duplicates when there is one - named in full, so +it is clear whether that copy is in this directory or elsewhere - and a +line per step with its action, rule and reason. A file krino could not decide about - unreadable content, a failed duplicate check - is listed with the reason and cannot be selected. .Pp @@ -86,7 +88,13 @@ and .Cm d keys do in the terminal review; .Cm With checked -does the same for every checked file at once. Either way the plan changes +does the same for every checked file at once. A file krino found another +copy of also offers +.Cm Keep this copy, replace the other : +the file takes the other's place and that copy goes to the Trash, which +.Cm krino undo +can still reverse. It is a decision made by hand, so the rule that no rule +may delete a duplicate does not apply to it. Either way the plan changes and nothing else: the files move when .Cm Apply is pressed, and closing the window with a plan still unapplied says so -- cgit v1.3