From ca2a8c8aa736e8828226dc2b433d5c4beab59942 Mon Sep 17 00:00:00 2001 From: Mikhail Yenuchenko Date: Thu, 6 Aug 2026 22:15:19 +0300 Subject: [PATCH] fix: accept zero retention limits, retire Store() for typed accessors Phase 8 (PROJECT_REVIEW_PLAN.md 8.1): 0 in MaxLogFiles/MaxLogAgeDays now means "keep everything" end to end. runner.CleanupLogs already treated <= 0 as disabled; validateConfig, the Settings form, and loadOrCreateConfig's backfill were the only things making that state unreachable. Phase 9 (1.1, rolling up 1.2, 1.3, 7.3): added Service.Config() and Service.Paths(), copying under mu, and converted every UI site that read Service state through the raw *storage.Store returned by Store() (now removed). jobs_view's pause control is now driven by refreshView reading svc.Config().Paused on every event instead of only mirroring its own tap handler, which makes it an actual consumer of SchedulerStateChanged. mainwindow's event listener is a real type switch, and events.go's doc comment no longer claims a compiler exhaustiveness check Go doesn't have. Unexported the redundant SetAutostart/AutostartStatus package functions in platform/autostart now that only the Manager methods are used outside the package. Co-Authored-By: Claude Sonnet 5 --- docs/CHANGELOG.md | 5 ++ docs/STANDARDS.md | 8 +++ src/app/events.go | 9 ++- src/app/operations.go | 10 +-- src/app/operations_test.go | 29 +++++--- src/app/service.go | 26 +++++-- src/platform/autostart/autostart_linux.go | 8 +-- .../autostart/autostart_linux_test.go | 4 +- src/platform/autostart/autostart_other.go | 8 +-- src/platform/autostart/autostart_windows.go | 10 +-- .../autostart/autostart_windows_test.go | 2 +- src/runner/cleanup.go | 5 ++ src/storage/store.go | 12 ++-- src/storage/store_test.go | 29 ++++++++ src/ui/jobs_view.go | 67 +++++++++++-------- src/ui/mainwindow.go | 35 +++++----- src/ui/run.go | 5 +- src/ui/settings_view.go | 66 +++++++++++------- 18 files changed, 223 insertions(+), 115 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index a4d0f3b..1be7786 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -51,6 +51,11 @@ the app icon (experimental).** gets slower the longer the app has been running. Measured on 5000 accumulated records, one History redraw went from **15.8 ms to 0.9 ms**; at the new cap the width rescan alone accounted for 1.5 ms of every redraw. +- **Max log files and max log age days now accept 0, meaning "keep + everything."** Log cleanup already supported disabling either policy; the + Settings form and the Service validator rejected the value that would have + turned it on. A config that already set either to 0 is no longer silently + rewritten back to the 100/30 defaults on load. **Jobs:** diff --git a/docs/STANDARDS.md b/docs/STANDARDS.md index a7ee553..cfb072e 100644 --- a/docs/STANDARDS.md +++ b/docs/STANDARDS.md @@ -71,6 +71,14 @@ change to their shape has to stay compatible on its own. = 0) and is overridable per job (`Job.TimeoutSeconds *int`: unset = inherit the global default, 0 = no timeout, positive = seconds). Neither zero may be normalized away on load — 0 is a value, not a missing field. +- **`Config.MaxLogFiles` and `Config.MaxLogAgeDays` of 0 mean "keep everything", + not "unset".** `runner.CleanupLogs` already treated `<= 0` as "policy + disabled"; `app.validateConfig` and the Settings form now accept 0 (only a + negative count is rejected), and `storage.loadOrCreateConfig` no longer + backfills 0 to 100 / 30 — a config written before either field existed still + picks up the default because `json.Unmarshal` leaves an absent key holding + whatever `DefaultConfig()` set, the same mechanism `DefaultTimeoutSeconds` + relies on. - **A `StartOnly` process is expected to outlive GoSentry.** The option exists to launch something and let go of it, so the runner builds that invocation on `context.Background()`, not on the application's lifecycle context: quitting diff --git a/src/app/events.go b/src/app/events.go index d21ab88..352c573 100644 --- a/src/app/events.go +++ b/src/app/events.go @@ -4,9 +4,12 @@ import "gitea.mixdep.ru/mix/gosentry/src/domain" // Event is something the Service did to its state that observers may want to // react to. It is a sealed interface: the concrete types in this file are the -// only implementations (enforced by the unexported isEvent marker), so a UI -// listener can exhaustively type-switch over them and the compiler will flag a -// new event type that a switch forgot to handle. +// only implementations (enforced by the unexported isEvent marker), so an +// Event handed to an Observer is always one of the types declared here — a +// caller outside this package cannot manufacture a new one. Go's type switch +// has no exhaustiveness check, so sealing buys that guarantee, not a +// compile-time warning when a new event type is added and a listener forgets +// to handle it; the listener still has to be updated by hand. // // Events replace the old single onChange callback. Instead of the scheduler // reaching into the GUI, the Service emits typed events and the UI subscribes — diff --git a/src/app/operations.go b/src/app/operations.go index 091f27c..006b70c 100644 --- a/src/app/operations.go +++ b/src/app/operations.go @@ -503,11 +503,13 @@ func validateConfig(config domain.Config) error { if strings.TrimSpace(config.LogsDir) == "" { return errors.New("logs directory is required") } - if config.MaxLogFiles <= 0 { - return errors.New("max log files must be a positive number") + // 0 means "keep everything" (see runner.CleanupLogs); only a negative count + // is rejected, the same three-state shape as DefaultTimeoutSeconds below. + if config.MaxLogFiles < 0 { + return errors.New("max log files must be zero (unlimited) or a positive number") } - if config.MaxLogAgeDays <= 0 { - return errors.New("max log age days must be a positive number") + if config.MaxLogAgeDays < 0 { + return errors.New("max log age days must be zero (unlimited) or a positive number") } if config.ExecutionMode != domain.ExecutionModeParallel && config.ExecutionMode != domain.ExecutionModeSequential { return errors.New("execution mode must be 'parallel' or 'sequential'") diff --git a/src/app/operations_test.go b/src/app/operations_test.go index 63ab45f..0237703 100644 --- a/src/app/operations_test.go +++ b/src/app/operations_test.go @@ -522,9 +522,9 @@ func TestUpdateSettingsPersistsAndValidates(t *testing.T) { svc := newTempService(t, nil) bad := svc.store.Config - bad.MaxLogFiles = 0 + bad.MaxLogFiles = -1 if err := svc.UpdateSettings(bad); err == nil { - t.Error("expected validation error for non-positive max log files") + t.Error("expected validation error for negative max log files") } good := svc.store.Config @@ -533,8 +533,21 @@ func TestUpdateSettingsPersistsAndValidates(t *testing.T) { if err := svc.UpdateSettings(good); err != nil { t.Fatalf("UpdateSettings: %v", err) } - if svc.Store().Config.MaxLogAgeDays != 7 || svc.Store().Config.NotifyOnFailure { - t.Errorf("config not applied: %+v", svc.Store().Config) + if svc.store.Config.MaxLogAgeDays != 7 || svc.store.Config.NotifyOnFailure { + t.Errorf("config not applied: %+v", svc.store.Config) + } + + // 0 means "keep everything" (see STANDARDS §Intentional behavior), not an + // invalid value, so it must be accepted and persisted rather than rejected + // or silently backfilled. + unlimited := svc.store.Config + unlimited.MaxLogFiles = 0 + unlimited.MaxLogAgeDays = 0 + if err := svc.UpdateSettings(unlimited); err != nil { + t.Fatalf("UpdateSettings with zero retention limits: %v", err) + } + if svc.store.Config.MaxLogFiles != 0 || svc.store.Config.MaxLogAgeDays != 0 { + t.Errorf("zero retention limits not preserved: %+v", svc.store.Config) } } @@ -549,8 +562,8 @@ func TestUpdateSettingsRejectsInvalidConfigs(t *testing.T) { {"missing jobs file", func(c *domain.Config) { c.JobsFile = " " }}, {"jobs file without a file name", func(c *domain.Config) { c.JobsFile = "jobs" + string(filepath.Separator) }}, {"missing logs dir", func(c *domain.Config) { c.LogsDir = "" }}, - {"non-positive max files", func(c *domain.Config) { c.MaxLogFiles = 0 }}, - {"non-positive max age", func(c *domain.Config) { c.MaxLogAgeDays = -1 }}, + {"negative max files", func(c *domain.Config) { c.MaxLogFiles = -1 }}, + {"negative max age", func(c *domain.Config) { c.MaxLogAgeDays = -1 }}, {"negative default timeout", func(c *domain.Config) { c.DefaultTimeoutSeconds = -1 }}, } for _, tc := range tests { @@ -712,12 +725,12 @@ func TestUpdateSettingsRefusesJobsFileSwitchWhileRunning(t *testing.T) { if err := svc.UpdateSettings(config); err == nil { t.Error("expected the jobs-file switch to be refused while a job is running") } - if svc.Store().Config.JobsFile == config.JobsFile { + if svc.store.Config.JobsFile == config.JobsFile { t.Error("the refused switch must not have been persisted") } // A setting that does not touch the jobs file still saves during a run. - unrelated := svc.Store().Config + unrelated := svc.store.Config unrelated.NotifyOnFailure = !unrelated.NotifyOnFailure if err := svc.UpdateSettings(unrelated); err != nil { t.Errorf("unrelated setting should still save during a run: %v", err) diff --git a/src/app/service.go b/src/app/service.go index 7d56f8f..2da02c3 100644 --- a/src/app/service.go +++ b/src/app/service.go @@ -204,11 +204,27 @@ func Open() (*Service, error) { return svc, nil } -// Store returns the underlying store. It is exposed so callers that still need -// resolved paths and config (the GUI, during the transition) can reach them; -// later phases narrow this surface. -func (s *Service) Store() *storage.Store { - return s.store +// Config returns a copy of the current application configuration, safe to +// call from any goroutine. UpdateSettings, SetGlobalPause, and SetJobListView +// are the only writers and all mutate store.Config under mu; copying under the +// same lock is what keeps a UI read from racing them, instead of holding onto +// the *storage.Store this used to hand out (see STANDARDS: the UI reads +// Service state through typed events and accessors, never shared mutable +// state). +func (s *Service) Config() domain.Config { + s.mu.Lock() + defer s.mu.Unlock() + return s.store.Config +} + +// Paths returns a copy of the store's resolved filesystem paths. AppDir and +// ConfigPath are fixed for the process; JobsPath, JobsDir, and LogsDir are +// re-derived under mu on every settings save (storage.Store.applyConfigPaths), +// so this copies under the same lock as Config for the same reason. +func (s *Service) Paths() storage.Paths { + s.mu.Lock() + defer s.mu.Unlock() + return s.store.Paths } // Jobs returns a copy of the durable jobs slice. Returning a copy keeps callers diff --git a/src/platform/autostart/autostart_linux.go b/src/platform/autostart/autostart_linux.go index 428d29b..6baa258 100644 --- a/src/platform/autostart/autostart_linux.go +++ b/src/platform/autostart/autostart_linux.go @@ -18,16 +18,16 @@ type linuxManager struct{} func New() Manager { return linuxManager{} } func (linuxManager) Set(enabled, startInTray bool, executablePath, iconPath string) error { - return SetAutostart(enabled, startInTray, executablePath, iconPath) + return setAutostart(enabled, startInTray, executablePath, iconPath) } func (linuxManager) Status(expectedEnabled, startInTray bool, executablePath string) (bool, string) { - return AutostartStatus(expectedEnabled, startInTray, executablePath) + return autostartStatus(expectedEnabled, startInTray, executablePath) } const autostartDesktopFileName = "gosentry.desktop" -func SetAutostart(enabled bool, startInTray bool, executablePath string, iconPath string) error { +func setAutostart(enabled bool, startInTray bool, executablePath string, iconPath string) error { desktopPath, err := autostartDesktopPath() if err != nil { return err @@ -58,7 +58,7 @@ X-GNOME-Autostart-enabled=true return nil } -func AutostartStatus(expectedEnabled bool, startInTray bool, executablePath string) (bool, string) { +func autostartStatus(expectedEnabled bool, startInTray bool, executablePath string) (bool, string) { desktopPath, err := autostartDesktopPath() if err != nil { return false, "Cannot resolve XDG autostart directory" diff --git a/src/platform/autostart/autostart_linux_test.go b/src/platform/autostart/autostart_linux_test.go index b629500..ed9f068 100644 --- a/src/platform/autostart/autostart_linux_test.go +++ b/src/platform/autostart/autostart_linux_test.go @@ -14,7 +14,7 @@ func TestLinuxAutostartStartsInTray(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) executablePath := "/opt/Go Sentry/gosentry" - if err := SetAutostart(true, true, executablePath, "/opt/Go Sentry/gosentry.png"); err != nil { + if err := setAutostart(true, true, executablePath, "/opt/Go Sentry/gosentry.png"); err != nil { t.Fatalf("enable autostart: %v", err) } @@ -37,7 +37,7 @@ func TestLinuxAutostartWithoutTrayFlag(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) executablePath := "/opt/Go Sentry/gosentry" - if err := SetAutostart(true, false, executablePath, ""); err != nil { + if err := setAutostart(true, false, executablePath, ""); err != nil { t.Fatalf("enable autostart: %v", err) } diff --git a/src/platform/autostart/autostart_other.go b/src/platform/autostart/autostart_other.go index 8f99fb7..8d5ed1b 100644 --- a/src/platform/autostart/autostart_other.go +++ b/src/platform/autostart/autostart_other.go @@ -10,21 +10,21 @@ type otherManager struct{} func New() Manager { return otherManager{} } func (otherManager) Set(enabled, startInTray bool, executablePath, iconPath string) error { - return SetAutostart(enabled, startInTray, executablePath, iconPath) + return setAutostart(enabled, startInTray, executablePath, iconPath) } func (otherManager) Status(expectedEnabled, startInTray bool, executablePath string) (bool, string) { - return AutostartStatus(expectedEnabled, startInTray, executablePath) + return autostartStatus(expectedEnabled, startInTray, executablePath) } -func SetAutostart(enabled bool, startInTray bool, executablePath string, iconPath string) error { +func setAutostart(enabled bool, startInTray bool, executablePath string, iconPath string) error { if !enabled { return nil } return fmt.Errorf("autostart is not implemented for this platform") } -func AutostartStatus(expectedEnabled bool, startInTray bool, executablePath string) (bool, string) { +func autostartStatus(expectedEnabled bool, startInTray bool, executablePath string) (bool, string) { if !expectedEnabled { return true, "Autostart is off" } diff --git a/src/platform/autostart/autostart_windows.go b/src/platform/autostart/autostart_windows.go index 1adbc2c..8e5bae3 100644 --- a/src/platform/autostart/autostart_windows.go +++ b/src/platform/autostart/autostart_windows.go @@ -17,17 +17,17 @@ type windowsManager struct{} func New() Manager { return windowsManager{} } func (windowsManager) Set(enabled, startInTray bool, executablePath, iconPath string) error { - return SetAutostart(enabled, startInTray, executablePath, iconPath) + return setAutostart(enabled, startInTray, executablePath, iconPath) } func (windowsManager) Status(expectedEnabled, startInTray bool, executablePath string) (bool, string) { - return AutostartStatus(expectedEnabled, startInTray, executablePath) + return autostartStatus(expectedEnabled, startInTray, executablePath) } const autostartName = "GoSentry" const startupShortcutFile = autostartName + ".lnk" -func SetAutostart(enabled bool, startInTray bool, executablePath string, iconPath string) error { +func setAutostart(enabled bool, startInTray bool, executablePath string, iconPath string) error { // Windows autostart used to write HKCU\Run values, but that approach became // brittle once paths with spaces and the "--start-in-tray" argument entered // the picture. A Startup-folder shortcut stores target path and arguments as @@ -44,7 +44,7 @@ func SetAutostart(enabled bool, startInTray bool, executablePath string, iconPat return removeIfExists(shortcutPath) } -func AutostartStatus(expectedEnabled bool, startInTray bool, executablePath string) (bool, string) { +func autostartStatus(expectedEnabled bool, startInTray bool, executablePath string) (bool, string) { shortcutPath, err := startupShortcutPath() if err != nil { return false, "Startup folder cannot be resolved" @@ -126,7 +126,7 @@ func readShortcut(shortcutPath string) (string, string, error) { // OEM code page (e.g. CP866 on Russian Windows). Without this override, // [Console]::Out.Write encodes Cyrillic and other non-ASCII characters as // OEM bytes; Go then reads them as UTF-8 and gets a different string from - // os.Executable, causing AutostartStatus to report "shortcut points to + // os.Executable, causing autostartStatus to report "shortcut points to // another executable" for any install path that contains non-ASCII chars. // New-Object System.Text.UTF8Encoding($false) is UTF-8 without BOM. script := `[Console]::OutputEncoding = New-Object System.Text.UTF8Encoding($false); $shell = New-Object -ComObject WScript.Shell; $shortcut = $shell.CreateShortcut($env:GOSENTRY_SHORTCUT_PATH); [Console]::Out.Write($shortcut.TargetPath + [Environment]::NewLine + $shortcut.Arguments)` diff --git a/src/platform/autostart/autostart_windows_test.go b/src/platform/autostart/autostart_windows_test.go index a4c7a8e..15811ff 100644 --- a/src/platform/autostart/autostart_windows_test.go +++ b/src/platform/autostart/autostart_windows_test.go @@ -134,7 +134,7 @@ func TestAutostartStatusRequiresMatchingTrayFlag(t *testing.T) { t.Fatalf("create shortcut: %v", err) } - ok, message := AutostartStatus(true, false, targetPath) + ok, message := autostartStatus(true, false, targetPath) if ok { t.Fatalf("expected problem when tray flag mismatches, got OK: %s", message) } diff --git a/src/runner/cleanup.go b/src/runner/cleanup.go index 04571f7..d839ccd 100644 --- a/src/runner/cleanup.go +++ b/src/runner/cleanup.go @@ -9,6 +9,11 @@ import ( "time" ) +// CleanupLogs enforces the count and age retention policies on the .log files +// in logsDir. maxFiles <= 0 disables the count policy and maxAgeDays <= 0 +// disables the age policy, independently — "keep everything" is a value the +// user can choose in Settings, not just an internal default (STANDARDS +// §Intentional behavior). func CleanupLogs(logsDir string, maxFiles int, maxAgeDays int) error { entries, err := os.ReadDir(logsDir) if err != nil { diff --git a/src/storage/store.go b/src/storage/store.go index a8f9891..eecb14a 100644 --- a/src/storage/store.go +++ b/src/storage/store.go @@ -144,12 +144,12 @@ func loadOrCreateConfig(paths Paths) (domain.Config, error) { if strings.TrimSpace(config.LogsDir) == "" { config.LogsDir = "logs" } - if config.MaxLogFiles <= 0 { - config.MaxLogFiles = 100 - } - if config.MaxLogAgeDays <= 0 { - config.MaxLogAgeDays = 30 - } + // MaxLogFiles and MaxLogAgeDays are deliberately not normalized: 0 means + // "keep everything" (see runner.CleanupLogs), not a missing value, so + // backfilling it here would make that choice impossible to persist. A config + // written before either field existed already carries 0 from json.Unmarshal + // leaving the DefaultConfig() value in config untouched, so old files still + // pick up 100 / 30 without an explicit backfill. if config.ExecutionMode == "" { config.ExecutionMode = domain.ExecutionModeParallel } diff --git a/src/storage/store_test.go b/src/storage/store_test.go index 07ddcae..231987b 100644 --- a/src/storage/store_test.go +++ b/src/storage/store_test.go @@ -183,6 +183,35 @@ func TestLoadOrCreateConfigCreatesDefaultsOnFirstRun(t *testing.T) { } } +// TestLoadOrCreateConfigPreservesZeroRetentionLimits verifies that 0 in +// max_log_files / max_log_age_days is read back as 0 ("keep everything"), not +// backfilled to the 100 / 30 defaults, since a config that already has the +// field set is not the "field is missing" case loadOrCreateConfig backfills. +func TestLoadOrCreateConfigPreservesZeroRetentionLimits(t *testing.T) { + dir := t.TempDir() + paths := Paths{ + AppDir: dir, + ConfigPath: filepath.Join(dir, ConfigFileName), + } + want := domain.DefaultConfig() + want.MaxLogFiles = 0 + want.MaxLogAgeDays = 0 + if err := writeJSON(paths.ConfigPath, want); err != nil { + t.Fatal(err) + } + + got, err := loadOrCreateConfig(paths) + if err != nil { + t.Fatal(err) + } + if got.MaxLogFiles != 0 { + t.Errorf("MaxLogFiles: got %d, want 0 (unlimited)", got.MaxLogFiles) + } + if got.MaxLogAgeDays != 0 { + t.Errorf("MaxLogAgeDays: got %d, want 0 (unlimited)", got.MaxLogAgeDays) + } +} + // TestLoadOrCreateJobsSeedsSampleJobsOnFirstRun verifies that a missing // jobs.json is created with the sample jobs from defaultJobs, so a new user // sees scheduled and manual execution without inventing a command. diff --git a/src/ui/jobs_view.go b/src/ui/jobs_view.go index 949f018..f309694 100644 --- a/src/ui/jobs_view.go +++ b/src/ui/jobs_view.go @@ -57,13 +57,14 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) { selected = -1 } selectedFolder := allFolders - schedulerPaused := svc.Store().Config.Paused - listView := svc.Store().Config.JobListView + initialConfig := svc.Config() + schedulerPaused := initialConfig.Paused + listView := initialConfig.JobListView filteredJobs := filteredJobIndexes(jobs, selectedFolder) - dp := newDetailsPanel(job{}, &domain.JobRuntime{}, svc.Store().Config.OverlapPolicy, svc.Store().Config.DefaultTimeoutSeconds) + dp := newDetailsPanel(job{}, &domain.JobRuntime{}, initialConfig.OverlapPolicy, initialConfig.DefaultTimeoutSeconds) if selected >= 0 { - dp.update(jobs[selected], runtimeFor(selected), svc.Store().Config.OverlapPolicy, svc.Store().Config.DefaultTimeoutSeconds) + dp.update(jobs[selected], runtimeFor(selected), initialConfig.OverlapPolicy, initialConfig.DefaultTimeoutSeconds) } else { dp.clear() } @@ -76,16 +77,28 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) { return } selected = index - dp.update(jobs[selected], runtimeFor(selected), svc.Store().Config.OverlapPolicy, svc.Store().Config.DefaultTimeoutSeconds) + // Overlap policy and the default timeout are global settings that can + // change from the Settings tab while this view is open, so they are + // re-read on every update rather than captured once at construction. + config := svc.Config() + dp.update(jobs[selected], runtimeFor(selected), config.OverlapPolicy, config.DefaultTimeoutSeconds) } - // list and folderSelect are declared early so closures below can reference - // them before the widget.NewList / widget.NewSelect calls assign the values. + // list, folderSelect, and applySchedulerState are declared early so closures + // below can reference them before the widgets that assign the values exist. var list *widget.List var folderSelect *widget.Select + var applySchedulerState func(bool) refreshView := func() { syncFromService() + if applySchedulerState != nil { + // The pause state is Service-owned and can change from outside this + // view (Settings has no such control today, but the event that + // reports it — SchedulerStateChanged — is consumed here rather than + // relying solely on the tap handler's own mirror). + applySchedulerState(svc.Config().Paused) + } filteredJobs = filteredJobIndexes(jobs, selectedFolder) updateDetails(selected) dp.logs.Refresh() @@ -258,26 +271,15 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) { refreshView() }) - stopAllText, stopAllIcon := "Disable auto", theme.MediaPauseIcon() - if schedulerPaused { - stopAllText, stopAllIcon = "Enable auto", theme.MediaPlayIcon() - } - schedulerStateText := "Scheduler running" - if schedulerPaused { - schedulerStateText = "Scheduler paused" - } - schedulerState := widget.NewLabel(schedulerStateText) - stopAllButton := widget.NewButtonWithIcon(stopAllText, stopAllIcon, nil) - stopAllButton.OnTapped = func() { - // SetGlobalPause flips the pause flag, updates every job's next-run text, - // and emits the activity record the observer logs. Revert if the save fails. - schedulerPaused = !schedulerPaused - if err := svc.SetGlobalPause(schedulerPaused); err != nil { - schedulerPaused = !schedulerPaused - dialog.ShowError(err, w) - return - } - if schedulerPaused { + schedulerState := widget.NewLabel("") + stopAllButton := widget.NewButtonWithIcon("", nil, nil) + // applySchedulerState is the one place that draws the pause control and its + // status text from a pause value, so refreshView can drive it from whatever + // the Service reports (including a SchedulerStateChanged the general refresh + // picks up) instead of only the tap handler mirroring its own toggle. + applySchedulerState = func(paused bool) { + schedulerPaused = paused + if paused { schedulerState.SetText("Scheduler paused") stopAllButton.SetText("Enable auto") stopAllButton.SetIcon(theme.MediaPlayIcon()) @@ -286,6 +288,17 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) { stopAllButton.SetText("Disable auto") stopAllButton.SetIcon(theme.MediaPauseIcon()) } + } + applySchedulerState(schedulerPaused) + stopAllButton.OnTapped = func() { + // SetGlobalPause flips the pause flag, updates every job's next-run text, + // and emits the activity record the observer logs. Revert if the save fails. + if err := svc.SetGlobalPause(!schedulerPaused); err != nil { + dialog.ShowError(err, w) + return + } + // refreshView re-derives the pause state from the Service (see + // applySchedulerState above), so it is the single place that draws it. refreshView() } pauseButton := widget.NewButtonWithIcon("Pause", theme.MediaPauseIcon(), func() { diff --git a/src/ui/mainwindow.go b/src/ui/mainwindow.go index 62311e7..e4c6a62 100644 --- a/src/ui/mainwindow.go +++ b/src/ui/mainwindow.go @@ -65,43 +65,42 @@ func newMainView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func(time. // the main thread in both cases, so the engine never mutates Fyne state off // the UI thread. This is the sole place events touch widgets. (Resolves #4.) svc.Subscribe(app.ObserverFunc(func(ev app.Event) { - recorded, isRecorded := ev.(app.RunRecorded) - errOccurred, isError := ev.(app.ErrorOccurred) - jobsLoaded, isJobsLoaded := ev.(app.JobsLoaded) fyne.Do(func() { - if isRecorded { - events.add(recorded.Record) - r := recorded.Record - if r.State == "Failed" && - (r.Trigger == "Manual" || r.Trigger == "Schedule") && + // A type switch does not get compiler-enforced exhaustiveness (see + // app.Event's doc comment) — JobChanged and SchedulerStateChanged + // intentionally fall through to the unconditional refresh() below + // without their own case, since a broad state re-read is all they need. + switch e := ev.(type) { + case app.RunRecorded: + events.add(e.Record) + if e.Record.State == "Failed" && + (e.Record.Trigger == "Manual" || e.Record.Trigger == "Schedule") && svc.ShouldNotifyOnFailure() { timing := notificationTiming{ - JobName: r.JobName, + JobName: e.Record.JobName, EmittedAt: time.Now(), } - if finished, err := time.ParseInLocation(runRecordTimeLayout, r.Time, time.Local); err == nil { + if finished, err := time.ParseInLocation(runRecordTimeLayout, e.Record.Time, time.Local); err == nil { timing.RunFinished = finished } fyne.Do(func() { timing.UIQueuedAt = time.Now() fyne.CurrentApp().SendNotification(&fyne.Notification{ Title: "GoSentry: Job Failed", - Content: r.JobName + ": " + r.Detail, + Content: e.Record.JobName + ": " + e.Record.Detail, }) timing.AfterSendAt = time.Now() - if err := appendNotificationTimingLog(svc.Store().Paths.LogsDir, timing); err != nil { + if err := appendNotificationTimingLog(svc.Paths().LogsDir, timing); err != nil { fyne.LogError("Failed to write notification timing log", err) } }) } - } - if isError { - events.add(newEvent(0, "Service", "Error", errOccurred.Err.Error())) - } - if isJobsLoaded { + case app.ErrorOccurred: + events.add(newEvent(0, "Service", "Error", e.Err.Error())) + case app.JobsLoaded: // Selecting an existing jobs file replaces the job list without a // prompt, so History carries the receipt: how many jobs, from where. - detail := strconv.Itoa(jobsLoaded.Count) + " jobs from " + jobsLoaded.Path + detail := strconv.Itoa(e.Count) + " jobs from " + e.Path events.add(newEvent(0, "Service", "Jobs loaded", detail)) } refresh() diff --git a/src/ui/run.go b/src/ui/run.go index a57668d..5430f2f 100644 --- a/src/ui/run.go +++ b/src/ui/run.go @@ -70,12 +70,13 @@ func Run(startInTray bool) { a.Run() return } - keepInTray = svc.Store().Config.KeepRunningInTray + config := svc.Config() + keepInTray = config.KeepRunningInTray startHidden = resolveStartHidden(startInTray, keepInTray) applyTrayBehavior(a, w, keepInTray, false) // Apply the persisted theme before building content so the window renders in // the chosen theme from the first frame rather than flashing the default one. - applyTheme(a, svc.Store().Config.Theme) + applyTheme(a, config.Theme) content, recordStartup := newMainView(w, svc) w.SetContent(content) serveSingleInstance(instanceListener, w) diff --git a/src/ui/settings_view.go b/src/ui/settings_view.go index 142fb39..600d88b 100644 --- a/src/ui/settings_view.go +++ b/src/ui/settings_view.go @@ -27,7 +27,14 @@ var settingsCaptions = []string{ } func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { - store := svc.Store() + // saved mirrors the config as last persisted (or freshly loaded at + // construction); it is a local copy the closures below compare the form + // against and reassign after a successful save, rather than holding onto + // the live *storage.Store the Service owns (see app.Service.Config). + // paths never changes after construction of this view — AppDir and + // ConfigPath are fixed for the process — so it is read once, not refreshed. + saved := svc.Config() + paths := svc.Paths() // updateSaveState compares the form to the saved config and enables Save only // when something differs. It is defined below (once Save and every field // exist) but declared here so the field change handlers can reference it. @@ -36,14 +43,14 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { // both the initial load and the Cancel/Defaults buttons below. var loadFields func(domain.Config) startOnLogin := widget.NewCheck("Start on login", nil) - startOnLogin.SetChecked(store.Config.StartOnLogin) + startOnLogin.SetChecked(saved.StartOnLogin) minimizeToTray := widget.NewCheck("Keep running in the system tray", nil) - minimizeToTray.SetChecked(store.Config.KeepRunningInTray) + minimizeToTray.SetChecked(saved.KeepRunningInTray) autostartStatus := widget.NewLabel("") trayRestartHint := widget.NewLabel("") trayRestartHint.Truncation = fyne.TextTruncateClip refreshAutostartStatus := func() { - if settingsPendingAutostart(startOnLogin, minimizeToTray, store.Config) { + if settingsPendingAutostart(startOnLogin, minimizeToTray, saved) { autostartStatus.SetText("Pending: save settings to apply") return } @@ -67,15 +74,15 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { } minimizeToTray.OnChanged = func(bool) { refreshAutostartStatus() - refreshTrayRestartHint(minimizeToTray.Checked != store.Config.KeepRunningInTray) + refreshTrayRestartHint(minimizeToTray.Checked != saved.KeepRunningInTray) updateSaveState() } refreshAutostartStatus() notifications := widget.NewCheck("Show desktop notifications for failed jobs", nil) - notifications.SetChecked(store.Config.NotifyOnFailure) + notifications.SetChecked(saved.NotifyOnFailure) notifications.OnChanged = func(bool) { updateSaveState() } themeSelect := widget.NewSelect([]string{themeLabelSystem, themeLabelGoSentry}, nil) - themeSelect.SetSelected(themeLabel(store.Config.Theme)) + themeSelect.SetSelected(themeLabel(saved.Theme)) // Preview the theme the moment it is picked so the choice is visible before // saving; Save persists it. Reverting the selection reverts the preview, and // closing without saving falls back to the stored theme on next launch. @@ -87,20 +94,20 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { []string{string(domain.ExecutionModeParallel), string(domain.ExecutionModeSequential)}, nil, ) - executionModeSelect.SetSelected(string(store.Config.ExecutionMode)) + executionModeSelect.SetSelected(string(saved.ExecutionMode)) executionModeSelect.OnChanged = func(string) { updateSaveState() } overlapPolicySelect := widget.NewSelect( []string{string(domain.OverlapPolicySkip), string(domain.OverlapPolicyQueue)}, nil, ) - overlapPolicySelect.SetSelected(string(store.Config.OverlapPolicy)) + overlapPolicySelect.SetSelected(string(saved.OverlapPolicy)) overlapPolicySelect.OnChanged = func(string) { updateSaveState() } defaultTimeout := widget.NewEntry() defaultTimeout.SetPlaceHolder("0 = no timeout") - defaultTimeout.SetText(strconv.Itoa(store.Config.DefaultTimeoutSeconds)) + defaultTimeout.SetText(strconv.Itoa(saved.DefaultTimeoutSeconds)) defaultTimeout.OnChanged = func(string) { updateSaveState() } jobsFile := widget.NewEntry() - jobsFile.SetText(store.Config.JobsFile) + jobsFile.SetText(saved.JobsFile) jobsFile.OnChanged = func(string) { updateSaveState() } // The picker only offers existing files; a jobs file that does not exist yet // is entered by typing its path, which Save then creates. @@ -108,7 +115,7 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { chooseJSONFile(w, jobsFile) }) logsDir := widget.NewEntry() - logsDir.SetText(store.Config.LogsDir) + logsDir.SetText(saved.LogsDir) logsDir.OnChanged = func(string) { updateSaveState() } logsDirBrowse := widget.NewButtonWithIcon("Browse", theme.FolderOpenIcon(), func() { chooseFolder(w, logsDir) @@ -118,13 +125,15 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { // manager. It reveals whatever the field currently holds, so an edit can be // checked before Save. logsDirOpen := widget.NewButtonWithIcon("Open", theme.FolderIcon(), func() { - openFolder(w, settingsFolderPath(store.Paths.AppDir, logsDir.Text)) + openFolder(w, settingsFolderPath(paths.AppDir, logsDir.Text)) }) maxLogFiles := widget.NewEntry() - maxLogFiles.SetText(strconv.Itoa(store.Config.MaxLogFiles)) + maxLogFiles.SetPlaceHolder("0 = unlimited") + maxLogFiles.SetText(strconv.Itoa(saved.MaxLogFiles)) maxLogFiles.OnChanged = func(string) { updateSaveState() } maxLogAgeDays := widget.NewEntry() - maxLogAgeDays.SetText(strconv.Itoa(store.Config.MaxLogAgeDays)) + maxLogAgeDays.SetPlaceHolder("0 = unlimited") + maxLogAgeDays.SetText(strconv.Itoa(saved.MaxLogAgeDays)) maxLogAgeDays.OnChanged = func(string) { updateSaveState() } // Autostart status sits on its own row beneath the checkbox (rather than // beside it) so the Application section fits within a half-width column. @@ -134,13 +143,13 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { saveSettings := widget.NewButtonWithIcon("Save settings", theme.DocumentSaveIcon(), func() { files, err := strconv.Atoi(strings.TrimSpace(maxLogFiles.Text)) - if err != nil || files <= 0 { - settingsStatus.SetText("Max log files must be a positive number") + if err != nil || files < 0 { + settingsStatus.SetText("Max log files must be zero (unlimited) or a positive number") return } days, err := strconv.Atoi(strings.TrimSpace(maxLogAgeDays.Text)) - if err != nil || days <= 0 { - settingsStatus.SetText("Max log age days must be a positive number") + if err != nil || days < 0 { + settingsStatus.SetText("Max log age days must be zero (unlimited) or a positive number") return } if strings.TrimSpace(jobsFile.Text) == "" { @@ -159,7 +168,7 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { // Build the new config from the form and hand it to the Service, which // validates it, persists config and jobs to the (possibly new) directory, // and runs log cleanup so tightened retention limits take effect at once. - config := store.Config + config := saved config.JobsFile = strings.TrimSpace(jobsFile.Text) config.LogsDir = strings.TrimSpace(logsDir.Text) config.MaxLogFiles = files @@ -171,11 +180,16 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { config.OverlapPolicy = domain.OverlapPolicy(overlapPolicySelect.Selected) config.DefaultTimeoutSeconds = timeout config.Theme = themeFromLabel(themeSelect.Selected) - previousKeepInTray := store.Config.KeepRunningInTray + previousKeepInTray := saved.KeepRunningInTray if err := svc.UpdateSettings(config); err != nil { settingsStatus.SetText("Save failed: " + err.Error()) return } + // UpdateSettings may re-resolve paths (a jobs-file switch adopts a + // different directory), so pick up the fresh copy rather than assuming + // config is exactly what landed. + saved = svc.Config() + paths = svc.Paths() if err := svc.ApplyAutostart(); err != nil { refreshAutostartStatus() settingsStatus.SetText("Saved, autostart failed: " + err.Error()) @@ -198,7 +212,7 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { // fields compare against their canonical string form; any unparsable text // counts as a change so the user can click Save and see the validation error. updateSaveState = func() { - c := store.Config + c := saved changed := startOnLogin.Checked != c.StartOnLogin || minimizeToTray.Checked != c.KeepRunningInTray || notifications.Checked != c.NotifyOnFailure || @@ -235,17 +249,17 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { logsDir.SetText(c.LogsDir) maxLogFiles.SetText(strconv.Itoa(c.MaxLogFiles)) maxLogAgeDays.SetText(strconv.Itoa(c.MaxLogAgeDays)) - if settingsPendingAutostart(startOnLogin, minimizeToTray, store.Config) { + if settingsPendingAutostart(startOnLogin, minimizeToTray, saved) { autostartStatus.SetText("Pending: save settings to apply") } else { refreshAutostartStatus() } - refreshTrayRestartHint(minimizeToTray.Checked != store.Config.KeepRunningInTray) + refreshTrayRestartHint(minimizeToTray.Checked != saved.KeepRunningInTray) settingsStatus.SetText("") updateSaveState() } cancelSettings := widget.NewButtonWithIcon("Cancel", theme.CancelIcon(), func() { - loadFields(store.Config) + loadFields(saved) }) restoreDefaults := widget.NewButtonWithIcon("Defaults", theme.MediaReplayIcon(), func() { loadFields(domain.DefaultConfig()) @@ -261,7 +275,7 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { executionModeSelect: executionModeSelect, overlapPolicySelect: overlapPolicySelect, defaultTimeout: defaultTimeout, - configPath: store.Paths.ConfigPath, + configPath: paths.ConfigPath, jobsFile: jobsFile, jobsFileBrowse: jobsFileBrowse, logsDir: logsDir,