From dc103f8a05d6bbcf465a85b0dacdc64b7cfbc9cd Mon Sep 17 00:00:00 2001 From: foxnne Date: Mon, 19 Oct 2026 09:16:04 -0500 Subject: [PATCH] app: review fixes for setting the person's panes aside - Nothing is saved while a demo holds the panes, whatever else happens (`Workbench.holding`): a copy that could not be made was swallowed, and the demo's panes saved over the person's. Where there is no copy, the end reads theirs back from disk, which nothing overwrote. - A demo started before the first rebuild restored last session's panes held the default tree, and the saved panes loaded mid-demo. That restore now waits for the demo to end. - Each document reopens into the pane it was in, recorded when the demo began, rather than the current one, so the arrangement put back does not depend on the layout's saved assignments. The demo test now starts from the person's own two panes and checks both, each with its document, after a demo that split the editor. Co-Authored-By: Claude Opus 5.5 --- plugins/workbench/src/Workbench.zig | 6 ++- plugins/workbench/src/workbench_layout.zig | 45 +++++++++++++++++----- src/editor/Demo.zig | 9 +++-- tests/demos.zig | 27 ++++++++++++- 4 files changed, 71 insertions(+), 16 deletions(-) diff --git a/plugins/workbench/src/Workbench.zig b/plugins/workbench/src/Workbench.zig index 309a55a80..f1e39cd60 100644 --- a/plugins/workbench/src/Workbench.zig +++ b/plugins/workbench/src/Workbench.zig @@ -38,8 +38,10 @@ workspaces: std.AutoArrayHashMapUnmanaged(u64, Workspace) = .empty, /// plugin's own directory whenever it changes (`workbench_layout.savePanes`). panes: Panes, panes_dirty: bool = false, -/// The person's arrangement while a demo has the window (`holdPanes`), a copy of its own, and the -/// pane they were in: nothing is saved while it is held. +/// A demo has the window (`holdPanes`): nothing is saved, and last session's panes wait. +holding: bool = false, +/// The person's arrangement while a demo has the window, a copy of its own (null where it could +/// not be made, or before there was one), and the pane they were in. held_panes: ?Panes = null, held_open: u64 = 0, /// Where the panes are arranged when the app arranges them instead of `panes`: each pane a view diff --git a/plugins/workbench/src/workbench_layout.zig b/plugins/workbench/src/workbench_layout.zig index 64c398642..5ce090f74 100644 --- a/plugins/workbench/src/workbench_layout.zig +++ b/plugins/workbench/src/workbench_layout.zig @@ -19,7 +19,9 @@ pub fn rebuildWorkspaces(wb: *Workbench) !void { const host = runtime.host(); const arena = host.arena(); - if (!wb.restored) { + // Not while a demo has the window: last session's panes and tabs come back once it has gone + // (`restorePanes`), not into the middle of it. + if (!wb.restored and !wb.holding) { wb.restored = true; // Where the app arranges the panes, it brought them back itself — and carries an // arrangement saved before it did over into its own (fizzy's `Workspaces.carryOver`). @@ -157,7 +159,7 @@ pub fn drawWorkspaces(wb: *Workbench, index: usize) !dvui.App.Result { fn savePanes(wb: *Workbench) void { wb.panes_dirty = false; // A demo has the window: what it arranges is not the person's (`holdPanes`). - if (wb.held_panes != null) return; + if (wb.holding) return; if (comptime builtin.target.cpu.arch == .wasm32) return; const path = panesPath(wb) orelse return; defer wb.allocator.free(path); @@ -239,14 +241,27 @@ fn adoptPanes(wb: *Workbench, loaded: Panes) void { /// Keep the person's arrangement while a demo has the window, and save nothing until it is put /// back (`restorePanes`): the demo's splits are not theirs. A second hold keeps the first. +/// +/// Nothing is saved while holding, whatever else happens, so the arrangement on disk stays the +/// person's: a copy that could not be made is read back from there instead. Before the first +/// rebuild has restored last session's panes there is nothing of theirs here to copy; that +/// restore waits for the demo to end. pub fn holdPanes(wb: *Workbench) void { - if (wb.held_panes != null) return; + if (wb.holding) return; + wb.holding = true; + wb.held_open = wb.open_workspace_grouping; + if (!wb.restored) return; // A copy through a snapshot: a snapshot borrows the pane names from the live tree, which the // demo's first cut replaces (`onePane`). - const snap = wb.panes.snapshot(wb.allocator) catch return; + const snap = wb.panes.snapshot(wb.allocator) catch |err| { + dvui.log.err("workbench: could not set the panes aside ({t}); they come back from disk", .{err}); + return; + }; defer snap.deinit(wb.allocator); - wb.held_panes = Panes.fromSnapshot(wb.allocator, snap) catch return; - wb.held_open = wb.open_workspace_grouping; + wb.held_panes = Panes.fromSnapshot(wb.allocator, snap) catch |err| blk: { + dvui.log.err("workbench: could not set the panes aside ({t}); they come back from disk", .{err}); + break :blk null; + }; } /// One pane, as a demo's keyframe declares the window: the open one, the others closing with @@ -264,11 +279,21 @@ pub fn onePane(wb: *Workbench) void { adoptPanes(wb, one); } -/// The person's arrangement again (`holdPanes`), and saving with it. +/// The person's arrangement again (`holdPanes`), and saving with it: the copy, or what is on disk +/// where there is none. Before last session's panes were ever restored, one pane, for the next +/// rebuild to restore them into. pub fn restorePanes(wb: *Workbench) void { - const held = wb.held_panes orelse return; - wb.held_panes = null; - adoptPanes(wb, held); + if (!wb.holding) return; + wb.holding = false; + if (wb.held_panes) |held| { + wb.held_panes = null; + adoptPanes(wb, held); + } else if (wb.restored) { + loadPanes(wb); + } else { + wb.open_workspace_grouping = wb.held_open; + onePane(wb); + } if (wb.workspaces.contains(wb.held_open)) wb.open_workspace_grouping = wb.held_open; } diff --git a/src/editor/Demo.zig b/src/editor/Demo.zig index 9c326d96d..eed40cd3c 100644 --- a/src/editor/Demo.zig +++ b/src/editor/Demo.zig @@ -104,6 +104,9 @@ const Saved = struct { const Doc = struct { path: []u8, + /// The pane it was in (`documentGrouping`): it opens there again, in the arrangement put + /// back (`Workbench.restorePanes`), rather than every document in the one pane. + grouping: u64, /// What it was opened from, when its path cannot open it again (`reopenable`): a file the /// web page fetched or the person uploaded, a zip whose folder is the project. Null: it /// reopens from its path. @@ -481,7 +484,7 @@ fn begin(ctx: *anyopaque, _: *const automation.Tape) void { // Never saved (`untitled-1`): no file, and clean, since `mayStart` refused unsaved work, so // nothing to give back. Opened from its bytes it would come back as a file of that name. if (!reopenable(editor, path) and !doc.owner.documentHasRecognizedSaveExtension(doc)) continue; - var saved: Saved.Doc = .{ .path = gpa.dupe(u8, path) catch continue }; + var saved: Saved.Doc = .{ .path = gpa.dupe(u8, path) catch continue, .grouping = doc.owner.documentGrouping(doc) }; if (!reopenable(editor, path)) { saved.bytes = doc.owner.documentBytes(doc, path, gpa) catch null; if (saved.bytes == null) dvui.log.warn("demo: {s} cannot be opened again after the demo: its owner gives no bytes for it", .{path}); @@ -528,7 +531,7 @@ fn end(ctx: *anyopaque) void { for (saved.docs) |d| { const bytes = d.bytes orelse continue; const path = gpa.dupe(u8, d.path) catch continue; - _ = editor.openFileFromBytes(path, bytes, editor.workbench.currentGroupingID()) catch |err| + _ = editor.openFileFromBytes(path, bytes, d.grouping) catch |err| dvui.log.warn("demo: could not reopen {s}: {t}", .{ d.path, err }); } if (saved.folder) |f| { @@ -538,7 +541,7 @@ fn end(ctx: *anyopaque) void { } for (saved.docs) |d| { if (d.bytes != null) continue; - _ = editor.openFilePath(d.path, editor.workbench.currentGroupingID()) catch |err| + _ = editor.openFilePath(d.path, d.grouping) catch |err| dvui.log.warn("demo: could not reopen {s}: {t}", .{ d.path, err }); } // The active one last, whichever way it came back. diff --git a/tests/demos.zig b/tests/demos.zig index abb7d575a..9469db69d 100644 --- a/tests/demos.zig +++ b/tests/demos.zig @@ -190,7 +190,20 @@ test "demo: every bundled demo plays to the end through fizzy's stage, a seek la const wb = &app.editor.workbench; p.unload(); try dvui.testing.settle(headlessFrame); + // The person's own split first: a file of theirs in a pane of its own beside the zip's. + const readme = try std.fmt.allocPrint(gpa, "{s}/notes.md", .{app.root_buf[0 .. std.mem.indexOfScalar(u8, &app.root_buf, 0) orelse 0]}); + defer gpa.free(readme); + try std.Io.Dir.cwd().writeFile(dvui.io, .{ .sub_path = readme, .data = "# Notes\n" }); + const beside = wb.newGroupingID(); + _ = try app.editor.openFilePath(readme, beside); + // Loaded off the UI thread: it lands a frame or more on. + frames = 0; + while (app.editor.app.host.docFromPath(readme) == null and frames < 200) : (frames += 1) _ = try dvui.testing.step(headlessFrame); + try dvui.testing.settle(headlessFrame); + const zip_pane = (app.editor.app.host.docFromPath("site.zip") orelse return error.TestUnexpectedResult).owner.documentGrouping(app.editor.app.host.docFromPath("site.zip").?); + try std.testing.expect(zip_pane != beside); const panes_before = wb.workspaces.count(); + try std.testing.expectEqual(@as(usize, 2), panes_before); const split = try splitDemo(); defer gpa.free(split); try app.editor.demo.playTape(split); @@ -204,8 +217,20 @@ test "demo: every bundled demo plays to the end through fizzy's stage, a seek la try std.testing.expectEqual(@as(usize, 1), wb.workspaces.count()); p.unload(); try dvui.testing.settle(headlessFrame); + frames = 0; + while (app.editor.app.host.docFromPath(readme) == null and frames < 200) : (frames += 1) _ = try dvui.testing.step(headlessFrame); + try dvui.testing.settle(headlessFrame); try std.testing.expectEqual(panes_before, wb.workspaces.count()); - try std.testing.expect(wb.held_panes == null); + try std.testing.expect(wb.held_panes == null and !wb.holding); + // Each document back in its own pane, and both panes still there after a rebuild more. + const back = app.editor.app.host.docFromPath(readme) orelse return error.TestUnexpectedResult; + try std.testing.expectEqual(beside, back.owner.documentGrouping(back)); + const zip_back = app.editor.app.host.docFromPath("site.zip") orelse return error.TestUnexpectedResult; + try std.testing.expectEqual(zip_pane, zip_back.owner.documentGrouping(zip_back)); + try dvui.testing.settle(headlessFrame); + try std.testing.expectEqual(panes_before, wb.workspaces.count()); + try app.editor.rawCloseFileID(back.id); + try dvui.testing.settle(headlessFrame); } // A tape loaded some other way is not the one the service started.