diff options
Diffstat (limited to 'gui')
| -rw-r--r-- | gui/internal/model/history.go | 9 | ||||
| -rw-r--r-- | gui/internal/model/plan.go | 25 | ||||
| -rw-r--r-- | gui/internal/model/plan_test.go | 43 | ||||
| -rw-r--r-- | gui/internal/ui/history.go | 20 | ||||
| -rw-r--r-- | gui/internal/ui/plan.go | 20 | ||||
| -rw-r--r-- | gui/internal/ui/window.go | 48 |
6 files changed, 157 insertions, 8 deletions
diff --git a/gui/internal/model/history.go b/gui/internal/model/history.go index 7c166bb..87ab6e7 100644 --- a/gui/internal/model/history.go +++ b/gui/internal/model/history.go @@ -114,6 +114,10 @@ type UndoTab struct { sess *engine.Session up *engine.UndoPlan locks []*lock.Lock + + // applying is set while the reversal is in flight; Close refuses then, + // for the reason PlanTab.applying gives. + applying bool } // PlanUndo builds the reversal of runID and locks every directory it @@ -208,6 +212,8 @@ func (t *UndoTab) SelectedCount() int { // said no to (spec §9). Refused files ride along unchanged, as they do on // the command line. The locks are released afterwards: the plan is history. func (t *UndoTab) Apply(ctx context.Context) (*engine.ApplyResult, error) { + t.applying = true + defer func() { t.applying = false }() toApply := &engine.UndoPlan{Run: t.up.Run, Cleanup: t.up.Cleanup} for i, f := range t.up.Files { if f.Refused == "" && !t.Rows[i].Selected { @@ -266,6 +272,9 @@ func undoOutcome(row UndoRow, fr engine.FileResult) string { // does. One lock's failure never stops the rest from being released, or the // session from being closed. Closing twice is not an error. func (t *UndoTab) Close() error { + if t.applying { + return ErrApplying + } var first error for _, l := range t.locks { if err := l.Release(); err != nil && first == nil { diff --git a/gui/internal/model/plan.go b/gui/internal/model/plan.go index ccbf17b..c3e9654 100644 --- a/gui/internal/model/plan.go +++ b/gui/internal/model/plan.go @@ -7,6 +7,7 @@ package model import ( "context" + "errors" "fmt" "sort" "time" @@ -50,8 +51,24 @@ type PlanTab struct { sess *engine.Session dp *engine.DirPlan lock *lock.Lock + + // applying is set while Apply is in flight. Close must refuse then: + // releasing the lock and closing the log under a running apply moves + // files the log never records, so undo cannot see them, and unlocks a + // directory krino is still working in. The window can reach Close from + // several places while an apply runs - saving rules or settings, adding + // a directory, closing the window - so the refusal lives here rather + // than in whichever of them remembers. + applying bool + + // testBeforeApply, when set, is called just before the engine's Apply + // begins, so a test can act while the apply is in flight. + testBeforeApply func() } +// ErrApplying is returned by Close while an apply is still running. +var ErrApplying = errors.New("krino is still applying this plan") + // Counts is the plan's summary line. type Counts struct { Scanned, Acting, Excluded, Skipped, Unmatched, Warned int @@ -95,6 +112,9 @@ func (t *PlanTab) Run() string { return t.sess.Run() } // Close releases the directory's lock and ends the run, which Apply also // does once the plan is history. Closing twice is not an error. func (t *PlanTab) Close() error { + if t.applying { + return ErrApplying + } if t.lock == nil { return nil } @@ -303,7 +323,12 @@ func (t *PlanTab) Apply(ctx context.Context) (*engine.ApplyResult, error) { approved[r.Rel] = true } } + t.applying = true + if t.testBeforeApply != nil { + t.testBeforeApply() + } res, err := t.sess.Apply(ctx, t.dp, approved) + t.applying = false t.Applied = true // An applied plan is history: the run is over and the directory free // again, without closing the window (GUI design §3). diff --git a/gui/internal/model/plan_test.go b/gui/internal/model/plan_test.go index a36cf85..1bae2c5 100644 --- a/gui/internal/model/plan_test.go +++ b/gui/internal/model/plan_test.go @@ -462,3 +462,46 @@ func TestKeepThisCopyRefusesANonDuplicate(t *testing.T) { t.Error("a row that does not exist was accepted") } } + +// TestCloseDuringApplyIsRefused: Close releases the directory lock and +// closes the log. Called while an apply is running - which the window +// allows: saving rules, saving settings, adding a directory and closing the +// window all reach it - the engine goes on moving files with the log shut +// under it, so a file is moved that no krino undo can see, and the +// directory is unlocked while krino is still working in it. +func TestCloseDuringApplyIsRefused(t *testing.T) { + conf := "(path \"~/dl\")\n(rule \"pdfs\" (when (type pdf)) (move \"Docs\"))\n" + e, _ := sandboxDir(t, conf, map[string]string{"a.pdf": "one", "b.pdf": "two"}) + tab, err := Plan(context.Background(), e, e.Dirs[0]) + if err != nil { + t.Fatal(err) + } + for i := range tab.Rows { + tab.Rows[i].Selected = true + } + + started := make(chan struct{}) + done := make(chan struct{}) + tab.testBeforeApply = func() { + close(started) + // Hold the apply open while Close is attempted. + <-done + } + var applyErr error + go func() { + _, applyErr = tab.Apply(context.Background()) + }() + <-started + + if err := tab.Close(); err == nil { + t.Error("Close during an apply was allowed: the lock and the log go out from under it") + } + close(done) + // Let the apply finish before the sandbox is torn down. + for i := 0; i < 200 && !tab.Applied; i++ { + time.Sleep(10 * time.Millisecond) + } + if applyErr != nil { + t.Errorf("the apply itself failed: %v", applyErr) + } +} diff --git a/gui/internal/ui/history.go b/gui/internal/ui/history.go index f82f6e3..3a3166e 100644 --- a/gui/internal/ui/history.go +++ b/gui/internal/ui/history.go @@ -4,6 +4,7 @@ package ui import ( "context" + "errors" "fmt" "strings" @@ -36,6 +37,10 @@ type historyView struct { undo *gtk.Button cancel *gtk.Button + // applying is set while a reversal is in flight; the window refuses + // anything that would take its lock or its log away. + applying bool + tab *model.UndoTab cancelOp context.CancelFunc } @@ -349,6 +354,8 @@ func (h *historyView) onUndo() { return } n := h.tab.SelectedCount() + h.applying = true + h.w.setApplying(true) h.setBusy(true) h.w.setStatus("reversing %d file(s)...", n) var res *engine.ApplyResult @@ -357,6 +364,8 @@ func (h *historyView) onUndo() { res, err = h.tab.Apply(ctx) return err }, func(err error) { + h.applying = false + h.w.setApplying(false) h.setBusy(false) h.fillPlan() h.undo.SetSensitive(false) @@ -404,6 +413,9 @@ func (h *historyView) updateUndoButton() { } // setBusy turns the buttons on or off around a background operation. +// busyApplying reports whether an undo is in flight. +func (h *historyView) busyApplying() bool { return h.applying } + func (h *historyView) setBusy(busy bool) { h.reload.SetSensitive(!busy) h.more.SetSensitive(!busy && len(h.runRows) >= h.limit) @@ -419,15 +431,19 @@ func (h *historyView) setBusy(busy bool) { } // closeTab drops the open undo plan and releases its locks. -func (h *historyView) closeTab() { +func (h *historyView) closeTab() bool { if h.tab == nil { - return + return true } if err := h.tab.Close(); err != nil { h.w.setStatus("undo: %v", err) + if errors.Is(err, model.ErrApplying) { + return false + } } h.tab = nil h.clearPlan() + return true } // clearList removes every row of a ListBox. diff --git a/gui/internal/ui/plan.go b/gui/internal/ui/plan.go index 1ee2a51..e59d4dc 100644 --- a/gui/internal/ui/plan.go +++ b/gui/internal/ui/plan.go @@ -4,6 +4,7 @@ package ui import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -63,6 +64,7 @@ type planView struct { previewOff bool sortFollowsPrefs bool startSelected bool + applying bool menu *gtk.Popover keep *gtk.Button menuRow int @@ -555,6 +557,8 @@ func (p *planView) onApply() { return } n := p.tab.SelectedCount() + p.applying = true + p.w.setApplying(true) p.setBusy(true) p.w.setStatus("applying %d file(s)...", n) var res *engine.ApplyResult @@ -563,6 +567,8 @@ func (p *planView) onApply() { res, err = p.tab.Apply(ctx) return err }, func(err error) { + p.applying = false + p.w.setApplying(false) p.setBusy(false) p.apply.SetSensitive(false) p.fillList() @@ -576,17 +582,27 @@ func (p *planView) onApply() { } // closeTab drops the open plan and releases the directory's lock. -func (p *planView) closeTab() { +func (p *planView) closeTab() bool { if p.tab == nil { - return + return true } if err := p.tab.Close(); err != nil { p.w.setStatus("%s: %v", escape(p.tab.Dir.Name), err) + if errors.Is(err, model.ErrApplying) { + // The lock and the log must stay put until the apply is done, + // or files move with nothing recording them. + return false + } } p.tab = nil p.fillList() + return true } +// busyApplying reports whether an apply is in flight, so the window can +// refuse anything that would take the lock or the log away from it. +func (p *planView) busyApplying() bool { return p.applying } + // setBusy turns the buttons on or off around a background operation. func (p *planView) setBusy(busy bool) { p.scan.SetSensitive(!busy) diff --git a/gui/internal/ui/window.go b/gui/internal/ui/window.go index 3d40441..a8a087e 100644 --- a/gui/internal/ui/window.go +++ b/gui/internal/ui/window.go @@ -34,6 +34,8 @@ type Window struct { plan *planView history *historyView rules *rulesView + notebook *gtk.Notebook + settings *gtk.Button status *gtk.Label prefs model.Prefs leaving bool @@ -50,6 +52,7 @@ func NewWindow(app *gtk.Application, e *engine.Engine) *Window { w.win.SetDefaultSize(1200, 720) notebook := gtk.NewNotebook() + w.notebook = notebook w.plan = newPlanView(w) notebook.AppendPage(w.plan.root, gtk.NewLabel("Plan")) w.history = newHistoryView(w) @@ -77,6 +80,7 @@ func NewWindow(app *gtk.Application, e *engine.Engine) *Window { settings.SetTooltipText("krino's defaults, and how this window behaves") settings.SetMarginEnd(6) settings.ConnectClicked(func() { w.showSettings() }) + w.settings = settings notebook.SetActionWidget(settings, gtk.PackEnd) w.status = gtk.NewLabel("") @@ -105,9 +109,12 @@ func NewWindow(app *gtk.Application, e *engine.Engine) *Window { w.confirmLeaving() return true } - w.plan.closeTab() + // An apply in flight keeps its lock and its log: closing the + // window under it would move files nothing records. + if !w.plan.closeTab() || !w.history.closeTab() { + return true + } w.plan.closePreview() - w.history.closeTab() return false }) themeColours(w.win) @@ -118,17 +125,50 @@ func NewWindow(app *gtk.Application, e *engine.Engine) *Window { // Show puts the window on screen. func (w *Window) Show() { w.win.Show() } +// setApplying greys out everything that would pull the directory lock or +// the log away from a running apply: the other tabs, and Settings. The +// model refuses those anyway, but a button that cannot work should say so +// by being unavailable rather than by an error afterwards. +func (w *Window) setApplying(busy bool) { + if w.settings != nil { + w.settings.SetSensitive(!busy) + } + if w.notebook == nil { + return + } + current := int(w.notebook.CurrentPage()) + for i := 0; i < int(w.notebook.NPages()); i++ { + if i == current { + continue + } + if page, ok := w.notebook.NthPage(i).(interface{ SetSensitive(bool) }); ok { + page.SetSensitive(!busy) + } + } +} + // reloadEngine re-reads the configuration, after the rules editor saves, so // every tab works from the rules the user just wrote. An open plan came // from the old ones, so it is closed and its lock released. +// +// It refuses while an apply is running. Saving rules, saving settings and +// adding a directory all come through here, and every one of them is +// reachable from the window while files are being moved; closing the plan +// then releases the directory lock and shuts the log under the engine, so a +// file moves that no krino undo can see and the rest of the plan is +// abandoned. func (w *Window) reloadEngine() error { + if w.plan.busyApplying() || w.history.busyApplying() { + return model.ErrApplying + } e, diags := engine.Load(w.engine.MainFile) if len(diags) > 0 { return diags[0] } e.CacheDir = w.engine.CacheDir - w.plan.closeTab() - w.history.closeTab() + if !w.plan.closeTab() || !w.history.closeTab() { + return model.ErrApplying + } w.engine = e return nil } |