From 6621fc5c9ba4144bf52a6d8707768c2d58542d8a Mon Sep 17 00:00:00 2001 From: Reese Norris Date: Tue, 28 Jul 2026 23:23:24 -0400 Subject: [PATCH] fix: address review feedback for Client Setup GUI Protect FormState under mutex for debounced dry-plan; stop plan timer on window close; run Plan/Apply/Revert/Launch off the UI thread; do not persist public-VATSIM override across sessions; add BuildInstall tests. --- cmd/openfsd-client/gui/app.go | 5 + cmd/openfsd-client/gui/install_test.go | 213 ++++++++++++++ cmd/openfsd-client/gui/legal.go | 5 +- cmd/openfsd-client/gui/model.go | 4 +- cmd/openfsd-client/gui/settings.go | 44 +-- cmd/openfsd-client/gui/window.go | 370 +++++++++++++++++-------- 6 files changed, 499 insertions(+), 142 deletions(-) create mode 100644 cmd/openfsd-client/gui/install_test.go diff --git a/cmd/openfsd-client/gui/app.go b/cmd/openfsd-client/gui/app.go index b0d1ee1..23a8c2d 100644 --- a/cmd/openfsd-client/gui/app.go +++ b/cmd/openfsd-client/gui/app.go @@ -33,10 +33,15 @@ func Run(engine *clientinject.Engine) error { ctrl := newController(engine, a, w) w.SetContent(ctrl.buildUI()) + // Stop debounced plan timer and ignore late fyne.Do updates after close. + w.SetOnClosed(func() { + ctrl.stop() + }) ctrl.loadSettingsAndRefresh() // Prefer showing; on headless Fyne may panic or fail — caller can recover. w.ShowAndRun() + ctrl.stop() return nil } diff --git a/cmd/openfsd-client/gui/install_test.go b/cmd/openfsd-client/gui/install_test.go new file mode 100644 index 0000000..3045595 --- /dev/null +++ b/cmd/openfsd-client/gui/install_test.go @@ -0,0 +1,213 @@ +package gui + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/renorris/openfsd/internal/clientinject" +) + +// installFakeAdapter is a minimal Adapter for BuildInstall / FirstDetectPath tests. +type installFakeAdapter struct { + id string + name string + cands []clientinject.InstallCandidate +} + +func (a *installFakeAdapter) ClientID() string { return a.id } +func (a *installFakeAdapter) DisplayName() string { return a.name } +func (a *installFakeAdapter) SupportedProfiles() []string { return []string{"fake-1"} } +func (a *installFakeAdapter) Discover(context.Context) ([]clientinject.InstallCandidate, error) { + return append([]clientinject.InstallCandidate(nil), a.cands...), nil +} +func (a *installFakeAdapter) Verify(clientinject.Install, *clientinject.Profile) error { + return nil +} +func (a *installFakeAdapter) EndpointConstraints(*clientinject.Profile) []clientinject.Constraint { + return nil +} +func (a *installFakeAdapter) Plan(clientinject.Install, *clientinject.Profile, clientinject.Endpoints) (*clientinject.Plan, error) { + return &clientinject.Plan{}, nil +} +func (a *installFakeAdapter) Apply(context.Context, *clientinject.Plan, clientinject.FileWriter) error { + return nil +} +func (a *installFakeAdapter) HealthCheck(clientinject.Install, clientinject.Endpoints) error { + return nil +} +func (a *installFakeAdapter) LaunchArgs(clientinject.Install, clientinject.Endpoints) []string { + return nil +} + +func TestBuildInstall_FromProfileRelativePath(t *testing.T) { + root := t.TempDir() + peRel := "app.exe" + cfgRel := "config.xml" + if err := os.WriteFile(filepath.Join(root, peRel), []byte("MZ"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, cfgRel), []byte(""), 0o644); err != nil { + t.Fatal(err) + } + + store := clientinject.NewProfileStore() + if err := store.Add(&clientinject.Profile{ + SchemaVersion: 2, + ProfileID: "fake-1", + ClientID: "fake", + PrimaryBinary: clientinject.PrimaryBinarySpec{RelativePath: peRel}, + ConfigFiles: []clientinject.ConfigFileSpec{{RelativePath: cfgRel}}, + }); err != nil { + t.Fatal(err) + } + eng := clientinject.NewEngine(store) // no adapter — profile fallback path + install := BuildInstall(eng, "fake", root) + if install.PrimaryPE != filepath.Join(root, peRel) { + t.Fatalf("PrimaryPE=%q", install.PrimaryPE) + } + if len(install.ConfigPaths) != 1 || install.ConfigPaths[0] != filepath.Join(root, cfgRel) { + t.Fatalf("ConfigPaths=%v", install.ConfigPaths) + } + if install.ClientID != "fake" || install.RootDir != filepath.Clean(root) { + t.Fatalf("%+v", install) + } +} + +func TestBuildInstall_DiscoverMatch(t *testing.T) { + root := t.TempDir() + pe := filepath.Join(root, "client.exe") + cfg := filepath.Join(root, "cfg.xml") + if err := os.WriteFile(pe, []byte("MZ"), 0o644); err != nil { + t.Fatal(err) + } + fake := &installFakeAdapter{ + id: "fake", + name: "Fake", + cands: []clientinject.InstallCandidate{{ + ClientID: "fake", + RootDir: root, + PrimaryPE: pe, + ConfigPaths: []string{cfg}, + DisplayHint: "test install", + }}, + } + eng := clientinject.NewEngine(clientinject.NewProfileStore(), fake) + install := BuildInstall(eng, "fake", root) + if install.PrimaryPE != pe { + t.Fatalf("PrimaryPE=%q want %q", install.PrimaryPE, pe) + } + if len(install.ConfigPaths) != 1 || install.ConfigPaths[0] != cfg { + t.Fatalf("ConfigPaths=%v", install.ConfigPaths) + } +} + +func TestBuildInstall_ManifestSeed(t *testing.T) { + root := t.TempDir() + peRel := "app.exe" + if err := os.WriteFile(filepath.Join(root, peRel), []byte("MZ"), 0o644); err != nil { + t.Fatal(err) + } + store := clientinject.NewProfileStore() + if err := store.Add(&clientinject.Profile{ + SchemaVersion: 2, + ProfileID: "fake-1", + ClientID: "fake", + PrimaryBinary: clientinject.PrimaryBinarySpec{RelativePath: peRel, SHA1: "abc"}, + }); err != nil { + t.Fatal(err) + } + // Write a minimal inject manifest. + m := clientinject.Manifest{ + ClientID: "fake", + ProfileID: "fake-1", + PESHA1: "deadbeefcafebabe", + Status: clientinject.ManifestStatusApplied, + InstallRoot: root, + } + data, err := json.Marshal(m) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(clientinject.ManifestPath(root), data, 0o644); err != nil { + t.Fatal(err) + } + + eng := clientinject.NewEngine(store) + install := BuildInstall(eng, "fake", root) + if install.ProfileID != "fake-1" { + t.Fatalf("ProfileID=%q", install.ProfileID) + } + if install.HashSHA1 != "deadbeefcafebabe" { + t.Fatalf("HashSHA1=%q", install.HashSHA1) + } +} + +func TestFirstDetectPath(t *testing.T) { + root := t.TempDir() + pe := filepath.Join(root, "x.exe") + fake := &installFakeAdapter{ + id: "fake", + cands: []clientinject.InstallCandidate{{ + ClientID: "fake", + RootDir: root, + PrimaryPE: pe, + }}, + } + eng := clientinject.NewEngine(nil, fake) + cand, ok := FirstDetectPath(eng, "fake") + if !ok || cand.RootDir != root { + t.Fatalf("ok=%v cand=%+v", ok, cand) + } + if _, ok := FirstDetectPath(eng, "missing"); ok { + t.Fatal("expected no candidate") + } + if _, ok := FirstDetectPath(nil, "fake"); ok { + t.Fatal("nil engine") + } +} + +func TestSettingsDoesNotPersistPublicVATSIMAck(t *testing.T) { + f := FormState{ + ClientID: "vpilot", + InstallPath: "/x", + WebBaseURL: "https://auth.vatsim.net", + FSDHost: "h", + UnderstandPublicVATSIM: true, + } + s := SettingsFromForm(f) + // Round-trip via JSON as saved on disk. + dir := t.TempDir() + path := filepath.Join(dir, "settings.json") + if err := SaveSettings(path, s); err != nil { + t.Fatal(err) + } + // Ensure raw JSON does not contain the ack key. + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(raw) != "" { + var m map[string]any + if err := json.Unmarshal(raw, &m); err != nil { + t.Fatal(err) + } + if _, ok := m["understand_public_vatsim"]; ok { + t.Fatalf("should not persist understand_public_vatsim: %s", raw) + } + } + got, err := LoadSettings(path) + if err != nil { + t.Fatal(err) + } + var f2 FormState + got.ApplyToForm(&f2) + if f2.UnderstandPublicVATSIM { + t.Fatal("ApplyToForm must not restore public VATSIM ack") + } + if f2.WebBaseURL != f.WebBaseURL { + t.Fatalf("WebBaseURL=%q", f2.WebBaseURL) + } +} diff --git a/cmd/openfsd-client/gui/legal.go b/cmd/openfsd-client/gui/legal.go index 4aa3f08..5a8bcfb 100644 --- a/cmd/openfsd-client/gui/legal.go +++ b/cmd/openfsd-client/gui/legal.go @@ -8,8 +8,9 @@ const LegalApplyBanner = "For private openfsd networks you are authorized to use "Do not use this tool to reconfigure clients for the public VATSIM network." // PublicVATSIMHosts is the known public VATSIM host set used for soft warnings -// when WebBaseURL (or related endpoints) point at production VATSIM services. -// Matching does not hard-block Apply; the user may override with "I understand". +// when WebBaseURL's host matches (design: gate on Web base only; FSD/AFV hosts +// are not checked here). Matching does not hard-block Apply; the user may +// override with "I understand" for the current session only. var PublicVATSIMHosts = map[string]struct{}{ "auth.vatsim.net": {}, "status.vatsim.net": {}, diff --git a/cmd/openfsd-client/gui/model.go b/cmd/openfsd-client/gui/model.go index 2f8bcab..5df721b 100644 --- a/cmd/openfsd-client/gui/model.go +++ b/cmd/openfsd-client/gui/model.go @@ -129,8 +129,8 @@ func IsPublicVATSIMHost(hostOrURL string) bool { return ok } -// PublicVATSIMWarning returns a non-empty warning when WebBaseURL points at a -// known public VATSIM host. +// PublicVATSIMWarning returns a non-empty warning when WebBaseURL's host is in +// PublicVATSIMHosts. Only WebBaseURL is gated (not FSDHost / AFVBaseURL). func PublicVATSIMWarning(webBase string) string { host := WebBaseHost(webBase) if host == "" || !IsPublicVATSIMHost(host) { diff --git a/cmd/openfsd-client/gui/settings.go b/cmd/openfsd-client/gui/settings.go index c5d9c06..c5a08c4 100644 --- a/cmd/openfsd-client/gui/settings.go +++ b/cmd/openfsd-client/gui/settings.go @@ -9,36 +9,38 @@ import ( // Settings is the persisted user preferences (paths + endpoints only). // Never store passwords or network credentials. +// UnderstandPublicVATSIM is intentionally NOT persisted — every session must +// re-acknowledge a public VATSIM WebBaseURL soft warning. type Settings struct { - ClientID string `json:"client_id,omitempty"` - InstallPath string `json:"install_path,omitempty"` - WebBaseURL string `json:"web_base_url,omitempty"` - FSDHost string `json:"fsd_host,omitempty"` - FSDPort int `json:"fsd_port,omitempty"` - FSDServerName string `json:"fsd_server_name,omitempty"` - AFVBaseURL string `json:"afv_base_url,omitempty"` - ForceDisableAFV bool `json:"force_disable_afv,omitempty"` - PreferShortJWTPath bool `json:"prefer_short_jwt_path,omitempty"` - UnderstandPublicVATSIM bool `json:"understand_public_vatsim,omitempty"` + ClientID string `json:"client_id,omitempty"` + InstallPath string `json:"install_path,omitempty"` + WebBaseURL string `json:"web_base_url,omitempty"` + FSDHost string `json:"fsd_host,omitempty"` + FSDPort int `json:"fsd_port,omitempty"` + FSDServerName string `json:"fsd_server_name,omitempty"` + AFVBaseURL string `json:"afv_base_url,omitempty"` + ForceDisableAFV bool `json:"force_disable_afv,omitempty"` + PreferShortJWTPath bool `json:"prefer_short_jwt_path,omitempty"` } // SettingsFromForm copies persistable fields from FormState. +// Does not include UnderstandPublicVATSIM (session-only ack). func SettingsFromForm(f FormState) Settings { return Settings{ - ClientID: f.ClientID, - InstallPath: f.InstallPath, - WebBaseURL: f.WebBaseURL, - FSDHost: f.FSDHost, - FSDPort: f.FSDPort, - FSDServerName: f.FSDServerName, - AFVBaseURL: f.AFVBaseURL, - ForceDisableAFV: f.ForceDisableAFV, - PreferShortJWTPath: f.PreferShortJWTPath, - UnderstandPublicVATSIM: f.UnderstandPublicVATSIM, + ClientID: f.ClientID, + InstallPath: f.InstallPath, + WebBaseURL: f.WebBaseURL, + FSDHost: f.FSDHost, + FSDPort: f.FSDPort, + FSDServerName: f.FSDServerName, + AFVBaseURL: f.AFVBaseURL, + ForceDisableAFV: f.ForceDisableAFV, + PreferShortJWTPath: f.PreferShortJWTPath, } } // ApplyToForm merges settings into form (non-empty / meaningful fields). +// Never sets UnderstandPublicVATSIM — always leave false for a fresh session. func (s Settings) ApplyToForm(f *FormState) { if f == nil { return @@ -64,7 +66,7 @@ func (s Settings) ApplyToForm(f *FormState) { } f.ForceDisableAFV = s.ForceDisableAFV f.PreferShortJWTPath = s.PreferShortJWTPath - f.UnderstandPublicVATSIM = s.UnderstandPublicVATSIM + f.UnderstandPublicVATSIM = false } // DefaultConfigDir returns OS user config dir + openfsd-client. diff --git a/cmd/openfsd-client/gui/window.go b/cmd/openfsd-client/gui/window.go index fc4e579..180c7ab 100644 --- a/cmd/openfsd-client/gui/window.go +++ b/cmd/openfsd-client/gui/window.go @@ -23,13 +23,22 @@ import ( // controller owns form widgets and wires them to the engine. // All layout is client-agnostic; per-client behavior flows from Adapter + Plan. +// +// Concurrency: c.mu protects form, closed, busy, planTimer, lastPlan, and +// preflight. Widget mutations run only on the Fyne UI thread (callbacks or +// fyne.Do). Heavy Plan/Apply/hash work runs on worker goroutines. type controller struct { eng *clientinject.Engine app fyne.App win fyne.Window - form FormState - mu sync.Mutex + mu sync.Mutex + form FormState + closed bool + busy bool // Apply/Revert/Launch in flight + planTimer *time.Timer + lastPlan *clientinject.Plan + preflight error slots []ClientSlot slotByLabel map[string]ClientSlot @@ -61,11 +70,6 @@ type controller struct { launchBtn *widget.Button logEntry *widget.Entry - - // debounce dry-plan - planTimer *time.Timer - lastPlan *clientinject.Plan - preflight error } func newController(eng *clientinject.Engine, a fyne.App, w fyne.Window) *controller { @@ -83,6 +87,38 @@ func newController(eng *clientinject.Engine, a fyne.App, w fyne.Window) *control return c } +// stop cancels debounced dry-plan work and marks the controller closed so late +// fyne.Do callbacks skip widget updates after window teardown. +func (c *controller) stop() { + c.mu.Lock() + defer c.mu.Unlock() + c.closed = true + if c.planTimer != nil { + c.planTimer.Stop() + c.planTimer = nil + } +} + +func (c *controller) isClosed() bool { + c.mu.Lock() + defer c.mu.Unlock() + return c.closed +} + +// formCopy returns a snapshot of FormState under the form mutex. +func (c *controller) formCopy() FormState { + c.mu.Lock() + defer c.mu.Unlock() + return c.form +} + +// updateForm mutates FormState under the form mutex (UI-thread writers). +func (c *controller) updateForm(fn func(*FormState)) { + c.mu.Lock() + defer c.mu.Unlock() + fn(&c.form) +} + func (c *controller) buildUI() fyne.CanvasObject { // --- Header --- title := widget.NewLabelWithStyle("openfsd Client Setup", fyne.TextAlignLeading, fyne.TextStyle{Bold: true}) @@ -103,18 +139,17 @@ func (c *controller) buildUI() fyne.CanvasObject { c.clientSelect = widget.NewSelect(labels, func(sel string) { slot, ok := c.slotByLabel[sel] if !ok || !slot.Enabled { - // Revert to previous enabled selection. c.syncClientSelectFromForm() c.appendLog("Client %q is not available yet.", sel) return } - c.form.ClientID = slot.ID + c.updateForm(func(f *FormState) { f.ClientID = slot.ID }) c.schedulePlan() }) if firstEnabled != "" { c.clientSelect.SetSelected(firstEnabled) if s, ok := c.slotByLabel[firstEnabled]; ok { - c.form.ClientID = s.ID + c.updateForm(func(f *FormState) { f.ClientID = s.ID }) } } @@ -122,7 +157,7 @@ func (c *controller) buildUI() fyne.CanvasObject { c.installEntry = widget.NewEntry() c.installEntry.SetPlaceHolder("Client install directory") c.installEntry.OnChanged = func(s string) { - c.form.InstallPath = s + c.updateForm(func(f *FormState) { f.InstallPath = s }) c.schedulePlan() } c.detectBtn = widget.NewButton("Detect", func() { @@ -151,14 +186,14 @@ func (c *controller) buildUI() fyne.CanvasObject { c.webBaseEntry = widget.NewEntry() c.webBaseEntry.SetPlaceHolder("https://fsd.example.com") c.webBaseEntry.OnChanged = func(s string) { - c.form.WebBaseURL = s + c.updateForm(func(f *FormState) { f.WebBaseURL = s }) c.updatePublicVATSIMHint() c.schedulePlan() } c.fsdHostEntry = widget.NewEntry() c.fsdHostEntry.SetPlaceHolder("fsd.example.com") c.fsdHostEntry.OnChanged = func(s string) { - c.form.FSDHost = s + c.updateForm(func(f *FormState) { f.FSDHost = s }) c.schedulePlan() } c.fsdPortEntry = widget.NewEntry() @@ -168,33 +203,33 @@ func (c *controller) buildUI() fyne.CanvasObject { if err != nil { return } - c.form.FSDPort = n + c.updateForm(func(f *FormState) { f.FSDPort = n }) c.schedulePlan() } c.fsdServerEntry = widget.NewEntry() c.fsdServerEntry.SetText("OPENFSD") c.fsdServerEntry.OnChanged = func(s string) { - c.form.FSDServerName = s + c.updateForm(func(f *FormState) { f.FSDServerName = s }) c.schedulePlan() } c.afvBaseEntry = widget.NewEntry() c.afvBaseEntry.SetPlaceHolder("https://voice.example.com (optional)") c.afvBaseEntry.OnChanged = func(s string) { - c.form.AFVBaseURL = s + c.updateForm(func(f *FormState) { f.AFVBaseURL = s }) c.schedulePlan() } c.forceNoVoice = widget.NewCheck("Force disable voice (-novoice)", func(v bool) { - c.form.ForceDisableAFV = v + c.updateForm(func(f *FormState) { f.ForceDisableAFV = v }) c.schedulePlan() }) c.preferShortJWT = widget.NewCheck("Prefer short JWT path (/j, …)", func(v bool) { - c.form.PreferShortJWTPath = v + c.updateForm(func(f *FormState) { f.PreferShortJWTPath = v }) c.schedulePlan() }) c.publicVATSIMLbl = widget.NewLabel("") c.publicVATSIMLbl.Wrapping = fyne.TextWrapWord c.publicVATSIMAck = widget.NewCheck("I understand (public VATSIM host override)", func(v bool) { - c.form.UnderstandPublicVATSIM = v + c.updateForm(func(f *FormState) { f.UnderstandPublicVATSIM = v }) c.refreshApplyEnabled() }) @@ -235,7 +270,6 @@ func (c *controller) buildUI() fyne.CanvasObject { c.logEntry.Wrapping = fyne.TextWrapWord c.logEntry.Disable() - // Assemble scrollable body body := container.NewVBox( header, widget.NewCard("Client", "", c.clientSelect), @@ -256,13 +290,13 @@ func (c *controller) buildUI() fyne.CanvasObject { widget.NewCard("Log", "", c.logEntry), ) - scroll := container.NewVScroll(body) - return scroll + return container.NewVScroll(body) } func (c *controller) syncClientSelectFromForm() { + id := c.formCopy().ClientID for _, s := range c.slots { - if s.Enabled && s.ID == c.form.ClientID { + if s.Enabled && s.ID == id { c.clientSelect.SetSelected(SlotLabel(s)) return } @@ -280,22 +314,26 @@ func (c *controller) loadSettingsAndRefresh() { c.appendLog("load settings: %v", err) return } - s.ApplyToForm(&c.form) - // Push into widgets without infinite OnChanged loops is fine; they schedule plan. - if c.form.ClientID != "" { + c.updateForm(func(f *FormState) { + s.ApplyToForm(f) + // Never restore public-VATSIM override across sessions. + f.UnderstandPublicVATSIM = false + }) + f := c.formCopy() + if f.ClientID != "" { c.syncClientSelectFromForm() } - c.installEntry.SetText(c.form.InstallPath) - c.webBaseEntry.SetText(c.form.WebBaseURL) - c.fsdHostEntry.SetText(c.form.FSDHost) - c.fsdPortEntry.SetText(FormatPortString(c.form.FSDPort)) - if c.form.FSDServerName != "" { - c.fsdServerEntry.SetText(c.form.FSDServerName) + c.installEntry.SetText(f.InstallPath) + c.webBaseEntry.SetText(f.WebBaseURL) + c.fsdHostEntry.SetText(f.FSDHost) + c.fsdPortEntry.SetText(FormatPortString(f.FSDPort)) + if f.FSDServerName != "" { + c.fsdServerEntry.SetText(f.FSDServerName) } - c.afvBaseEntry.SetText(c.form.AFVBaseURL) - c.forceNoVoice.SetChecked(c.form.ForceDisableAFV) - c.preferShortJWT.SetChecked(c.form.PreferShortJWTPath) - c.publicVATSIMAck.SetChecked(c.form.UnderstandPublicVATSIM) + c.afvBaseEntry.SetText(f.AFVBaseURL) + c.forceNoVoice.SetChecked(f.ForceDisableAFV) + c.preferShortJWT.SetChecked(f.PreferShortJWTPath) + c.publicVATSIMAck.SetChecked(false) c.updatePublicVATSIMHint() c.schedulePlan() c.appendLog("Loaded settings from %s", path) @@ -306,7 +344,7 @@ func (c *controller) saveSettings() { if err != nil { return } - if err := SaveSettings(path, SettingsFromForm(c.form)); err != nil { + if err := SaveSettings(path, SettingsFromForm(c.formCopy())); err != nil { c.appendLog("save settings: %v", err) return } @@ -324,7 +362,7 @@ func (c *controller) appendLog(format string, args ...any) { } func (c *controller) updatePublicVATSIMHint() { - warn := PublicVATSIMWarning(c.form.WebBaseURL) + warn := PublicVATSIMWarning(c.formCopy().WebBaseURL) if warn == "" { c.publicVATSIMLbl.SetText("") return @@ -335,18 +373,29 @@ func (c *controller) updatePublicVATSIMHint() { func (c *controller) schedulePlan() { c.mu.Lock() defer c.mu.Unlock() + if c.closed { + return + } if c.planTimer != nil { c.planTimer.Stop() } - // Debounce dry-plan on keystroke / form change. + // Debounce dry-plan on keystroke / form change (runs off UI thread). c.planTimer = time.AfterFunc(350*time.Millisecond, func() { c.runDryPlan() }) } +// runDryPlan executes Plan / preflight / hash on a worker goroutine, then +// marshals UI updates via fyne.Do. Form is snapshotted under c.mu. func (c *controller) runDryPlan() { - // Snapshot form under lock-free read (widgets already wrote form fields). + c.mu.Lock() + if c.closed { + c.mu.Unlock() + return + } f := c.form + c.mu.Unlock() + issues := ValidateForm(f) install := BuildInstall(c.eng, f.ClientID, f.InstallPath) @@ -356,7 +405,6 @@ func (c *controller) runDryPlan() { preflightErr = err } } - // Best-effort hash for fingerprint when PE readable. if install.HashSHA1 == "" && install.PrimaryPE != "" { if sum, err := fileSHA1OS(install.PrimaryPE); err == nil { install.HashSHA1 = sum @@ -374,15 +422,18 @@ func (c *controller) runDryPlan() { var planErr error if len(issues) == 0 && f.InstallPath != "" { plan, planErr = c.eng.Plan(context.Background(), install, f.Endpoints()) - if planErr != nil { - // Still show fingerprint; plan panel shows error. - } } - // Update UI on main thread. fyne.Do(func() { + c.mu.Lock() + if c.closed { + c.mu.Unlock() + return + } c.preflight = preflightErr c.lastPlan = plan + c.mu.Unlock() + c.fingerprint.SetText(FormatFingerprint(install, profileID, preflightErr)) if len(install.ConfigPaths) > 0 { c.configList.SetText("Config files:\n • " + strings.Join(install.ConfigPaths, "\n • ")) @@ -420,9 +471,24 @@ func (c *controller) runDryPlan() { } func (c *controller) refreshApplyEnabled() { - issues := ValidateForm(c.form) - ok, reason := CanApply(c.form, c.lastPlan, issues) - if c.preflight != nil && errors.Is(c.preflight, clientinject.ErrClientRunning) { + c.mu.Lock() + f := c.form + plan := c.lastPlan + preflight := c.preflight + busy := c.busy + c.mu.Unlock() + + if busy { + c.applyBtn.Disable() + c.revertBtn.Disable() + c.launchBtn.Disable() + c.applyBtn.SetText("Apply (working…)") + return + } + + issues := ValidateForm(f) + ok, reason := CanApply(f, plan, issues) + if preflight != nil && errors.Is(preflight, clientinject.ErrClientRunning) { ok = false reason = "client appears to be running — quit before Apply" } @@ -437,8 +503,7 @@ func (c *controller) refreshApplyEnabled() { } else { c.applyBtn.SetText("Apply") } - // Revert enabled when install path set - if strings.TrimSpace(c.form.InstallPath) == "" { + if strings.TrimSpace(f.InstallPath) == "" { c.revertBtn.Disable() c.launchBtn.Disable() } else { @@ -447,6 +512,13 @@ func (c *controller) refreshApplyEnabled() { } } +func (c *controller) setBusy(v bool) { + c.mu.Lock() + c.busy = v + c.mu.Unlock() + c.refreshApplyEnabled() +} + func truncate(s string, n int) string { if len(s) <= n { return s @@ -455,9 +527,10 @@ func truncate(s string, n int) string { } func (c *controller) onDetect() { - cand, ok := FirstDetectPath(c.eng, c.form.ClientID) + f := c.formCopy() + cand, ok := FirstDetectPath(c.eng, f.ClientID) if !ok { - c.appendLog("No install detected for %s — set path manually.", c.form.ClientID) + c.appendLog("No install detected for %s — set path manually.", f.ClientID) dialog.ShowInformation("Detect", "No install found for this client on this machine. Enter the install path manually.", c.win) return } @@ -466,12 +539,15 @@ func (c *controller) onDetect() { } func (c *controller) onApply() { - // Soft legal banner every Apply. + f := c.formCopy() msg := LegalApplyBanner - if warn := PublicVATSIMWarning(c.form.WebBaseURL); warn != "" { + if warn := PublicVATSIMWarning(f.WebBaseURL); warn != "" { msg += "\n\n" + warn } - if c.lastPlan != nil && len(c.lastPlan.Blockers) > 0 { + c.mu.Lock() + plan := c.lastPlan + c.mu.Unlock() + if plan != nil && len(plan.Blockers) > 0 { dialog.ShowError(fmt.Errorf("plan has blockers — fix endpoints first"), c.win) return } @@ -485,7 +561,7 @@ func (c *controller) onApply() { } func (c *controller) doApply() { - f := c.form + f := c.formCopy() issues := ValidateForm(f) if len(issues) > 0 { dialog.ShowError(fmt.Errorf("%s", issues[0].Message), c.win) @@ -495,34 +571,59 @@ func (c *controller) doApply() { dialog.ShowError(fmt.Errorf("public VATSIM host — check “I understand” or change Web base URL"), c.win) return } - install := BuildInstall(c.eng, f.ClientID, f.InstallPath) - plan, err := c.eng.Plan(context.Background(), install, f.Endpoints()) - if err != nil { - c.appendLog("plan: %v", err) - dialog.ShowError(err, c.win) - return - } - if len(plan.Blockers) > 0 { - c.appendLog("plan blocked: %s", strings.Join(plan.Blockers, "; ")) - dialog.ShowError(fmt.Errorf("plan blockers: %s", strings.Join(plan.Blockers, "; ")), c.win) - return - } - c.appendLog("Applying %d mutations…", len(plan.Mutations)) - res, err := c.eng.Apply(context.Background(), plan) - if err != nil { - c.appendLog("apply failed: %v", err) - dialog.ShowError(err, c.win) - c.schedulePlan() - return - } - c.saveSettings() - c.appendLog("Applied OK manifest=%s applied=%v", res.ManifestPath, res.Applied) - dialog.ShowInformation("Apply complete", fmt.Sprintf("Patched successfully.\nManifest: %s\nApplied: %v", res.ManifestPath, res.Applied), c.win) - c.schedulePlan() + + c.setBusy(true) + c.appendLog("Planning / applying…") + eng := c.eng + go func() { + install := BuildInstall(eng, f.ClientID, f.InstallPath) + plan, err := eng.Plan(context.Background(), install, f.Endpoints()) + if err != nil { + fyne.Do(func() { + if c.isClosed() { + return + } + c.setBusy(false) + c.appendLog("plan: %v", err) + dialog.ShowError(err, c.win) + }) + return + } + if len(plan.Blockers) > 0 { + fyne.Do(func() { + if c.isClosed() { + return + } + c.setBusy(false) + c.appendLog("plan blocked: %s", strings.Join(plan.Blockers, "; ")) + dialog.ShowError(fmt.Errorf("plan blockers: %s", strings.Join(plan.Blockers, "; ")), c.win) + }) + return + } + nMut := len(plan.Mutations) + res, err := eng.Apply(context.Background(), plan) + fyne.Do(func() { + if c.isClosed() { + return + } + c.setBusy(false) + if err != nil { + c.appendLog("apply failed: %v", err) + dialog.ShowError(err, c.win) + c.schedulePlan() + return + } + c.saveSettings() + c.appendLog("Applied OK manifest=%s applied=%v (%d mutations)", res.ManifestPath, res.Applied, nMut) + dialog.ShowInformation("Apply complete", fmt.Sprintf("Patched successfully.\nManifest: %s\nApplied: %v", res.ManifestPath, res.Applied), c.win) + c.schedulePlan() + }) + }() } func (c *controller) onRevert() { - root := strings.TrimSpace(c.form.InstallPath) + f := c.formCopy() + root := strings.TrimSpace(f.InstallPath) if root == "" { dialog.ShowError(fmt.Errorf("install path required"), c.win) return @@ -531,43 +632,80 @@ func (c *controller) onRevert() { if !yes { return } - if err := c.eng.Revert(context.Background(), filepath.Clean(root)); err != nil { - c.appendLog("revert: %v", err) - dialog.ShowError(err, c.win) - return - } - c.appendLog("Reverted %s", root) - dialog.ShowInformation("Revert complete", "Stock files restored from backup.", c.win) - c.schedulePlan() + c.setBusy(true) + c.appendLog("Reverting…") + eng := c.eng + rootClean := filepath.Clean(root) + go func() { + err := eng.Revert(context.Background(), rootClean) + fyne.Do(func() { + if c.isClosed() { + return + } + c.setBusy(false) + if err != nil { + c.appendLog("revert: %v", err) + dialog.ShowError(err, c.win) + return + } + c.appendLog("Reverted %s", rootClean) + dialog.ShowInformation("Revert complete", "Stock files restored from backup.", c.win) + c.schedulePlan() + }) + }() }, c.win) } func (c *controller) onLaunch() { - f := c.form - install := BuildInstall(c.eng, f.ClientID, f.InstallPath) - a, ok := c.eng.Adapters[f.ClientID] - if !ok { - dialog.ShowError(fmt.Errorf("no adapter for %q", f.ClientID), c.win) - return - } - args := a.LaunchArgs(install, f.Endpoints()) - pe := clientinject.AbsPrimaryPE(install) - if pe == "" { - dialog.ShowError(fmt.Errorf("no primary PE path"), c.win) - return - } - c.appendLog("Launch: %s %s", pe, strings.Join(args, " ")) - cmd := exec.Command(pe, args...) - cmd.Dir = install.RootDir - if err := cmd.Start(); err != nil { - // Best-effort (Wine / non-Windows may fail). - c.appendLog("launch failed: %v (use printed args manually)", err) - dialog.ShowInformation("Launch", fmt.Sprintf("Could not start process:\n%v\n\nCommand:\n%s %s\n\nWorking directory:\n%s", - err, pe, strings.Join(args, " "), install.RootDir), c.win) - return - } - _ = cmd.Process.Release() - c.appendLog("Started pid (detached)") + f := c.formCopy() + c.setBusy(true) + eng := c.eng + go func() { + install := BuildInstall(eng, f.ClientID, f.InstallPath) + a, ok := eng.Adapters[f.ClientID] + if !ok { + fyne.Do(func() { + if c.isClosed() { + return + } + c.setBusy(false) + dialog.ShowError(fmt.Errorf("no adapter for %q", f.ClientID), c.win) + }) + return + } + args := a.LaunchArgs(install, f.Endpoints()) + pe := clientinject.AbsPrimaryPE(install) + if pe == "" { + fyne.Do(func() { + if c.isClosed() { + return + } + c.setBusy(false) + dialog.ShowError(fmt.Errorf("no primary PE path"), c.win) + }) + return + } + cmd := exec.Command(pe, args...) + cmd.Dir = install.RootDir + startErr := cmd.Start() + if startErr == nil { + _ = cmd.Process.Release() + } + fyne.Do(func() { + if c.isClosed() { + return + } + c.setBusy(false) + c.appendLog("Launch: %s %s", pe, strings.Join(args, " ")) + if startErr != nil { + c.appendLog("launch failed: %v (use printed args manually)", startErr) + dialog.ShowInformation("Launch", fmt.Sprintf("Could not start process:\n%v\n\nCommand:\n%s %s\n\nWorking directory:\n%s", + startErr, pe, strings.Join(args, " "), install.RootDir), c.win) + return + } + c.appendLog("Started pid (detached)") + }) + }() } func fileSHA1OS(path string) (string, error) { @@ -576,7 +714,5 @@ func fileSHA1OS(path string) (string, error) { if err != nil { return "", err } - // local hash to avoid exporting engine helper - sum := sha1SumHex(data) - return sum, nil + return sha1SumHex(data), nil }