From 18da0215260e5594d679ce4b08c30e91442544d8 Mon Sep 17 00:00:00 2001 From: Mikhail Yenuchenko Date: Fri, 7 Aug 2026 03:35:10 +0300 Subject: [PATCH] chore: land the remaining low-severity items from the whole-project review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 11 of PROJECT_REVIEW_PLAN.md: the themed cleanup pass over every low-severity finding still open (2.2-2.3, 3.4-3.6, 4.3-4.7, 6.4-6.7, 7.1-7.3, 8.2-8.3, 9.1-9.4, and the under-documented decisions in §10/§11). Behavioral fixes: - Reassign duplicate job IDs in a hand-edited jobs.json instead of letting two jobs share one runtime, schedule entry, and SeedStats bucket. - Disambiguate run-log file names that collide within the same second. - Compute AvgDurationMS as DurationSumMS/TimedRunCount instead of an incremental integer mean, so it always matches the seeded-from-logs average instead of drifting from truncation error. - Clean absolute paths in ResolveConfiguredPath so two spellings of the same jobs file do not trigger a spurious adoption. - Report InstallDesktopIcon failures through ErrorOccurred instead of discarding them silently. - Move settingsView's blocking AutostartStatus (PowerShell on Windows) off the UI thread. - Give notify-timing.tsv its own extension so CleanupLogs no longer manages it as a run log. - Replace the settingsView Save handler's second copy of validateConfig's rules with a bare parse, letting the Service's own error surface. Cleanups: - Delete collectActivity, the dead yaml tags on RunRecord, and the logArguments/LogArguments alias. - Fold the two systemTrayRegistered/mainWindowHidden globals into one trayState instance Run owns and threads through Settings and the single-instance reveal path. - Fix stale comments/docs: the frozen window-size restore claim, a reference to a renamed recordRun, README's "Pause all" and notification wording, the PowerShell quoting note for TESTS.md's coverage command, and scripts/test.bat's UTF-8 checkmarks under a non-UTF-8 code page. - Document the single-instance fallback's consequence and the unauthenticated instance-channel port in STANDARDS.md; record the config-shim retirement plan in ROADMAP.md. 3.5, 7.3, and 9.4 turned out to already be fixed by earlier phases; no change needed for those three. Co-Authored-By: Claude Sonnet 5 --- README.md | 7 ++-- docs/ARCHITECTURE.md | 3 +- docs/ROADMAP.md | 24 ++++++++++++-- docs/STANDARDS.md | 16 +++++++++ docs/TESTS.md | 34 +++++++++++++++++-- scripts/test.bat | 7 ++++ src/app/platform.go | 17 +++++++--- src/app/run.go | 3 +- src/app/run_test.go | 9 +++++ src/app/service.go | 1 + src/domain/record.go | 18 +++++----- src/domain/runtime.go | 8 +++++ src/runner/invocation.go | 2 -- src/runner/logfile.go | 24 ++++++++++++-- src/runner/logfile_test.go | 36 ++++++++++++++++++++ src/runner/runner.go | 2 +- src/runner/runner_test.go | 4 +-- src/runner/seed.go | 5 +++ src/runner/seed_test.go | 3 ++ src/storage/store.go | 25 +++++++++++--- src/storage/store_test.go | 45 +++++++++++++++++++++++++ src/ui/history_view.go | 18 ---------- src/ui/history_view_test.go | 27 --------------- src/ui/jobs_view.go | 4 ++- src/ui/jobs_view_helpers.go | 2 +- src/ui/mainwindow.go | 24 +++++--------- src/ui/mainwindow_test.go | 4 +-- src/ui/notify_timing.go | 5 ++- src/ui/run.go | 19 +++++------ src/ui/settings_view.go | 66 +++++++++++++++++++++---------------- src/ui/singleinstance.go | 9 +++-- src/ui/tray.go | 53 +++++++++++++++-------------- 32 files changed, 357 insertions(+), 167 deletions(-) create mode 100644 src/runner/logfile_test.go diff --git a/README.md b/README.md index a33095d..7c0998c 100644 --- a/README.md +++ b/README.md @@ -181,7 +181,7 @@ Named descriptors are also accepted: `@hourly`, `@daily`, `@weekly`, 3. Set **Schedule**, **Command**, optional **Arguments**, **Folder**, and **Enabled**. 4. Use **Run now** for a one-off manual run without waiting for the schedule. 5. Use **Pause** on a single job to suspend it without deleting it. -6. Use **Pause all** as a global stop switch for all scheduled runs. +6. Use **Disable auto** as a global stop switch for all scheduled runs. 7. Open **History** to see past runs, their trigger (`Manual`, `Schedule`, or `UI`), state, and log file. 8. Open **Settings** to change the storage paths, log cleanup limits, queue behavior, and notifications. @@ -241,8 +241,9 @@ sets one. ## Notifications When **Notify on failure** is enabled in Settings, GoSentry sends a desktop -notification whenever a scheduled or manual run exits with a non-zero exit code. -The notification shows the job name and the exit code. +notification whenever a scheduled or manual run ends in the `Failed` state — +a non-zero exit code, a timeout, or a process that failed to start. +The notification shows the job name and the failure detail. ## Autostart diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index f21ab78..4173d41 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -237,8 +237,9 @@ applies to them — and so measure launch latency only. | `RunCount` | total runs recorded | | `FailCount` | runs that exited non-zero | | `LastDurationMS` | wall-clock time of the most recent run (launch latency for `StartOnly`) | -| `AvgDurationMS` | mean over all runs with a recorded duration | +| `AvgDurationMS` | mean over all runs with a recorded duration, computed as `DurationSumMS / TimedRunCount` on every update rather than folded incrementally, so it never disagrees with the exact sum/count average `runner.aggregateLogStats` computes when seeding from logs | | `MaxDurationMS` | longest recorded run | +| `DurationSumMS` | running total of every timed run's duration; the source `AvgDurationMS` is divided from | `runner.RunJob` measures the wall-clock start→finish and sets `DurationMS` on the returned `RunRecord`. `runner/logfile.go` writes a `duration` line into the diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index d382c0f..db038f4 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -18,15 +18,35 @@ machine):** average **773 ms** per toast (695–874 ms), dominated by PowerShell cold start. Re-run the script when comparing after a native toast implementation. **App-side timing:** each failure notification appends one line to -`logs/notify-timing.log` (`ms_after_run`, `ms_fyne_do`, `ms_send`, +`logs/notify-timing.tsv` (`ms_after_run`, `ms_fyne_do`, `ms_send`, `ms_app_total`). These columns end when Fyne returns from `SendNotification`; OS -toast latency is not included. +toast latency is not included. The `.tsv` extension keeps it out of +`runner.CleanupLogs`, which only manages `.log` files — this file is +diagnostic instrumentation for this item, not job output, and should be +removed (or unified with the run-log retention policy under its own knob) once +the native-toast direction below lands and the timing data is no longer +needed. **Direction:** add `src/platform/notify/` with a native Windows toast (WinRT or a maintained Go wrapper), used for failure notifications on Windows. Keep Fyne `SendNotification` on Linux (DBus / xdg-desktop-portal) unless profiling shows it needs the same treatment. +### Retire the config compatibility shims + +Two read-only shims in `storage.loadOrCreateConfig` rewrite an old file into +the current shape on the next save, so each becomes dead the moment a user's +config has been saved once by a build that has it: + +- `Config.JobsDir` (pre-0.15, superseded by `Config.JobsFile`). +- `Theme == "default"` (pre-1.0.1, superseded by `ThemeSystem`). + +Neither has an expiry. Remove both — the field, the migration branch, and +`TestLoadOrCreateConfigMigratesJobsDir` / +`TestLoadOrCreateConfigMigratesLegacyThemeDefault` — once a release has shipped +long enough that a config file still carrying either old shape is not a +realistic upgrade path GoSentry needs to support. + ### Dynamic tray icon toggle Fyne exposes `SetSystemTrayIcon` and related APIs only at application startup. diff --git a/docs/STANDARDS.md b/docs/STANDARDS.md index cfb072e..210bd3a 100644 --- a/docs/STANDARDS.md +++ b/docs/STANDARDS.md @@ -121,6 +121,22 @@ change to their shape has to stay compatible on its own. an occurrence that fired before the pause/disable. The details pane appends ", N queued" to the statistics line via `DisplayStats` whenever the count is non-zero. +- **Single-instance arbitration falls back to "start anyway" when the port is + held by something else.** `acquireSingleInstance` (`singleinstance.go`) + binds `127.0.0.1:37653`; if that fails and a dial to the same address does + not answer as GoSentry either, startup continues rather than refusing to + open because of an unrelated local listener. The consequence is deliberate + but worth spelling out: two GoSentry processes can then run two schedulers + against the same `jobs.json` and the same logs directory, each overwriting + the other's saves. Atomic writes (`writeFileAtomic`) prevent a *torn* file + from a concurrent write, but not one process's save clobbering the other's. +- **The single-instance channel is an unauthenticated localhost TCP port.** + Port 37653 accepts one command, `"show"`, from any local process — including + one running as a different user on a shared machine. This is a deliberate + scope choice, not an oversight: the command only raises the existing window, + so the impact of an unwelcome sender is a window popping up, not data + exposure or control. Anything with a larger blast radius on that channel + would need real authentication. ## Out of scope diff --git a/docs/TESTS.md b/docs/TESTS.md index 099a65a..225ab48 100644 --- a/docs/TESTS.md +++ b/docs/TESTS.md @@ -26,6 +26,12 @@ The GUI tests build the Fyne desktop backend, so CGO must be enabled; on Windows that means the MSYS2 UCRT64 toolchain described in [DEVELOPMENT.md](DEVELOPMENT.md). +`src/ui` dominates `go test -race ./...`'s wall time — around 229s in the +2026-08-05 whole-project review, against under 8s for every other package +combined. Budget iteration accordingly: a change confined to `domain`, +`storage`, `runner`, `scheduler`, or `app` gets a fast feedback loop; a `ui` +change does not. + ### Manual test commands Run all tests: @@ -63,6 +69,15 @@ covered by the `app` tests. Measure the engine packages together instead: go test -coverpkg=./src/domain,./src/storage,./src/runner,./src/scheduler,./src/app ./src/domain ./src/storage ./src/runner ./src/scheduler ./src/app ``` +In the PowerShell environment DEVELOPMENT.md prescribes on Windows, PowerShell +splits the comma-separated `-coverpkg` list on its own and the command fails +with `directory not found`. Use the stop-parsing token, or quote the whole +flag: + +```powershell +go test --% -coverpkg=./src/domain,./src/storage,./src/runner,./src/scheduler,./src/app ./src/domain ./src/storage ./src/runner ./src/scheduler ./src/app +``` + That figure was 84.4% at the 2026-08-04 review, which is the number to compare against before concluding that coverage has slipped. @@ -245,6 +260,8 @@ Tests JSON round-tripping, default generation, and backward compatibility. | `TestJobsRoundTrip` | Verifies that jobs saved to JSON are reloaded with identical field values. | | `TestConfigRoundTrip` | Verifies that settings saved to JSON are reloaded with identical field values. | | `TestNormalizeJobsFillsDefaults` | Verifies that `normalizeJobs` assigns sequential IDs and sets default name, schedule, and command for jobs missing those fields. | +| `TestNormalizeJobsReassignsDuplicateIDs` | Verifies that a hand-edited `jobs.json` with two entries sharing one ID gets the later duplicates reassigned instead of colliding on one runtime. | +| `TestResolveConfiguredPathCleansAbsolutePaths` | Verifies (Windows only) that forward-slash and backslash spellings of the same absolute path resolve to the same string. | | `TestLoadOrCreateConfigCreatesDefaultsOnFirstRun` | Verifies that a missing config file is created with sane defaults. | | `TestLoadOrCreateJobsSeedsSampleJobsOnFirstRun` | Verifies that a missing jobs file is created with the sample jobs from `defaultJobs`. | | `TestLoadOrCreateConfigKeepsZeroTimeoutOnReload` | Verifies that `default_timeout_seconds: 0` survives a reload rather than being normalized away — 0 is a value, not a missing field. | @@ -376,6 +393,19 @@ Tests log-file cleanup by age and by count. --- +### src/runner/logfile_test.go + +**Package:** `runner` + +Tests the disambiguating suffix `writeRunLog` applies when two runs land on +the same second. + +| Test | Purpose | +|------|---------| +| `TestUniqueLogPathAvoidsCollision` | Verifies repeated calls for the same file name return distinct paths instead of silently overwriting an existing log. | + +--- + ### src/platform/autostart/autostart_windows_test.go **Location:** `src/platform/autostart/autostart_windows_test.go` @@ -508,8 +538,6 @@ column-width behaviour of the assembled table. | Test | Purpose | |------|---------| -| `TestCollectActivityMergesAndSorts` | Verifies per-job logs are merged and sorted by time. | -| `TestCollectActivitySkipsMissingRuntimes` | Verifies missing runtime entries are skipped safely. | | `TestHistoryCellText` | Verifies table cell text for all columns; empty trigger → `Unknown`. | | `TestLogFileName` | Verifies log path basename extraction on Windows and Unix paths. | | `TestNewEventUsesConsistentTimestampShape` | Verifies UI events use the same timestamp layout as run records. | @@ -593,7 +621,7 @@ Tests the failure-notification timing diagnostics added in 1.0.2. | Test | Purpose | |------|---------| | `TestNotificationTimingFormatLine` | Verifies `notificationTiming.formatLine` renders the job name and the three millisecond deltas (`ms_after_run`, `ms_fyne_do`, `ms_send`) plus their sum (`ms_app_total`). | -| `TestAppendNotificationTimingLogWritesHeaderAndRow` | Verifies `appendNotificationTimingLog` creates `notify-timing.log` with its header on first write and appends a row containing the job name. | +| `TestAppendNotificationTimingLogWritesHeaderAndRow` | Verifies `appendNotificationTimingLog` creates `notify-timing.tsv` with its header on first write and appends a row containing the job name. | --- diff --git a/scripts/test.bat b/scripts/test.bat index e2a724d..c4c010a 100644 --- a/scripts/test.bat +++ b/scripts/test.bat @@ -6,6 +6,13 @@ REM Runs go vet and go test with race detection REM Move to repository root cd /d "%~dp0\.." +REM This file is UTF-8 (the ✓/✗ below). cmd.exe reads batch files in the +REM console's active code page, which defaults to the system locale (e.g. +REM CP866 on Russian Windows) rather than UTF-8, so without this the two +REM symbols render as mojibake. Switching the console to UTF-8 first fixes +REM that; >nul silences chcp's own "Active code page" confirmation line. +chcp 65001 >nul + REM Fyne uses native libraries through CGO. MSYS2 UCRT64 provides the GCC toolchain REM expected by the Windows build; prepending it keeps the script self-contained REM without permanently changing the user's system PATH. diff --git a/src/app/platform.go b/src/app/platform.go index bd38ee7..59c8acf 100644 --- a/src/app/platform.go +++ b/src/app/platform.go @@ -1,18 +1,25 @@ package app import ( + "fmt" + "gitea.mixdep.ru/mix/gosentry/src/platform/desktop" ) // InstallDesktopIcon installs the application's .desktop file and icon on // Linux (no-op on other platforms). The resulting icon path is stored in -// store.Paths.DesktopIcon so ApplyAutostart can reference it. +// store.Paths.DesktopIcon so ApplyAutostart can reference it. A failure is +// reported through ErrorOccurred rather than discarded, so the visible symptom +// (a generic dock icon) has an explanation in History instead of none. func (s *Service) InstallDesktopIcon(appID string, iconBytes []byte) { - if iconPath, err := desktop.InstallDesktopIntegration(appID, s.store.Paths.ExecutablePath, iconBytes); err == nil { - s.mu.Lock() - s.store.Paths.DesktopIcon = iconPath - s.mu.Unlock() + iconPath, err := desktop.InstallDesktopIntegration(appID, s.store.Paths.ExecutablePath, iconBytes) + if err != nil { + s.emit(ErrorOccurred{Err: fmt.Errorf("install desktop icon: %w", err)}) + return } + s.mu.Lock() + s.store.Paths.DesktopIcon = iconPath + s.mu.Unlock() } // AutostartStatus reports whether the platform autostart entry matches the diff --git a/src/app/run.go b/src/app/run.go index 9632ba6..40cea56 100644 --- a/src/app/run.go +++ b/src/app/run.go @@ -253,7 +253,8 @@ func updateStats(rt *domain.JobRuntime, r domain.RunRecord) { rt.MaxDurationMS = r.DurationMS } rt.TimedRunCount++ - rt.AvgDurationMS = (rt.AvgDurationMS*int64(rt.TimedRunCount-1) + r.DurationMS) / int64(rt.TimedRunCount) + rt.DurationSumMS += r.DurationMS + rt.AvgDurationMS = rt.DurationSumMS / int64(rt.TimedRunCount) } // runningOutput is the placeholder output shown while a job is running, before diff --git a/src/app/run_test.go b/src/app/run_test.go index 098206e..40ae8eb 100644 --- a/src/app/run_test.go +++ b/src/app/run_test.go @@ -103,6 +103,15 @@ func TestUpdateStats(t *testing.T) { if rt.AvgDurationMS != 233 { t.Errorf("after run 3: avg=%d, want 233", rt.AvgDurationMS) } + // AvgDurationMS must always be exactly DurationSumMS/TimedRunCount — a stored + // sum divided once, not an incremental mean that truncates on every step and + // compounds error over a long-running job. + if rt.DurationSumMS != 700 { + t.Errorf("DurationSumMS = %d, want 700", rt.DurationSumMS) + } + if want := rt.DurationSumMS / int64(rt.TimedRunCount); rt.AvgDurationMS != want { + t.Errorf("AvgDurationMS = %d, want DurationSumMS/TimedRunCount = %d", rt.AvgDurationMS, want) + } } func TestUpdateStatsSkipsZeroDuration(t *testing.T) { diff --git a/src/app/service.go b/src/app/service.go index 2da02c3..5c5fbbd 100644 --- a/src/app/service.go +++ b/src/app/service.go @@ -151,6 +151,7 @@ func (s *Service) applySeededStatsLocked(seeds map[int]runner.SeededStats) { runtime.AvgDurationMS = seed.AvgDurationMS runtime.MaxDurationMS = seed.MaxDurationMS runtime.TimedRunCount = seed.TimedRunCount + runtime.DurationSumMS = seed.DurationSumMS } } diff --git a/src/domain/record.go b/src/domain/record.go index e4d5999..2c5f72e 100644 --- a/src/domain/record.go +++ b/src/domain/record.go @@ -4,13 +4,13 @@ package domain // output is also written to a log file; the in-memory Output copy exists so the // latest run can be displayed without reopening the log on every repaint. type RunRecord struct { - Time string `yaml:"time"` - JobID int `yaml:"job_id"` - JobName string `yaml:"job_name"` - Trigger string `yaml:"trigger,omitempty"` - State string `yaml:"state"` - Detail string `yaml:"detail"` - LogFile string `yaml:"log_file,omitempty"` - Output string `yaml:"output,omitempty"` - DurationMS int64 `yaml:"duration_ms,omitempty"` + Time string + JobID int + JobName string + Trigger string + State string + Detail string + LogFile string + Output string + DurationMS int64 } diff --git a/src/domain/runtime.go b/src/domain/runtime.go index 2d6ed0f..cc33bf1 100644 --- a/src/domain/runtime.go +++ b/src/domain/runtime.go @@ -36,6 +36,14 @@ type JobRuntime struct { // launches that round to 0) increment RunCount but not this. StartOnly runs // otherwise contribute their launch latency. TimedRunCount int + // DurationSumMS is the running total of every timed run's duration. + // AvgDurationMS is always DurationSumMS/TimedRunCount, computed fresh on each + // update rather than folded incrementally — an incremental integer mean + // truncates on every step, and the error compounds over the life of a job + // that keeps running. A stored sum divided once per update matches the exact + // sum/count average runner.aggregateLogStats computes when seeding from logs, + // so the two no longer disagree about the same run history. + DurationSumMS int64 } // NewRuntime builds the initial runtime state for a freshly loaded or created diff --git a/src/runner/invocation.go b/src/runner/invocation.go index b72d513..7efeee5 100644 --- a/src/runner/invocation.go +++ b/src/runner/invocation.go @@ -64,5 +64,3 @@ func LogArguments(arguments string) string { } return strings.ReplaceAll(strings.TrimSpace(arguments), "\r\n", "\n") } - -func logArguments(arguments string) string { return LogArguments(arguments) } diff --git a/src/runner/logfile.go b/src/runner/logfile.go index 7d10410..298ebf0 100644 --- a/src/runner/logfile.go +++ b/src/runner/logfile.go @@ -23,9 +23,9 @@ func writeRunLog(logsDir string, job domain.Job, trigger string, state string, d // by run time. The job name is included for human scanning, but sanitized to // avoid characters that are invalid on Windows or awkward on shells. fileName := started.Format("20060102-150405") + "_" + sanitizeFileName(job.Name) + ".log" - path := filepath.Join(logsDir, fileName) + path := uniqueLogPath(logsDir, fileName) content := fmt.Sprintf("time: %s\njob_id: %d\njob_name: %s\ntrigger: %s\nstate: %s\ndetail: %s\nduration: %d\ncommand: %s\narguments: %s\nstart_only: %t\n\n%s\n", - started.Format("2006-01-02 15:04:05"), job.ID, job.Name, trigger, state, detail, durationMS, job.Command, logArguments(job.Arguments), job.StartOnly, output) + started.Format("2006-01-02 15:04:05"), job.ID, job.Name, trigger, state, detail, durationMS, job.Command, LogArguments(job.Arguments), job.StartOnly, output) if err := writeFileAtomic(logsDir, path, []byte(content), 0o644); err != nil { return "", fmt.Errorf("write log file: %w", err) } @@ -70,6 +70,26 @@ func writeFileAtomic(dir, path string, data []byte, perm os.FileMode) error { return nil } +// uniqueLogPath returns a path for fileName in dir, appending a disambiguating +// "-2", "-3", … suffix before the extension if the plain name is already +// taken. Two runs of the same job in the same second — a fast manual re-run, +// or a sub-second queue drain — would otherwise share one timestamp and the +// second write would silently overwrite the first. +func uniqueLogPath(dir, fileName string) string { + path := filepath.Join(dir, fileName) + if _, err := os.Stat(path); err != nil { + return path + } + ext := filepath.Ext(fileName) + base := strings.TrimSuffix(fileName, ext) + for n := 2; ; n++ { + candidate := filepath.Join(dir, fmt.Sprintf("%s-%d%s", base, n, ext)) + if _, err := os.Stat(candidate); err != nil { + return candidate + } + } +} + func sanitizeFileName(name string) string { name = strings.TrimSpace(name) if name == "" { diff --git a/src/runner/logfile_test.go b/src/runner/logfile_test.go new file mode 100644 index 0000000..4e79263 --- /dev/null +++ b/src/runner/logfile_test.go @@ -0,0 +1,36 @@ +package runner + +import ( + "os" + "path/filepath" + "testing" +) + +// TestUniqueLogPathAvoidsCollision pins the fix for two runs of the same job +// landing on the same second: without disambiguation the second write would +// silently overwrite the first. +func TestUniqueLogPathAvoidsCollision(t *testing.T) { + dir := t.TempDir() + const name = "20260101-120000_job.log" + + first := uniqueLogPath(dir, name) + if first != filepath.Join(dir, name) { + t.Fatalf("first call: got %q, want the plain name", first) + } + if err := os.WriteFile(first, []byte("one"), 0o644); err != nil { + t.Fatal(err) + } + + second := uniqueLogPath(dir, name) + if second == first { + t.Fatalf("second call returned the same path as an existing file: %q", second) + } + if err := os.WriteFile(second, []byte("two"), 0o644); err != nil { + t.Fatal(err) + } + + third := uniqueLogPath(dir, name) + if third == first || third == second { + t.Fatalf("third call collided with an existing file: %q (existing: %q, %q)", third, first, second) + } +} diff --git a/src/runner/runner.go b/src/runner/runner.go index 4ad280f..9de4a12 100644 --- a/src/runner/runner.go +++ b/src/runner/runner.go @@ -116,7 +116,7 @@ func startOnlyOutput(job domain.Job, pid int) string { builder.WriteString("command:\n") builder.WriteString(job.Command + "\n\n") builder.WriteString("arguments:\n") - builder.WriteString(logArguments(job.Arguments)) + builder.WriteString(LogArguments(job.Arguments)) builder.WriteString("\n\nstart_only:\ntrue") return builder.String() } diff --git a/src/runner/runner_test.go b/src/runner/runner_test.go index 600f354..a94771a 100644 --- a/src/runner/runner_test.go +++ b/src/runner/runner_test.go @@ -135,8 +135,8 @@ func TestLogArguments(t *testing.T) { {"--flag\n--value", "--flag\n--value"}, } for _, tc := range cases { - if got := logArguments(tc.input); got != tc.want { - t.Errorf("logArguments(%q) = %q, want %q", tc.input, got, tc.want) + if got := LogArguments(tc.input); got != tc.want { + t.Errorf("LogArguments(%q) = %q, want %q", tc.input, got, tc.want) } } } diff --git a/src/runner/seed.go b/src/runner/seed.go index 624d588..c62d446 100644 --- a/src/runner/seed.go +++ b/src/runner/seed.go @@ -21,6 +21,10 @@ type SeededStats struct { AvgDurationMS int64 MaxDurationMS int64 TimedRunCount int + // DurationSumMS is the running total AvgDurationMS was computed from, folded + // into JobRuntime.DurationSumMS so app.updateStats continues the same exact + // sum instead of restarting from a value it would have to reverse-multiply. + DurationSumMS int64 } // SeedStats scans logsDir once and reconstructs per-job execution-time @@ -113,6 +117,7 @@ func aggregateLogStats(files []logSummary) SeededStats { } if durationCount > 0 { stats.TimedRunCount = durationCount + stats.DurationSumMS = durationSum stats.AvgDurationMS = durationSum / int64(durationCount) } return stats diff --git a/src/runner/seed_test.go b/src/runner/seed_test.go index 7d92919..75c0a9c 100644 --- a/src/runner/seed_test.go +++ b/src/runner/seed_test.go @@ -61,6 +61,9 @@ func TestSeedStatsBasic(t *testing.T) { if s.AvgDurationMS != 400 { t.Errorf("AvgDurationMS = %d, want 400", s.AvgDurationMS) } + if s.DurationSumMS != 1200 { + t.Errorf("DurationSumMS = %d, want 1200", s.DurationSumMS) + } } // TestSeedStatsDurationLessLegacyLog verifies that a log without a duration diff --git a/src/storage/store.go b/src/storage/store.go index eecb14a..24c834b 100644 --- a/src/storage/store.go +++ b/src/storage/store.go @@ -19,6 +19,13 @@ type Store struct { // PeekKeepRunningInTray reads keep_running_in_tray from gosentry.json for startup // decisions that must run before app.Open(). On error it returns the built-in // default. +// +// Despite the name, this can write: loadOrCreateConfig creates gosentry.json +// with defaults on first run, the same as OpenStore does moments later when +// app.Open() parses the now-existing file again. The double parse and the +// write-on-read are both harmless — the second read just sees the file the +// first one created — but worth knowing before adding a third startup path +// that also wants an early look at the config. func PeekKeepRunningInTray() bool { paths, err := ResolvePaths() if err != nil { @@ -207,13 +214,18 @@ func loadOrCreateJobs(path string) ([]domain.Job, error) { func normalizeJobs(jobs []domain.Job) { next := 1 + seen := make(map[int]bool, len(jobs)) for index := range jobs { job := &jobs[index] - if job.ID <= 0 { - // IDs are assigned only when absent. Existing IDs stay stable because - // History and future log associations use them to identify jobs. + if job.ID <= 0 || seen[job.ID] { + // IDs are assigned only when absent or already claimed by an earlier job + // in this file — a hand-edited jobs.json can carry two entries with the + // same ID, which would otherwise share one runtime, one schedule-cache + // entry, and one SeedStats bucket. Existing, unique IDs stay stable + // because History and future log associations use them to identify jobs. job.ID = next } + seen[job.ID] = true if job.ID >= next { next = job.ID + 1 } @@ -241,7 +253,12 @@ func normalizeJobs(jobs []domain.Job) { // apply the same rule to a path the user has typed but not yet saved. func ResolveConfiguredPath(appDir string, path string) string { if filepath.IsAbs(path) { - return path + // Cleaned so two spellings of the same file (forward vs. backslashes, a + // trailing separator) resolve to the same string. UpdateSettings compares + // this against Paths.JobsPath to decide whether the jobs file is changing, + // so an uncleaned path here could trigger a spurious adoption against the + // file the app is already using. + return filepath.Clean(path) } // Relative paths are resolved against the executable directory, not the // process working directory. This matches ResolvePaths and keeps shortcuts, diff --git a/src/storage/store_test.go b/src/storage/store_test.go index 231987b..b195942 100644 --- a/src/storage/store_test.go +++ b/src/storage/store_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "runtime" "strings" "testing" @@ -145,6 +146,50 @@ func TestNormalizeJobsFillsDefaults(t *testing.T) { } } +// TestNormalizeJobsReassignsDuplicateIDs pins the fix for a hand-edited +// jobs.json carrying two entries with the same ID: without reassignment both +// would share one JobRuntime, one schedule-cache entry, and one SeedStats +// bucket, so editing or deleting either would silently affect both. +func TestNormalizeJobsReassignsDuplicateIDs(t *testing.T) { + jobs := []domain.Job{ + {ID: 5, Name: "First"}, + {ID: 5, Name: "Second"}, + {ID: 5, Name: "Third"}, + } + + normalizeJobs(jobs) + + seen := make(map[int]bool, len(jobs)) + for _, job := range jobs { + if seen[job.ID] { + t.Fatalf("ID %d assigned to more than one job after normalization: %+v", job.ID, jobs) + } + seen[job.ID] = true + } + if jobs[0].ID != 5 { + t.Errorf("first occurrence should keep its ID: got %d, want 5", jobs[0].ID) + } + if jobs[1].ID == 5 || jobs[2].ID == 5 { + t.Errorf("later duplicates should be reassigned away from 5: got %d, %d", jobs[1].ID, jobs[2].ID) + } +} + +// TestResolveConfiguredPathCleansAbsolutePaths pins the fix for two spellings +// of the same absolute path (forward vs. backslashes) resolving to different +// strings: UpdateSettings compares this against Paths.JobsPath as strings to +// decide whether the jobs file is changing, so an uncleaned path here could +// trigger a spurious adoption against the file already in use. +func TestResolveConfiguredPathCleansAbsolutePaths(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("backslash vs. forward-slash spellings of the same path are a Windows-only ambiguity") + } + got := ResolveConfiguredPath(`C:\app`, "C:/data/jobs.json") + want := ResolveConfiguredPath(`C:\app`, `C:\data\jobs.json`) + if got != want { + t.Errorf("forward-slash and backslash spellings resolved differently: %q vs %q", got, want) + } +} + func TestLoadOrCreateConfigCreatesDefaultsOnFirstRun(t *testing.T) { dir := t.TempDir() paths := Paths{ diff --git a/src/ui/history_view.go b/src/ui/history_view.go index c0b11bb..aa86058 100644 --- a/src/ui/history_view.go +++ b/src/ui/history_view.go @@ -5,8 +5,6 @@ import ( "strings" "time" - "gitea.mixdep.ru/mix/gosentry/src/domain" - "fyne.io/fyne/v2" "fyne.io/fyne/v2/container" "fyne.io/fyne/v2/theme" @@ -26,22 +24,6 @@ func newEvent(jobID int, jobName string, state string, detail string) event { } } -func collectActivity(jobs []job, runtimes map[int]*domain.JobRuntime) []event { - var events []event - for _, current := range jobs { - // At startup this is usually empty because jobs.json does not persist - // runtime logs. The function still centralizes the merge for future - // history loading from log metadata. - if rt := runtimes[current.ID]; rt != nil { - events = append(events, rt.Logs...) - } - } - sort.SliceStable(events, func(left int, right int) bool { - return events[left].Time < events[right].Time - }) - return events -} - // textWidth measures how wide s renders at the theme's current body text size. func textWidth(s string) float32 { return fyne.MeasureText(s, theme.TextSize(), fyne.TextStyle{}).Width diff --git a/src/ui/history_view_test.go b/src/ui/history_view_test.go index 168ca07..7826041 100644 --- a/src/ui/history_view_test.go +++ b/src/ui/history_view_test.go @@ -6,8 +6,6 @@ import ( "testing" "time" - "gitea.mixdep.ru/mix/gosentry/src/domain" - "fyne.io/fyne/v2" "fyne.io/fyne/v2/test" "fyne.io/fyne/v2/widget" @@ -57,31 +55,6 @@ func TestIndexOfID(t *testing.T) { } } -func TestCollectActivityMergesAndSorts(t *testing.T) { - jobs := []job{ - {ID: 1, Name: "A"}, - {ID: 2, Name: "B"}, - } - runtimes := map[int]*domain.JobRuntime{ - 1: {Logs: []domain.RunRecord{{Time: "2026-01-02 10:00:00", JobID: 1}}}, - 2: {Logs: []domain.RunRecord{{Time: "2026-01-01 09:00:00", JobID: 2}}}, - } - got := collectActivity(jobs, runtimes) - if len(got) != 2 { - t.Fatalf("len = %d, want 2", len(got)) - } - if got[0].Time != "2026-01-01 09:00:00" || got[1].Time != "2026-01-02 10:00:00" { - t.Errorf("sort order = %v, want ascending by Time", got) - } -} - -func TestCollectActivitySkipsMissingRuntimes(t *testing.T) { - jobs := []job{{ID: 1, Name: "A"}} - if got := collectActivity(jobs, nil); len(got) != 0 { - t.Errorf("nil runtimes: got %v, want empty", got) - } -} - func TestHistoryCellText(t *testing.T) { events := []event{{ Time: "2026-06-01 12:00:00", diff --git a/src/ui/jobs_view.go b/src/ui/jobs_view.go index ae47102..04b325e 100644 --- a/src/ui/jobs_view.go +++ b/src/ui/jobs_view.go @@ -85,8 +85,10 @@ func (v *jobsView) refresh() { // it is re-read here rather than mirrored from the tap handler alone — that is // what makes this view a consumer of SchedulerStateChanged. v.applySchedulerState(v.svc.Config().Paused) + // updateDetails already ends in a d.logs.Refresh() (both its update and clear + // paths do), so refreshing the activity list again here would redraw it twice + // per call. v.updateDetails() - v.dp.logs.Refresh() v.list.Refresh() v.syncListSelection() } diff --git a/src/ui/jobs_view_helpers.go b/src/ui/jobs_view_helpers.go index 6b75bb2..0004e31 100644 --- a/src/ui/jobs_view_helpers.go +++ b/src/ui/jobs_view_helpers.go @@ -8,7 +8,7 @@ import ( // lastJobLogs returns a fresh slice of the most recent activity entries for the // "Selected job activity" panel. Logs are stored newest-first (see -// app.Service.recordRun), so the leading entries are the latest; the result is +// app.prependLog), so the leading entries are the latest; the result is // capped at maxJobActivityRows. func lastJobLogs(logs []event) []event { n := len(logs) diff --git a/src/ui/mainwindow.go b/src/ui/mainwindow.go index e4c6a62..33156ba 100644 --- a/src/ui/mainwindow.go +++ b/src/ui/mainwindow.go @@ -21,20 +21,11 @@ const runRecordTimeLayout = "2006-01-02 15:04:05" type job = domain.Job type event = domain.RunRecord -func newMainView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func(time.Duration, bool)) { - svc.InstallDesktopIcon(appID, assets.IconBytes()) - - // Build the initial event history from the current runtime state. Jobs and - // runtimes are read here only for this one-time initialization; the jobs view - // owns all subsequent state via its own syncFromService closure. - initialJobs := svc.Jobs() - initialRuntimes := make(map[int]*domain.JobRuntime, len(initialJobs)) - for _, j := range initialJobs { - if rt := svc.Runtime(j.ID); rt != nil { - initialRuntimes[j.ID] = rt - } - } - events := newHistoryLog(collectActivity(initialJobs, initialRuntimes)) +func newMainView(w fyne.Window, svc *app.Service, tray *trayState) (fyne.CanvasObject, func(time.Duration, bool)) { + // History is session-only: jobs.json never persists JobRuntime.Logs (see + // domain.JobRuntime), so there is nothing to seed the History tab with at + // startup. It starts empty and fills as events arrive. + events := newHistoryLog(nil) jobsPanel, refreshJobsView := newJobsView(w, svc) @@ -106,12 +97,15 @@ func newMainView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func(time. refresh() }) })) + // Installed after Subscribe so a failure reaches History through + // ErrorOccurred instead of being emitted to no listener. + svc.InstallDesktopIcon(appID, assets.IconBytes()) svc.Start() tabs := container.NewAppTabs( container.NewTabItemWithIcon("Jobs", theme.ListIcon(), jobsPanel), container.NewTabItemWithIcon("History", theme.HistoryIcon(), history), - container.NewTabItemWithIcon("Settings", theme.SettingsIcon(), settingsView(w, svc)), + container.NewTabItemWithIcon("Settings", theme.SettingsIcon(), settingsView(w, svc, tray)), ) tabs.SetTabLocation(container.TabLocationTop) diff --git a/src/ui/mainwindow_test.go b/src/ui/mainwindow_test.go index 10a894f..8e96200 100644 --- a/src/ui/mainwindow_test.go +++ b/src/ui/mainwindow_test.go @@ -66,7 +66,7 @@ func TestMainViewFitsTheDefaultWindowSize(t *testing.T) { svc := app.NewService(store, nil) defer svc.Stop() - content, _ := newMainView(w, svc) + content, _ := newMainView(w, svc, &trayState{}) min := content.MinSize() if min.Width > defaultWindowWidth || min.Height > defaultWindowHeight { t.Errorf("content.MinSize() = %v, want within %vx%v", min, defaultWindowWidth, defaultWindowHeight) @@ -111,7 +111,7 @@ func TestMainViewRecordStartupAddsHistoryRow(t *testing.T) { svc := newTestService(t) defer svc.Stop() - content, recordStartup := newMainView(w, svc) + content, recordStartup := newMainView(w, svc, &trayState{}) w.SetContent(content) table := historyTable(t, content) diff --git a/src/ui/notify_timing.go b/src/ui/notify_timing.go index f66f9cf..bec79ba 100644 --- a/src/ui/notify_timing.go +++ b/src/ui/notify_timing.go @@ -7,7 +7,10 @@ import ( "time" ) -const notificationTimingLogName = "notify-timing.log" +// notificationTimingLogName deliberately does not end in .log: runner.CleanupLogs +// only manages .log files in the logs directory, and this diagnostic file +// should not be subject to (or counted against) that retention policy. +const notificationTimingLogName = "notify-timing.tsv" // notificationTiming captures wall-clock points from a failed run through // SendNotification. It does not include OS toast display latency — Fyne on diff --git a/src/ui/run.go b/src/ui/run.go index 5430f2f..20cfa1d 100644 --- a/src/ui/run.go +++ b/src/ui/run.go @@ -17,9 +17,10 @@ import ( const appID = "ru.mixeme.gosentry.desktop" // defaultWindowWidth and defaultWindowHeight are the size the window opens at -// on first launch (later launches restore the last size from preferences). -// Fyne enforces the assembled content's MinSize as a hard floor over these, so -// they only take effect if the content actually fits within them. +// on every launch. Window size persistence is frozen (see ROADMAP.md), so +// there is no saved size to restore. Fyne enforces the assembled content's +// MinSize as a hard floor over these, so they only take effect if the content +// actually fits within them. const defaultWindowWidth = 1024 const defaultWindowHeight = 660 @@ -60,10 +61,7 @@ func Run(startInTray bool) { w := a.NewWindow("GoSentry " + app.Version) setWindowsNotificationIcon() - prefs := a.Preferences() - winW := float32(prefs.FloatWithFallback("window.width", defaultWindowWidth)) - winH := float32(prefs.FloatWithFallback("window.height", defaultWindowHeight)) - w.Resize(fyne.NewSize(winW, winH)) + w.Resize(fyne.NewSize(defaultWindowWidth, defaultWindowHeight)) svc, err := app.Open() if err != nil { w.SetContent(container.NewPadded(widget.NewLabel("Failed to load GoSentry configuration: " + err.Error()))) @@ -73,13 +71,14 @@ func Run(startInTray bool) { config := svc.Config() keepInTray = config.KeepRunningInTray startHidden = resolveStartHidden(startInTray, keepInTray) - applyTrayBehavior(a, w, keepInTray, false) + tray := &trayState{} + tray.apply(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, config.Theme) - content, recordStartup := newMainView(w, svc) + content, recordStartup := newMainView(w, svc, tray) w.SetContent(content) - serveSingleInstance(instanceListener, w) + serveSingleInstance(instanceListener, w, tray) if startHidden { // Autostart launches intentionally stay hidden, so "window shown" would be // a misleading metric. Record a separate startup event for the tray path diff --git a/src/ui/settings_view.go b/src/ui/settings_view.go index 600d88b..3070add 100644 --- a/src/ui/settings_view.go +++ b/src/ui/settings_view.go @@ -26,7 +26,7 @@ var settingsCaptions = []string{ "GoSentry", "Go", "Fyne", "Repository", } -func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { +func settingsView(w fyne.Window, svc *app.Service, tray *trayState) fyne.CanvasObject { // 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 @@ -49,17 +49,36 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { autostartStatus := widget.NewLabel("") trayRestartHint := widget.NewLabel("") trayRestartHint.Truncation = fyne.TextTruncateClip + // autostartCheckGen guards against an in-flight check's result landing after + // a newer one started (e.g. the user toggles a checkbox again before the + // first check's PowerShell call returns). Both the increment and the compare + // happen on the main/Fyne thread, so this needs no lock of its own. + var autostartCheckGen int refreshAutostartStatus := func() { if settingsPendingAutostart(startOnLogin, minimizeToTray, saved) { autostartStatus.SetText("Pending: save settings to apply") return } - ok, message := svc.AutostartStatus() - if ok { - autostartStatus.SetText("OK: " + message) - return - } - autostartStatus.SetText("Problem: " + message) + // svc.AutostartStatus() reaches readShortcut on Windows, which spawns + // powershell.exe and blocks on CombinedOutput() — hundreds of milliseconds + // of cold start. Running it off the main thread keeps that from freezing + // the window on construction and on every checkbox toggle. + autostartStatus.SetText("Checking...") + autostartCheckGen++ + gen := autostartCheckGen + go func() { + ok, message := svc.AutostartStatus() + fyne.Do(func() { + if gen != autostartCheckGen { + return + } + if ok { + autostartStatus.SetText("OK: " + message) + return + } + autostartStatus.SetText("Problem: " + message) + }) + }() } refreshTrayRestartHint := func(pending bool) { if pending { @@ -142,27 +161,16 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { settingsStatus := widget.NewLabel("") 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 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 zero (unlimited) or a positive number") - return - } - if strings.TrimSpace(jobsFile.Text) == "" { - settingsStatus.SetText("Jobs file is required") - return - } - if strings.TrimSpace(logsDir.Text) == "" { - settingsStatus.SetText("Logs directory is required") - return - } - timeout, err := strconv.Atoi(strings.TrimSpace(defaultTimeout.Text)) - if err != nil || timeout < 0 { - settingsStatus.SetText("Default timeout must not be negative (0 = no timeout)") + // Only the parse itself happens here: a numeric field has to become an int + // before it can go into a domain.Config at all. Everything else — required + // fields, negative numbers, valid enum values — is Service.UpdateSettings' + // job (see app.validateConfig), so its error is what the user sees rather + // than a second copy of the same rules with different wording. + files, filesErr := strconv.Atoi(strings.TrimSpace(maxLogFiles.Text)) + days, daysErr := strconv.Atoi(strings.TrimSpace(maxLogAgeDays.Text)) + timeout, timeoutErr := strconv.Atoi(strings.TrimSpace(defaultTimeout.Text)) + if filesErr != nil || daysErr != nil || timeoutErr != nil { + settingsStatus.SetText("Max log files, max log age days, and default timeout must be numbers") return } // Build the new config from the form and hand it to the Service, which @@ -196,7 +204,7 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { return } refreshAutostartStatus() - applyTrayBehavior(fyne.CurrentApp(), w, config.KeepRunningInTray, true) + tray.apply(fyne.CurrentApp(), w, config.KeepRunningInTray, true) if previousKeepInTray != config.KeepRunningInTray { trayRestartHint.SetText(trayRestartHintText) } else { diff --git a/src/ui/singleinstance.go b/src/ui/singleinstance.go index 67fdd64..fd625de 100644 --- a/src/ui/singleinstance.go +++ b/src/ui/singleinstance.go @@ -34,11 +34,14 @@ func acquireSingleInstance(showExisting bool) (net.Listener, bool) { // If the port is unavailable but does not answer as GoSentry, continue // startup instead of making the application impossible to open because of an // unrelated local listener. In the normal duplicate-start case the dial above - // succeeds and this process exits after waking the first instance. + // succeeds and this process exits after waking the first instance. The + // consequence of this fallback — two schedulers able to run against the same + // jobs.json and logs directory — is recorded in STANDARDS.md alongside the + // unauthenticated nature of this same port. return nil, true } -func serveSingleInstance(listener net.Listener, w fyne.Window) { +func serveSingleInstance(listener net.Listener, w fyne.Window, tray *trayState) { if listener == nil { return } @@ -56,7 +59,7 @@ func serveSingleInstance(listener net.Listener, w fyne.Window) { // Accept runs on its own goroutine, so focusing the window must be // marshaled onto the main thread like every other widget update. fyne.Do(func() { - mainWindowHidden = false + tray.hidden = false w.Show() w.RequestFocus() }) diff --git a/src/ui/tray.go b/src/ui/tray.go index 544f71f..775b9b5 100644 --- a/src/ui/tray.go +++ b/src/ui/tray.go @@ -10,16 +10,19 @@ import ( fynedesktop "fyne.io/fyne/v2/driver/desktop" ) -// systemTrayRegistered tracks whether this process registered a tray icon at -// launch. Fyne cannot add or remove the icon mid-session, so toggling -// KeepRunningInTray in Settings updates close behavior immediately and shows a -// restart hint for the icon itself. -var systemTrayRegistered bool - -// mainWindowHidden tracks whether the primary window was hidden via the tray -// close intercept. Fyne exposes no Window.Visible API, so the flag drives the -// reveal-on-tray-disable path in applyTrayBehavior. -var mainWindowHidden bool +// trayState tracks the two pieces of tray-related process state that Fyne +// itself does not expose: whether this process has registered the tray icon +// (Fyne cannot add or remove it mid-session, so toggling KeepRunningInTray in +// Settings updates close behavior immediately but shows a restart hint for the +// icon itself) and whether the primary window is currently hidden via the tray +// close intercept (Fyne exposes no Window.Visible API). Run owns one instance +// and passes it to every call site of apply — settingsView's Save handler is +// the other one — so the coupling between them is explicit instead of hidden +// behind package-level globals that no test can reset. +type trayState struct { + registered bool + hidden bool +} const trayRestartHintText = "Restart GoSentry for the tray icon change to take effect." @@ -27,23 +30,23 @@ func resolveStartHidden(cliStartInTray, keepInTray bool) bool { return domain.ResolveStartHidden(cliStartInTray, keepInTray) } -// applyTrayBehavior configures window close handling for KeepRunningInTray. -// When revealIfHidden is true and the tray is off, a hidden window is shown so -// the user can still reach the app after disabling the tray mid-session. -func applyTrayBehavior(a fyne.App, w fyne.Window, keepInTray bool, revealIfHidden bool) { - if keepInTray && !systemTrayRegistered { - registerSystemTray(a, w) - systemTrayRegistered = true +// apply configures window close handling for KeepRunningInTray. When +// revealIfHidden is true and the tray is off, a hidden window is shown so the +// user can still reach the app after disabling the tray mid-session. +func (t *trayState) apply(a fyne.App, w fyne.Window, keepInTray bool, revealIfHidden bool) { + if keepInTray && !t.registered { + t.registerSystemTray(a, w) + t.registered = true } - setWindowCloseBehavior(w, keepInTray) - if !keepInTray && revealIfHidden && mainWindowHidden { - mainWindowHidden = false + t.setWindowCloseBehavior(w, keepInTray) + if !keepInTray && revealIfHidden && t.hidden { + t.hidden = false w.Show() w.RequestFocus() } } -func registerSystemTray(a fyne.App, w fyne.Window) { +func (t *trayState) registerSystemTray(a fyne.App, w fyne.Window) { desk, ok := a.(fynedesktop.App) if !ok { // Not every Fyne driver exposes desktop tray features. Returning silently @@ -74,7 +77,7 @@ func registerSystemTray(a fyne.App, w fyne.Window) { quit.IsQuit = true menu := fyne.NewMenu("GoSentry", fyne.NewMenuItem("Show", func() { - mainWindowHidden = false + t.hidden = false w.Show() w.RequestFocus() }), @@ -85,17 +88,17 @@ func registerSystemTray(a fyne.App, w fyne.Window) { desk.SetSystemTrayWindow(w) } -func setWindowCloseBehavior(w fyne.Window, keepInTray bool) { +func (t *trayState) setWindowCloseBehavior(w fyne.Window, keepInTray bool) { if keepInTray { w.SetCloseIntercept(func() { // Closing hides the window instead of quitting because scheduler tools are // expected to keep working in the background. The explicit Quit tray item // remains the way to stop the process. - mainWindowHidden = true + t.hidden = true w.Hide() }) return } - mainWindowHidden = false + t.hidden = false w.SetCloseIntercept(nil) }