diff --git a/internal/config/config.go b/internal/config/config.go index e693592..25f3296 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -45,6 +45,13 @@ type VM struct { SSHPort int `toml:"sshport"` Recipes []string `toml:"recipes"` + // Display is the user's screen preference: "" or "auto" (default), + // "window", or "vnc". core.validateDisplay is the single place that + // checks the value; empty means an old vm.toml predates this field, so + // it must read the same as "auto". qemu.DisplayKind is the rule that + // turns this into DisplayWindow or DisplayVNC. + Display string `toml:"display"` + // Forwards are user-declared TCP ports forwarded from host to guest, in // addition to the SSHPort forward that always exists. internal/qemu.Args // renders them into qemu's -netdev hostfwd= clauses. Changes apply at diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 2dc68a6..20f134f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -28,6 +28,7 @@ func TestSaveLoadRoundtrip(t *testing.T) { SSHPort: 2201, Recipes: []string{"xfce"}, Forwards: []PortForward{{HostPort: 8080, GuestPort: 80}}, + Display: "window", Dir: filepath.Join(Root(), "alpine-live"), } if err := want.Save(); err != nil { @@ -43,6 +44,30 @@ func TestSaveLoadRoundtrip(t *testing.T) { } } +// TestSaveLoadRoundtripEmptyDisplayMeansAuto covers a vm.toml with no display +// preference, the shape every pre-existing VM has. Load must leave it "", +// not default it to a literal "auto": core.validateDisplay and +// qemu.DisplayKind both already treat "" the same as "auto". +func TestSaveLoadRoundtripEmptyDisplayMeansAuto(t *testing.T) { + t.Setenv("STOAT_HOME", t.TempDir()) + if err := EnsureRoot(); err != nil { + t.Fatal(err) + } + + want := &VM{Name: "plain", Mode: "live", SSHPort: 2203, Dir: filepath.Join(Root(), "plain")} + if err := want.Save(); err != nil { + t.Fatal(err) + } + + got, err := Load("plain") + if err != nil { + t.Fatal(err) + } + if got.Display != "" { + t.Errorf("Display = %q, want empty", got.Display) + } +} + func TestSaveLoadRoundtripCloudVM(t *testing.T) { t.Setenv("STOAT_HOME", t.TempDir()) if err := EnsureRoot(); err != nil { diff --git a/internal/core/core.go b/internal/core/core.go index f39342f..243bce6 100644 --- a/internal/core/core.go +++ b/internal/core/core.go @@ -68,6 +68,11 @@ type Spec struct { Share string Recipes []string + // Display is the screen preference to record in vm.toml: "" or "auto" + // (default), "window", or "vnc". validateDisplay is the single check + // for the value; plan applies it. + Display string + // ConsolePassword: empty means config.DefaultConsolePassword, "random" // generates one, anything else is used verbatim. Written for the // cloudinit backend only; see config.VM.ConsolePassword for why no @@ -206,6 +211,10 @@ func plan(s Spec) (*config.VM, error) { return nil, err } + if err := validateDisplay(s.Display); err != nil { + return nil, err + } + port, err := config.FreePort() if err != nil { return nil, err @@ -232,6 +241,7 @@ func plan(s Spec) (*config.VM, error) { SSHPort: port, Recipes: s.Recipes, AllowExec: allowExec, + Display: s.Display, } if img.backend == "cloudinit" { diff --git a/internal/core/core_test.go b/internal/core/core_test.go index 5a8710a..4f422da 100644 --- a/internal/core/core_test.go +++ b/internal/core/core_test.go @@ -162,6 +162,30 @@ func TestPlanRefusesRelativeDiskSize(t *testing.T) { } } +func TestPlanRejectsInvalidDisplay(t *testing.T) { + dir := root(t) + haveImage(t, dir, "alpine-virt-3.24.1-x86_64.iso") + + _, err := plan(Spec{Name: "work", Image: "alpine-virt-3.24.1-x86_64.iso", Display: "fullscreen"}) + if !errors.Is(err, ErrInvalidSpec) { + t.Fatalf("err = %v, want ErrInvalidSpec", err) + } +} + +func TestPlanAcceptsEveryValidDisplay(t *testing.T) { + dir := root(t) + for _, pref := range []string{"", "auto", "window", "vnc"} { + haveImage(t, dir, "alpine-virt-3.24.1-x86_64.iso") + v, err := plan(Spec{Name: "work-" + pref, Image: "alpine-virt-3.24.1-x86_64.iso", Display: pref}) + if err != nil { + t.Fatalf("Display=%q: %v", pref, err) + } + if v.Display != pref { + t.Errorf("Display = %q, want %q", v.Display, pref) + } + } +} + func TestPlanRefusesDuplicateName(t *testing.T) { dir := root(t) haveImage(t, dir, "alpine-virt-3.24.1-x86_64.iso") diff --git a/internal/core/display.go b/internal/core/display.go index 6653d84..995dfdd 100644 --- a/internal/core/display.go +++ b/internal/core/display.go @@ -1,6 +1,10 @@ package core -import "github.com/novusedge/stoat/internal/qemu" +import ( + "fmt" + + "github.com/novusedge/stoat/internal/qemu" +) // Re-exported so a caller can branch on Display.Kind without importing // internal/qemu, which is otherwise entirely below this package. @@ -56,7 +60,19 @@ func DisplayKind(v VM, graphical bool) string { if v.State == StateBroken { return "" } - return qemu.DisplayKind(v.Mode, v.Installed, graphical) + return qemu.DisplayKind(v.Display, v.Mode, v.Installed, graphical) +} + +// validateDisplay checks a config.VM.Display candidate. "" and "auto" both +// mean the installer-console default; anything but those two and "window"/ +// "vnc" is rejected so a typo in vm.toml or an MCP call fails loudly instead +// of silently falling back to auto. +func validateDisplay(pref string) error { + switch pref { + case "", "auto", qemu.DisplayWindow, qemu.DisplayVNC: + return nil + } + return fmt.Errorf("%w: display must be one of \"\", auto, window, vnc, not %q", ErrInvalidSpec, pref) } // DisplayFor is DisplayKind plus the lookup of a VNC viewer installed on this diff --git a/internal/core/display_test.go b/internal/core/display_test.go index 8ce39ba..86a7886 100644 --- a/internal/core/display_test.go +++ b/internal/core/display_test.go @@ -119,3 +119,17 @@ func TestDisplayKindIsPure(t *testing.T) { t.Errorf("DisplayKind = %q, want %q on a host with no session", got, DisplayVNC) } } + +// A per-VM preference overrides the mode/installed default, but not the +// host's veto. +func TestDisplayKindHonoursPreference(t *testing.T) { + if got := DisplayKind(VM{Mode: "cloud", Display: "window"}, true); got != DisplayWindow { + t.Errorf("Display=window: got %q, want %q", got, DisplayWindow) + } + if got := DisplayKind(VM{Mode: "disk", Installed: false, Display: "vnc"}, true); got != DisplayVNC { + t.Errorf("Display=vnc: got %q, want %q", got, DisplayVNC) + } + if got := DisplayKind(VM{Mode: "cloud", Display: "window"}, false); got != DisplayVNC { + t.Errorf("Display=window on a headless host: got %q, want %q", got, DisplayVNC) + } +} diff --git a/internal/core/update.go b/internal/core/update.go index 3475d35..be2914b 100644 --- a/internal/core/update.go +++ b/internal/core/update.go @@ -60,6 +60,11 @@ type Patch struct { // looks written; this field is the escape hatch when that guess is wrong, // so it is mutable, unlike Name/OS/Backend/Mode. Installed *bool + + // Display is the screen preference: "" or "auto", "window", or "vnc". + // Safe: it only changes which -display argument qemu.Args builds next + // start, nothing about the running process. + Display *string } // checkImmutable reports ErrImmutableField, naming the field, when a Patch sets @@ -131,6 +136,12 @@ func Update(name string, p Patch) (VM, error) { if p.Installed != nil { v.Installed = *p.Installed } + if p.Display != nil { + if err := validateDisplay(*p.Display); err != nil { + return VM{}, err + } + v.Display = *p.Display + } if p.SSHPort != nil && *p.SSHPort != v.SSHPort { if err := validateSSHPort(v, *p.SSHPort); err != nil { diff --git a/internal/core/update_test.go b/internal/core/update_test.go index 3972756..5d1a42a 100644 --- a/internal/core/update_test.go +++ b/internal/core/update_test.go @@ -107,6 +107,39 @@ func TestUpdateRejectsInvalidRAMAndCPUs(t *testing.T) { } } +func TestUpdateDisplay(t *testing.T) { + root(t) + v := &config.VM{Name: "work", Mode: "disk", RAM: 1024, CPUs: 1, SSHPort: 2200} + if err := v.Save(); err != nil { + t.Fatal(err) + } + got, err := Update("work", Patch{Display: ptr("window")}) + if err != nil { + t.Fatal(err) + } + if got.Display != "window" { + t.Errorf("Display = %q, want window", got.Display) + } + reloaded, err := config.Load("work") + if err != nil { + t.Fatal(err) + } + if reloaded.Display != "window" { + t.Errorf("not persisted: Display = %q", reloaded.Display) + } +} + +func TestUpdateRejectsInvalidDisplay(t *testing.T) { + root(t) + v := &config.VM{Name: "work", Mode: "disk", RAM: 1024, CPUs: 1, SSHPort: 2200} + if err := v.Save(); err != nil { + t.Fatal(err) + } + if _, err := Update("work", Patch{Display: ptr("fullscreen")}); !errors.Is(err, ErrInvalidSpec) { + t.Errorf("err = %v, want ErrInvalidSpec", err) + } +} + func TestUpdateRecipesReplacesWholesale(t *testing.T) { root(t) v := &config.VM{Name: "work", Mode: "live", RAM: 1024, CPUs: 1, SSHPort: 2200, Recipes: []string{"old.yaml"}} diff --git a/internal/core/vm.go b/internal/core/vm.go index 2b29c2a..866127b 100644 --- a/internal/core/vm.go +++ b/internal/core/vm.go @@ -112,6 +112,11 @@ type VM struct { SSHPort int SSHUser string + // Display is the user's screen preference: "" or "auto" (default), + // "window", or "vnc". DisplayKind and DisplayFor turn this, plus the + // host's graphical session, into where the screen actually lands. + Display string + // ISO and Base are plain vm.toml facts. They are here because a caller // asking what a VM IS needs them, and without them the TUI would have to // keep a second config.Load beside every core.Get. @@ -210,6 +215,7 @@ func fromConfig(v *config.VM) VM { Applied: applied(v.Applied), SSHPort: v.SSHPort, SSHUser: v.SSHUser, + Display: v.Display, ISO: v.ISO, Base: v.Base, ConsolePassword: v.ConsolePassword, diff --git a/internal/qemu/args.go b/internal/qemu/args.go index 3659043..38ebb87 100644 --- a/internal/qemu/args.go +++ b/internal/qemu/args.go @@ -19,35 +19,43 @@ const ( DisplayVNC = "vnc" ) -// DisplayKind is the rule, stated over the three facts it actually depends on -// rather than over a *config.VM: core.VM is a different type that carries the -// first two, and it must ask this question rather than restate it. +// DisplayKind is the rule, stated over the four facts it actually depends on +// rather than over a *config.VM: core.VM is a different type that carries +// pref/mode/installed, and it must ask this question rather than restate it. // -// Only an uninstalled disk-mode VM wants a real window, because its OS -// installer draws to VGA rather than the serial console and a human has to -// drive it. live, cloud and installed disk VMs reach ssh with no console -// interaction, so their screen goes to the VNC socket instead. +// pref is config.VM.Display: "window" or "vnc" pins the surface outright, +// "" or "auto" runs the installer-console default below. Only an uninstalled +// disk-mode VM wants a window under that default, because its OS installer +// draws to VGA rather than the serial console and a human has to drive it. +// live, cloud and installed disk VMs reach ssh with no console interaction, +// so their screen goes to the VNC socket unless pref overrides it. // -// graphical is the host's veto, and it is a veto rather than a preference: +// graphical is the host's veto, checked before pref and unconditional: even +// a "window" preference falls back to VNC on a host with no display server. // -display gtk needs a display server on the HOST, and qemu does not degrade // when there is none, it exits 1 (measured: "gtk initialization failed", and // with gl=on it fails one option earlier still, on "OpenGL is not supported by -// display backend 'gtk'"). Before this argument existed, a fresh disk VM on a +// display backend 'gtk'"). Before this veto existed, a fresh disk VM on a // headless host could not be started at all, and the install could not be // completed by any route. It falls back to VNC instead, which serves the exact // same VGA framebuffer over a socket the user can reach from a machine that // does have a screen. Whoever answers this must answer it for the host, not // for the VM: see GraphicalSession. -// -// This is deliberately blind to whether the guest has a desktop on it. An -// installed disk VM running XFCE would like a window and does not get one; see -// docs/troubleshooting.md. Widening that needs an explicit per-VM preference, -// not a looser predicate. -func DisplayKind(mode string, installed, graphical bool) string { - if mode == "disk" && !installed && graphical { +func DisplayKind(pref, mode string, installed, graphical bool) string { + if !graphical { + return DisplayVNC + } + switch pref { + case DisplayWindow: return DisplayWindow + case DisplayVNC: + return DisplayVNC + default: + if mode == "disk" && !installed { + return DisplayWindow + } + return DisplayVNC } - return DisplayVNC } // NeedsWindow reports whether this VM gets a real qemu window on a host whose @@ -56,7 +64,7 @@ func DisplayKind(mode string, installed, graphical bool) string { // Exported so the TUI can describe the right escape hatch (a GTK window vs. // the VNC socket) without duplicating this rule. func NeedsWindow(v *config.VM, graphical bool) bool { - return DisplayKind(v.Mode, v.Installed, graphical) == DisplayWindow + return DisplayKind(v.Display, v.Mode, v.Installed, graphical) == DisplayWindow } // WantsWindow reports whether this VM would use a window if the host could diff --git a/internal/qemu/display_test.go b/internal/qemu/display_test.go index b14ea5a..29694bd 100644 --- a/internal/qemu/display_test.go +++ b/internal/qemu/display_test.go @@ -25,26 +25,43 @@ func fakeBins(t *testing.T, names ...string) { func TestDisplayKind(t *testing.T) { for _, c := range []struct { + pref string mode string installed bool graphical bool want string }{ - // The install console: the one case a human MUST be able to look at, - // because setup-alpine cannot be driven any other way. - {"disk", false, true, DisplayWindow}, + // pref "" and "auto" both run the installer-console default. + {"", "disk", false, true, DisplayWindow}, + {"auto", "disk", false, true, DisplayWindow}, // The same VM on a host with no display server. qemu would exit 1 on // the window rather than fall back, so stoat falls back for it. - {"disk", false, false, DisplayVNC}, - {"disk", true, true, DisplayVNC}, - {"live", false, true, DisplayVNC}, - {"live", true, true, DisplayVNC}, - {"cloud", false, true, DisplayVNC}, - {"cloud", true, true, DisplayVNC}, - {"cloud", false, false, DisplayVNC}, + {"", "disk", false, false, DisplayVNC}, + {"auto", "disk", false, false, DisplayVNC}, + {"", "disk", true, true, DisplayVNC}, + {"", "live", false, true, DisplayVNC}, + {"", "live", true, true, DisplayVNC}, + {"", "cloud", false, true, DisplayVNC}, + {"", "cloud", true, true, DisplayVNC}, + {"", "cloud", false, false, DisplayVNC}, + + // "window" pins the surface, on every mode/installed combination, + // as long as the host has a session. + {"window", "live", false, true, DisplayWindow}, + {"window", "disk", true, true, DisplayWindow}, + {"window", "cloud", false, true, DisplayWindow}, + // "vnc" pins the other way, including the installer-console case + // the auto default would have given a window. + {"vnc", "disk", false, true, DisplayVNC}, + {"vnc", "live", true, true, DisplayVNC}, + + // graphical is a veto: it wins over "window" too, so a headless + // host never gets handed a qemu argv that makes qemu exit 1. + {"window", "disk", false, false, DisplayVNC}, + {"window", "live", true, false, DisplayVNC}, } { - if got := DisplayKind(c.mode, c.installed, c.graphical); got != c.want { - t.Errorf("DisplayKind(%q, %v, graphical=%v) = %q, want %q", c.mode, c.installed, c.graphical, got, c.want) + if got := DisplayKind(c.pref, c.mode, c.installed, c.graphical); got != c.want { + t.Errorf("DisplayKind(%q, %q, %v, graphical=%v) = %q, want %q", c.pref, c.mode, c.installed, c.graphical, got, c.want) } } } @@ -58,7 +75,7 @@ func TestDisplayKindReadsNoEnvironment(t *testing.T) { t.Setenv("WAYLAND_DISPLAY", "") t.Setenv("XDG_RUNTIME_DIR", "") t.Setenv(GraphicalEnv, "0") - if got := DisplayKind("disk", false, true); got != DisplayWindow { + if got := DisplayKind("", "disk", false, true); got != DisplayWindow { t.Errorf("DisplayKind = %q, want %q: the argument decides, not the environment", got, DisplayWindow) } } diff --git a/internal/tui/detail.go b/internal/tui/detail.go index 1b41f05..136cb23 100644 --- a/internal/tui/detail.go +++ b/internal/tui/detail.go @@ -200,6 +200,29 @@ func (m model) updateDetail(msg tea.Msg) (tea.Model, tea.Cmd) { m.detail.vm = updated cmd := m.showToast(fmt.Sprintf("%s installed=%v", updated.Name, updated.Installed), false) return m, tea.Batch(loadVMs, cmd) + case "d": + v := m.detail.vm + // "auto" writes back as "", config.VM.Display's own default for a + // vm.toml that never set a preference. + next := nextDisplayPref(v.Display) + stored := next + if stored == "auto" { + stored = "" + } + updated, err := core.Update(v.Name, core.Patch{Display: &stored}) + if err != nil { + cmd := m.showToast(err.Error(), true) + return m, cmd + } + m.detail.vm = updated + note := "display: " + displayPrefLabel(updated.Display) + " (applies at next start)" + if v.State == core.StateRunning { + // Matches edit.go's saveEdit note for a running VM: this pane + // builds no restart flow of its own. + note += "; restart to apply" + } + cmd := m.showToast(note, false) + return m, cmd case "s": v, err := m.detail.coreVM() if err != nil { @@ -370,9 +393,10 @@ func (m model) viewDetail() string { } facts.row("", "", effect) } - // qemu.DisplayKind takes mode, installed, and host graphical directly, not - // a *config.VM, so a core.VM caller can call it without going through - // qemu.NeedsWindow/WantsWindow, which need a *config.VM. + line("display", displayPrefLabel(v.Display)) + // qemu.DisplayKind takes pref, mode, installed, and host graphical + // directly, not a *config.VM, so a core.VM caller can call it without + // going through qemu.NeedsWindow/WantsWindow, which need a *config.VM. // // A bare socket path is not enough: the user needs the actual command // that opens it, so this prints one for a viewer installed on this host. @@ -380,8 +404,8 @@ func (m model) viewDetail() string { // With no graphical session, the install console also lands on this // socket. That is the case where the user needs the explanation most, // since the VM would otherwise look like it refused to start. - if graphical := qemu.GraphicalSession(); qemu.DisplayKind(v.Mode, v.Installed, graphical) != qemu.DisplayWindow { - if !graphical && qemu.DisplayKind(v.Mode, v.Installed, true) == qemu.DisplayWindow { + if graphical := qemu.GraphicalSession(); qemu.DisplayKind(v.Display, v.Mode, v.Installed, graphical) != qemu.DisplayWindow { + if !graphical && qemu.DisplayKind(v.Display, v.Mode, v.Installed, true) == qemu.DisplayWindow { facts.row("", "", warnStyle.Render("no usable graphical session on this host: the installer console is on vnc")) } line("vnc", v.Paths.VNCSocket) diff --git a/internal/tui/detail_test.go b/internal/tui/detail_test.go index 6fac59e..6eab418 100644 --- a/internal/tui/detail_test.go +++ b/internal/tui/detail_test.go @@ -138,6 +138,51 @@ func TestToggleInstalledFailedSaveLeavesMemoryUnchanged(t *testing.T) { } } +// TestDisplayKeyCyclesAndPersists proves "d" cycles auto -> window -> vnc -> +// auto, saves each step through core.Update, and toasts the new value. +func TestDisplayKeyCyclesAndPersists(t *testing.T) { + t.Setenv("STOAT_HOME", t.TempDir()) + cv := &config.VM{Name: "cycle-test", Mode: "cloud"} + if err := cv.Save(); err != nil { + t.Fatalf("save fixture vm: %v", err) + } + v, err := core.Get(cv.Name) + if err != nil { + t.Fatal(err) + } + m := model{screen: screenDetail, detail: detailModel{vm: v}} + + for _, want := range []string{"window", "vnc", "auto"} { + newM, _ := m.updateDetail(keyMsg("d")) + m = newM.(model) + if m.detail.vm.Display != wantDisplay(want) { + t.Fatalf("Display = %q, want %q", m.detail.vm.Display, wantDisplay(want)) + } + if !strings.Contains(m.toast.text, "display: "+want) { + t.Fatalf("toast = %q, want it to mention display: %s", m.toast.text, want) + } + if m.toast.err { + t.Fatalf("toast marked as error for a successful cycle: %+v", m.toast) + } + reloaded, err := config.Load(cv.Name) + if err != nil { + t.Fatal(err) + } + if reloaded.Display != wantDisplay(want) { + t.Fatalf("not persisted: Display = %q, want %q", reloaded.Display, wantDisplay(want)) + } + } +} + +// wantDisplay maps a cycle step's label to the value core.Patch actually +// writes: "auto" round-trips to "", config.VM.Display's own default. +func wantDisplay(label string) string { + if label == "auto" { + return "" + } + return label +} + // TestTypeConsolePasswordKeyOnlyOfferedWhenAvailable proves the footer // advertises "t" (type console password into guest) only when the VM has // one to send. A stopped VM, or one with no console password set, must not diff --git a/internal/tui/edit.go b/internal/tui/edit.go index df475dd..6c6910f 100644 --- a/internal/tui/edit.go +++ b/internal/tui/edit.go @@ -37,6 +37,8 @@ type editModel struct { recipeNames []string recipeIdx int recipeSel map[string]bool + + display string // one of displayChoices; seeded from vm.Display, "" reads as "auto" } // edit field indices @@ -52,10 +54,11 @@ const ( // focus positions past the text inputs const ( eRecipes = eFieldCount + iota + eDisplay ) func newEdit(v *config.VM) editModel { - e := editModel{vm: v, recipeSel: map[string]bool{}} + e := editModel{vm: v, recipeSel: map[string]bool{}, display: displayPrefLabel(v.Display)} vals := []string{ strconv.Itoa(v.RAM), strconv.Itoa(v.CPUs), @@ -126,7 +129,7 @@ func (e editModel) order() []int { if e.vm.Mode != "live" { o = append(o, eDisk) } - o = append(o, eShare, eSSHPort) + o = append(o, eShare, eSSHPort, eDisplay) if len(e.recipeNames) > 0 { o = append(o, eRecipes) } @@ -257,6 +260,17 @@ func (e editModel) buildPatch() (core.Patch, error) { } } + // "auto" writes back as "", matching config.VM.Display's own default so + // a VM edited back to auto looks the same as one that never set a + // preference. + if e.display != displayPrefLabel(e.vm.Display) { + v := e.display + if v == "auto" { + v = "" + } + p.Display = &v + } + var picked []string for _, n := range e.recipeNames { if e.recipeSel[n] { @@ -373,15 +387,18 @@ func (m model) updateEdit(msg tea.Msg) (tea.Model, tea.Cmd) { m.edit.refocus() return m, nil case "left", "right": + d := 1 + if msg.String() == "left" { + d = -1 + } if m.edit.focus == eRecipes { if n := len(m.edit.recipeNames); n > 0 { - d := 1 - if msg.String() == "left" { - d = -1 - } m.edit.recipeIdx = (m.edit.recipeIdx + d + n) % n } } + if m.edit.focus == eDisplay { + m.edit.display = cycle(displayChoices, m.edit.display, d) + } return m, nil case keySpace: if m.edit.focus == eRecipes && len(m.edit.recipeNames) > 0 { @@ -462,6 +479,21 @@ func (m model) viewEdit() string { row(eShare, "share", e.inputs[eShare].View()) row(eSSHPort, "ssh", e.inputs[eSSHPort].View()) + + displayMarker := " " + if e.focus == eDisplay { + displayMarker = selStyle.Render(glyphCursor) + } + displayRow := radio("auto", e.display == "auto") + " " + + radio("window", e.display == "window") + " " + + radio("vnc", e.display == "vnc") + if e.focus == eDisplay { + displayRow = selStyle.Render(displayRow) + } + if was := displayPrefLabel(e.vm.Display); e.display != was { + displayRow += warnStyle.Render(" " + glyphWas + " was " + was) + } + b.row(displayMarker, "display", displayRow) b.gap() // Recipes are only offered when any exist for this VM's os/backend; diff --git a/internal/tui/edit_test.go b/internal/tui/edit_test.go index af0825f..03fdff1 100644 --- a/internal/tui/edit_test.go +++ b/internal/tui/edit_test.go @@ -275,6 +275,43 @@ func TestEditBuildPatchDoesNotMutateVM(t *testing.T) { } } +// TestEditDisplayPatchAndPersist checks that newEdit seeds display from the +// VM ("" reads as "auto"), that an unchanged display carries no patch, and +// that cycling to "window" writes it through core.Update. +func TestEditDisplayPatchAndPersist(t *testing.T) { + e := editFixture(t) // Display unset in editFixture's vm.toml + if e.display != "auto" { + t.Fatalf("display = %q, want auto", e.display) + } + + p, err := e.buildPatch() + if err != nil { + t.Fatalf("buildPatch: %v", err) + } + if p.Display != nil { + t.Fatalf("Display patch = %v, want nil for an unchanged auto", *p.Display) + } + + e.display = "window" + p, err = e.buildPatch() + if err != nil { + t.Fatalf("buildPatch: %v", err) + } + if p.Display == nil || *p.Display != "window" { + t.Fatalf("Display patch = %v, want \"window\"", p.Display) + } + if _, errText := saveEdit(e.name(), p, false); errText != "" { + t.Fatalf("saveEdit: %s", errText) + } + reloaded, err := config.Load(e.name()) + if err != nil { + t.Fatal(err) + } + if reloaded.Display != "window" { + t.Fatalf("not persisted: Display = %q", reloaded.Display) + } +} + // TestEditTabOrderSkipsDiskInLiveMode mirrors the create form's rule: focus // must never land on a row viewEdit does not draw. Otherwise keystrokes // silently edit an invisible field. Mode is immutable, so this test reads diff --git a/internal/tui/form.go b/internal/tui/form.go index 1008e4b..fba5cfd 100644 --- a/internal/tui/form.go +++ b/internal/tui/form.go @@ -233,6 +233,7 @@ type formModel struct { byoBackend string // override for the selected BYO image's backend; "" means "use iso.Infer's guess" byoOS string // override for the selected BYO image's OS; "" means "use iso.Infer's guess" mode string // "live" | "disk"; meaningful only while the selected image's backend is apkovl + display string // one of displayChoices; "auto" by default err string fetching bool fetchingOS string @@ -279,6 +280,7 @@ const ( fOS fRecipes fPassword + fDisplay ) // focusOrder is the tab-traversal order of focus positions, which must match @@ -308,7 +310,7 @@ func (f formModel) order() focusOrder { if m := f.effectiveMode(); m == "disk" || m == "cloud" { o = append(o, fDisk) } - o = append(o, fShare, fRecipes) + o = append(o, fShare, fDisplay, fRecipes) // The console password row is only meaningful for a cloud image; the // other backends never set one. if f.resolvedBackend() == "cloudinit" { @@ -435,7 +437,7 @@ func (f *formModel) selectImage(idx int) { } func newForm() formModel { - f := formModel{mode: "live", recipeSel: map[string]bool{}} + f := formModel{mode: "live", display: "auto", recipeSel: map[string]bool{}} labels := []string{"work", "4096", "4", "8G", "~/vms"} for i := 0; i < fieldCount; i++ { ti := theme.TextInput() @@ -641,6 +643,13 @@ func (m model) updateForm(msg tea.Msg) (tea.Model, tea.Cmd) { m.form.mode = "live" } return m, nil + case fDisplay: + d := 1 + if msg.String() == "left" { + d = -1 + } + m.form.display = cycle(displayChoices, m.form.display, d) + return m, nil case fPassword: m.form.randomPassword = !m.form.randomPassword return m, nil @@ -791,6 +800,9 @@ func (f formModel) spec() (core.Spec, error) { Share: strings.TrimSpace(f.inputs[fShare].Value()), Recipes: selected, } + if f.display != "auto" { + s.Display = f.display + } if f.randomPassword { s.ConsolePassword = "random" } @@ -883,6 +895,11 @@ func (m model) viewForm() string { row(fShare, "share", f.inputs[fShare].View()) + displayRow := radio("auto", f.display == "auto") + " " + + radio("window", f.display == "window") + " " + + radio("vnc", f.display == "vnc") + row(fDisplay, "display", displayRow) + recipesMarker := " " if f.focus == fRecipes { recipesMarker = selStyle.Render(glyphCursor) diff --git a/internal/tui/form_test.go b/internal/tui/form_test.go index 849cb56..71553dc 100644 --- a/internal/tui/form_test.go +++ b/internal/tui/form_test.go @@ -21,8 +21,9 @@ import ( ) // TestFormTabOrder pins tab focus to the order viewForm renders fields in -// (name, iso, mode, ram, cpus, [disk], share), not the field constants' -// declaration order (name, ram, cpus, disk, share, iso, mode). In live mode, +// (name, iso, mode, ram, cpus, [disk], share, display), not the field +// constants' declaration order (name, ram, cpus, disk, share, iso, mode). In +// live mode, // tab must not land on fDisk: viewForm renders no disk row, and no "❯" // marker, in that mode, so a keystroke there would silently edit an // invisible field. The test checks the exact visited sequence, forward and @@ -33,8 +34,8 @@ func TestFormTabOrder(t *testing.T) { mode string order []int // visual/traversal order, starting from the initial focus (fName) }{ - {"live", "live", []int{fName, fISO, fMode, fRAM, fCPUs, fShare, fRecipes}}, - {"disk", "disk", []int{fName, fISO, fMode, fRAM, fCPUs, fDisk, fShare, fRecipes}}, + {"live", "live", []int{fName, fISO, fMode, fRAM, fCPUs, fShare, fDisplay, fRecipes}}, + {"disk", "disk", []int{fName, fISO, fMode, fRAM, fCPUs, fDisk, fShare, fDisplay, fRecipes}}, } for _, c := range cases { @@ -181,6 +182,51 @@ func TestBuildAssignsSelectedRecipes(t *testing.T) { }) } +// TestBuildDisplayDefaultsToAutoAndCyclesRight checks that the create form +// leaves Display empty (auto) unless the user changes it, and that "right" +// on fDisplay cycles auto -> window -> vnc -> auto, carrying the choice into +// the built VM. +func TestBuildDisplayDefaultsToAutoAndCyclesRight(t *testing.T) { + t.Setenv("STOAT_HOME", t.TempDir()) + f := newForm() + f.inputs[fName].SetValue("displaytest") + f.images = []imageOption{stubImage(t, "alpine-standard-3.20.0-x86_64.iso")} + f.imgIdx = 0 + + vm, err := f.build() + if err != nil { + t.Fatalf("build: %v", err) + } + if vm.Display != "" { + t.Fatalf("Display = %q, want empty (auto) by default", vm.Display) + } + + m := model{form: f} + for _, want := range []string{"window", "vnc", "auto"} { + m.form.focus = fDisplay + mm, _ := m.updateForm(keyMsg("right")) + m = mm.(model) + if m.form.display != want { + t.Fatalf("after cycling, display = %q, want %q", m.form.display, want) + } + } + + m.form.inputs[fName].SetValue("displaytest2") + m.form.focus = fDisplay + mm, _ := m.updateForm(keyMsg("left")) + m = mm.(model) + if m.form.display != "vnc" { + t.Fatalf("left from auto: display = %q, want vnc", m.form.display) + } + vm2, err := m.form.build() + if err != nil { + t.Fatalf("build: %v", err) + } + if vm2.Display != "vnc" { + t.Fatalf("Display = %q, want vnc", vm2.Display) + } +} + // TestBuildRejectsRelativeDiskSize checks that build() refuses a relative // disk size like "+8G". qemu-img's resize reads a leading "+" as "grow by", // not "resize to", which would silently double a fresh overlay if the value diff --git a/internal/tui/keymap.go b/internal/tui/keymap.go index 7e13683..a3fc65c 100644 --- a/internal/tui/keymap.go +++ b/internal/tui/keymap.go @@ -136,6 +136,7 @@ func (h detailHelp) ShortHelp() []key.Binding { plainKey([]string{"e"}, "e", "edit"), plainKey([]string{"E"}, "E", "raw toml"), plainKey([]string{"i"}, "i", "installed"), + plainKey([]string{"d"}, "d", "display"), h.ssh(), plainKey([]string{"p"}, "p", "provision"), plainKey([]string{"S"}, "S", "snapshots"), @@ -155,7 +156,8 @@ func (h detailHelp) ShortHelp() []key.Binding { func (h detailHelp) FullHelp() [][]key.Binding { rows := [][]key.Binding{ {plainKey([]string{"e"}, "e", "edit form"), plainKey([]string{"E"}, "E", "raw vm.toml in $EDITOR")}, - {plainKey([]string{"i"}, "i", "installed"), h.ssh()}, + {plainKey([]string{"i"}, "i", "installed"), plainKey([]string{"d"}, "d", "cycle display: auto/window/vnc")}, + {h.ssh()}, {plainKey([]string{"p"}, "p", "provision"), plainKey([]string{"L"}, "L", "console log")}, {plainKey([]string{"S"}, "S", "snapshots")}, } diff --git a/internal/tui/labels.go b/internal/tui/labels.go index 41b1d42..a1e0e5b 100644 --- a/internal/tui/labels.go +++ b/internal/tui/labels.go @@ -44,6 +44,46 @@ func modeLabel(mode string) string { return mode } +// cycle returns the choice after (d=1) or before (d=-1) cur in choices, +// wrapping at either end. An unrecognised cur is treated as choices[0] +// before stepping. +func cycle(choices []string, cur string, d int) string { + idx := 0 + for i, c := range choices { + if c == cur { + idx = i + } + } + return choices[(idx+d+len(choices))%len(choices)] +} + +// displayChoices is the fixed cycle the create form, the edit form, and the +// detail screen's "d" key all offer for config.VM.Display. +var displayChoices = []string{"auto", "window", "vnc"} + +// displayPrefLabel is what a display preference row shows: "" and "auto" both +// read as "auto", since config.VM.Display treats them the same. +func displayPrefLabel(pref string) string { + if pref == "" { + return "auto" + } + return pref +} + +// nextDisplayPref cycles a display preference forward through +// displayChoices: auto -> window -> vnc -> auto. An unrecognised value (a +// hand-edited vm.toml) is treated as auto, the same fallback validateDisplay +// applies to "". +func nextDisplayPref(pref string) string { + cur := displayPrefLabel(pref) + for i, c := range displayChoices { + if c == cur { + return displayChoices[(i+1)%len(displayChoices)] + } + } + return displayChoices[0] +} + // modeHint is the sentence shown under a mode picker for the selected mode. func modeHint(mode string) string { switch mode {