Settings changes take effect, and keep this copy respects the plan

Five of the Settings window's controls - the sort order, the three
column toggles and the preview height - were read when it built its new
preferences but never connected to anything, so changing them did
nothing until some other control happened to fire, and then they all
landed at once out of nowhere. Every control is connected now.

The same closure composed a whole Prefs from its own widgets, which
wrote the remembered divider positions back as zeros: dragging the
panes to taste and then ticking any checkbox threw them away. A
settings change is now applied over the preferences as they are, by
model.Prefs.WithDisplay - which is where it can be tested, and is.

"Keep this copy, replace the other" wrote a Displaces straight into the
chain. internal/plan refuses to build two steps that displace one path,
because the second destroys what the first put there; the window went
round that code, so choosing it for two duplicates of one file left
both rows saying "done" with the first file in the Trash. It now
refuses the second, naming the file that has the place.

5 files changed, 135 insertions, 3 deletions

diff --git a/gui/internal/model/plan.go b/gui/internal/model/plan.go
index c3e9654..31a783b 100644
--- a/gui/internal/model/plan.go
+++ b/gui/internal/model/plan.go
@@ -16,6 +16,7 @@ import (
 	"git.labunix.xyz/krino/internal/lock"
 	"git.labunix.xyz/krino/internal/plan"
 	"git.labunix.xyz/krino/internal/scan"
+	"git.labunix.xyz/krino/internal/xdg"
 )
 
 // Row is one line of the Plan tab: a file krino would act on, or one it
@@ -292,6 +293,23 @@ func (t *PlanTab) KeepThisCopy(i int) error {
 	if row.Path == "" {
 		return fmt.Errorf("model: %s has no path", row.Rel)
 	}
+	// Only the first step to reach a path may displace it: once another
+	// step of this same plan is going to put its own file there, displacing
+	// it again would destroy that file. internal/plan refuses to build such
+	// a pair; the window writes its step by hand, so it must refuse too.
+	for _, c := range t.dp.Chains {
+		if c.File.Rel == row.Rel {
+			continue
+		}
+		for _, st := range c.Steps {
+			if st.Skip != "" {
+				continue
+			}
+			if st.Dst == row.DuplicateOf || st.Displaces == row.DuplicateOf {
+				return fmt.Errorf("model: %s is already spoken for by %s in this plan", xdg.Abbrev(row.DuplicateOf), c.File.Rel)
+			}
+		}
+	}
 	for j, c := range t.dp.Chains {
 		if c.File.Rel != row.Rel {
 			continue
diff --git a/gui/internal/model/plan_test.go b/gui/internal/model/plan_test.go
index 1bae2c5..241a049 100644
--- a/gui/internal/model/plan_test.go
+++ b/gui/internal/model/plan_test.go
@@ -505,3 +505,41 @@ func TestCloseDuringApplyIsRefused(t *testing.T) {
 		t.Errorf("the apply itself failed: %v", applyErr)
 	}
 }
+
+// TestKeepThisCopyTwiceIsRefused: "keep this copy" writes a Displaces
+// straight into the chain. The engine refuses to plan two steps that
+// displace one path - the second would destroy what the first just put
+// there - but the window went round that code. Both rows then reported
+// "done" while the first file was in the Trash.
+func TestKeepThisCopyTwiceIsRefused(t *testing.T) {
+	conf := "(path \"~/dl\")\n(rule \"dupes\" (when (duplicate)) (move \"Dupes\"))\n"
+	e, h := sandboxDir(t, conf, map[string]string{
+		"x.pdf": "the same bytes", "y.pdf": "the same bytes", "a.pdf": "the same bytes",
+	})
+	_ = h
+	tab, err := Plan(context.Background(), e, e.Dirs[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { tab.Close() })
+
+	var dupes []int
+	for i, r := range tab.Rows {
+		if r.DuplicateOf != "" {
+			dupes = append(dupes, i)
+		}
+	}
+	if len(dupes) < 2 {
+		t.Skipf("fixture produced %d duplicate rows, need two", len(dupes))
+	}
+	if err := tab.KeepThisCopy(dupes[0]); err != nil {
+		t.Fatalf("the first choice was refused: %v", err)
+	}
+	err = tab.KeepThisCopy(dupes[1])
+	if err == nil {
+		t.Fatal("two files were allowed to replace the same one; the second would trash what the first filed")
+	}
+	if !strings.Contains(err.Error(), "already") {
+		t.Errorf("refusal reads %q; it should say the place is already spoken for", err)
+	}
+}
diff --git a/gui/internal/model/prefs.go b/gui/internal/model/prefs.go
index f5ffee5..9ae79a0 100644
--- a/gui/internal/model/prefs.go
+++ b/gui/internal/model/prefs.go
@@ -112,3 +112,41 @@ func (p Prefs) Save() error {
 	}
 	return os.WriteFile(file, append(data, '\n'), 0o644)
 }
+
+// Display is everything the Settings window can change about the window
+// itself. It deliberately leaves out the divider positions: those are set
+// by dragging, and a settings change must not disturb them. Composing a
+// whole Prefs in the Settings window instead is what used to write
+// PreviewWidth, ListWidth and ListHeight back as zeros, so dragging the
+// panes to taste and then ticking any checkbox threw the panes away.
+type Display struct {
+	Sort          string
+	Layout        string
+	ShowSize      bool
+	ShowAge       bool
+	ShowRule      bool
+	Colours       bool
+	Preview       bool
+	SelectAll     bool
+	PreviewHeight int
+}
+
+// DisplayOf is the part of p the Settings window shows.
+func (p Prefs) DisplayOf() Display {
+	return Display{
+		Sort: p.Sort, Layout: p.Layout,
+		ShowSize: p.ShowSize, ShowAge: p.ShowAge, ShowRule: p.ShowRule,
+		Colours: p.Colours, Preview: p.Preview, SelectAll: p.SelectAll,
+		PreviewHeight: p.PreviewHeight,
+	}
+}
+
+// WithDisplay is p with d applied and everything else - where each divider
+// was left - kept as it was.
+func (p Prefs) WithDisplay(d Display) Prefs {
+	p.Sort, p.Layout = d.Sort, d.Layout
+	p.ShowSize, p.ShowAge, p.ShowRule = d.ShowSize, d.ShowAge, d.ShowRule
+	p.Colours, p.Preview, p.SelectAll = d.Colours, d.Preview, d.SelectAll
+	p.PreviewHeight = d.PreviewHeight
+	return p
+}
diff --git a/gui/internal/model/prefs_test.go b/gui/internal/model/prefs_test.go
index 328ac31..69cc678 100644
--- a/gui/internal/model/prefs_test.go
+++ b/gui/internal/model/prefs_test.go
@@ -57,3 +57,30 @@ func TestPrefsWithoutAFile(t *testing.T) {
 		t.Errorf("with damaged text: %+v, want the defaults", got)
 	}
 }
+
+// TestWithDisplayKeepsTheDividers: the Settings window used to compose a
+// whole Prefs from its own controls, which wrote the remembered pane
+// positions back as zeros - so dragging the dividers and then ticking any
+// checkbox in Settings threw the positions away and the next window opened
+// at the defaults.
+func TestWithDisplayKeepsTheDividers(t *testing.T) {
+	p := Prefs{
+		PreviewWidth: 640, ListWidth: 800, ListHeight: 300, PreviewHeight: 281,
+		Sort: SortName, ShowSize: true, ShowAge: true, ShowRule: true,
+		Colours: true, Preview: true, SelectAll: true, Layout: LayoutSide,
+	}
+	d := p.DisplayOf()
+	d.ShowRule = false
+	d.Sort = SortSize
+	got := p.WithDisplay(d)
+
+	if got.PreviewWidth != 640 || got.ListWidth != 800 || got.ListHeight != 300 {
+		t.Errorf("the dividers were disturbed: %+v", got)
+	}
+	if got.ShowRule || got.Sort != SortSize {
+		t.Errorf("the change did not take: %+v", got)
+	}
+	if !got.ShowSize || !got.ShowAge || !got.Colours || !got.Preview || !got.SelectAll {
+		t.Errorf("something else changed: %+v", got)
+	}
+}
diff --git a/gui/internal/ui/settings.go b/gui/internal/ui/settings.go
index 32a6066..361f415 100644
--- a/gui/internal/ui/settings.go
+++ b/gui/internal/ui/settings.go
@@ -137,6 +137,9 @@ func (w *Window) showSettings() {
 		box.Append(l)
 	}
 
+	// Everything this window can change, applied over the preferences as
+	// they are: the divider positions belong to the panes, not here, and
+	// composing a whole Prefs from these controls wrote them back as zeros.
 	apply := func() {
 		which := model.LayoutSide
 		if layout.Selected() == 1 {
@@ -146,8 +149,9 @@ func (w *Window) showSettings() {
 		if i := int(sortOrder.Selected()); i >= 0 && i < len(model.SortOrders) {
 			order = model.SortOrders[i]
 		}
-		p := model.Prefs{
+		p := w.prefs.WithDisplay(model.Display{
 			Sort:          order,
+			Layout:        which,
 			ShowSize:      showSize.Active(),
 			ShowAge:       showAge.Active(),
 			ShowRule:      showRule.Active(),
@@ -155,17 +159,24 @@ func (w *Window) showSettings() {
 			Preview:       preview.Active(),
 			SelectAll:     selectAll.Selected() == 0,
 			PreviewHeight: int(previewHeight.Value()),
-			Layout:        which,
-		}
+		})
 		w.applyPrefs(p)
 		if err := p.Save(); err != nil {
 			w.setStatus("settings: %v", err)
 		}
 	}
+	// Every control, not four of them: sort, the three column toggles and
+	// the preview height were read by apply but never connected to it, so
+	// changing them did nothing until some other control happened to fire.
 	colours.ConnectToggled(apply)
 	preview.ConnectToggled(apply)
+	showSize.ConnectToggled(apply)
+	showAge.ConnectToggled(apply)
+	showRule.ConnectToggled(apply)
 	selectAll.Connect("notify::selected", func() { apply() })
 	layout.Connect("notify::selected", func() { apply() })
+	sortOrder.Connect("notify::selected", func() { apply() })
+	previewHeight.ConnectValueChanged(apply)
 
 	s.save = gtk.NewButtonWithLabel("Save krino.conf")
 	s.save.AddCSSClass("suggested-action")