fix(window): keep pmacs.window.buffer() infallible with no argument
PR #155 review round 2, self-review of the round-2 commit.
The round-2 change labelled "minor" — resolving both arms of
pmacs.window.buffer() through the acting frontend for uniformity — made
the NO-ARGUMENT arm fallible. `acting_frontend` follows the interactive
origin, which can name a frontend that has no registered view: a bare
`dispatch_key` from an unattached peer does exactly that. `selected_window`
then raises "acting frontend has no layout" instead of answering.
Nothing surfaced that error, because the runtime callers do not pcall it.
killring, syntax, autosave, pair, indent and comment all read
pmacs.window.buffer() on ordinary edits, so the raise silently dropped
the operation: kill_ring_acceptance went 30/30 to 25/5, with
frontend_detached_drops_per_frontend_state reporting only "B has kill
state". main is 30/30, and reverting this one file restored it.
The no-arg arm is back on ambient active_buffer_id() and now documents
why that is deliberate rather than an oversight: dispatch sets
active_frontend to the acting frontend before running a command, so the
two agree on every real path, while only the ambient resolver has the
fallback that makes it total. The explicit-window arm keeps its Q#BP11
layout validation, which is what the arc actually needed.
acc19c pins it through the real path — a buffer.after-edit subscriber
reading pmacs.window.buffer() during a viewless peer's dispatch_key —
rather than by calling the binding directly. Bite-verified:
scripts/bite bbe4152 src/lua_bindings/mod.rs --test
bottom_panel_stage1_acceptance -- acc19c goes red with the exact
"acting frontend has no layout" traceback.
The ledger also records two gating facts found on the way: the workspace
sweep must run with an isolated XDG_CONFIG_HOME, because the real user
init.lua installs a local package and the losing race leaks a status
message into painted-frame comparisons; and a latent pre-existing main
bug in the buffer CRDT undo path, which is not this branch's and whose
proptest seed is deliberately not committed here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012j4omtTMn9v1UfmHQb9ap6
This commit is contained in:
parent
bbe4152048
commit
9110f9f32c
|
|
@ -137,13 +137,52 @@ If it does not, stop and repair the remote/fetch configuration.
|
|||
the viewport width, so any text equality over it is vacuously true.
|
||||
Emit `\r\n`, and guard text comparisons with a non-empty assertion
|
||||
the way the daemon pin guards on `!panel_hidden`.
|
||||
- **Round-2 self-review caught a regression the round-2 commit
|
||||
introduced**, in the change it labelled "minor": routing
|
||||
`pmacs.window.buffer()`'s **no-argument** arm through the fid-scoped
|
||||
`selected_window` validator made it **fallible**, and
|
||||
`acting_frontend` can name a frontend with **no registered view** (a
|
||||
bare `dispatch_key` from an unattached peer does exactly that). The
|
||||
runtime calls that function on ordinary edits from `killring`,
|
||||
`syntax`, `autosave`, `pair`, `indent` and `comment` **without
|
||||
`pcall`**, so the raise never surfaced as an error — it silently
|
||||
dropped the operation. `kill_ring_acceptance` went 30/30 → 25/5
|
||||
(`frontend_detached_drops_per_frontend_state`: "B has kill state").
|
||||
The no-arg arm is back on ambient `active_buffer_id()` and documented
|
||||
as deliberately infallible; the explicit-window arm keeps its Q#BP11
|
||||
validation. New **acc19c** pins it through the real path (a
|
||||
`buffer.after-edit` subscriber during a viewless peer's `dispatch_key`)
|
||||
and bites against the regressing commit.
|
||||
Generalizes: **a "uniformity" cleanup that changes a function's
|
||||
fallibility is not minor** — check every caller's error discipline
|
||||
first, and remember that an ambient resolver's fallback IS its
|
||||
contract.
|
||||
- Verification on this branch: `cargo fmt --check` clean; strict
|
||||
workspace Clippy clean; 1,817 default + 1,994 CRDT library tests;
|
||||
`bottom_panel_stage1_acceptance` 45/45; vterm Stage 1 9; M4 121; required GPU 152;
|
||||
`gpu_initial_target_acceptance` 14 CRDT; compile 67; vterm Stage 2 4 /
|
||||
Stage 3 5; folding Stage 2 48; statusline 7; listview 6; desktop 11;
|
||||
**workspace sweep 3,128 passed, zero failures**; `git diff --check`
|
||||
clean.
|
||||
`bottom_panel_stage1_acceptance` 46/46; kill ring 30 default + 30 CRDT;
|
||||
vterm Stage 1 9 default + 10 CRDT; M4 121; required GPU 152;
|
||||
compile 67; vterm Stage 2 4 / Stage 3 5 (7 CRDT); folding Stage 2 48;
|
||||
statusline 7; listview 6;
|
||||
**isolated-config workspace sweep 3,130 passed across 89 suites, zero
|
||||
failures**; `git diff --check` clean.
|
||||
- **Run the sweep with an isolated `XDG_CONFIG_HOME`.** The real
|
||||
`~/.config/pmacs/init.lua` on this desktop calls
|
||||
`pmacs.packages.install_local(...)`, so every editor the sweep builds
|
||||
races on one shared install root; a losing race sets a status message
|
||||
that leaks into the mode line and breaks
|
||||
`folding_stage2_acceptance::unfolded_frame_is_identical_to_the_pre_folding_baseline`,
|
||||
which compares whole painted frames. Standalone it is 48/48. This
|
||||
generalizes the known `compile_mode_acceptance` real-config trap:
|
||||
any suite that paints the status area inherits it.
|
||||
- **A latent pre-existing `main` bug surfaced while gating and is NOT
|
||||
this branch's**: `buffer::tests::proptests::rope_matches_crdt_projection_after_arbitrary_edits`
|
||||
fails on `main` @ `352bf0b` with `ops = [Insert(0,"a"),
|
||||
Insert(0,"aaa"), Replace(0,1,"a"), Undo]` — undo of a textually-null
|
||||
`Replace` returns a no-op edit result still carrying `crdt_op =
|
||||
Some`, violating the suite's own shape invariant. `src/buffer.rs` is
|
||||
byte-identical here, and the seed was deliberately **not** committed
|
||||
(it would make an unrelated failure deterministically red on this
|
||||
PR). Needs its own lane.
|
||||
- Durable test lesson from this round: `TerminalViewStatus.scroll_offset`
|
||||
is documented as the retained rows between **this viewport** and the
|
||||
live tail, so it necessarily tracks the viewport height. Asserting it
|
||||
|
|
|
|||
|
|
@ -12357,9 +12357,9 @@ fn install_window_module(lua: &Lua, core: &SharedCore) -> mlua::Result<Table> {
|
|||
|
||||
{
|
||||
let cc = core.clone();
|
||||
// With no argument: the selected window's buffer (unchanged).
|
||||
// With an explicit window id: that window's buffer, validated
|
||||
// against the acting frontend's layout like every other
|
||||
// With no argument: the ambient active buffer, exactly as before
|
||||
// this arc. With an explicit window id: that window's buffer,
|
||||
// validated against the acting frontend's layout like every other
|
||||
// `WindowId`-taking operation (bottom-panel arc, Q#BP11) — an
|
||||
// adopter has to be able to ask "is my buffer the one in the
|
||||
// panel" without first selecting the panel.
|
||||
|
|
@ -12367,14 +12367,24 @@ fn install_window_module(lua: &Lua, core: &SharedCore) -> mlua::Result<Table> {
|
|||
"buffer",
|
||||
lua.create_function(
|
||||
move |lua, target: Option<u64>| -> mlua::Result<BufferIdLua> {
|
||||
// Both arms resolve through the ACTING frontend, using
|
||||
// the same validator the rest of the window surface
|
||||
// does — no ambient `active_buffer_id()` asymmetry.
|
||||
let fid = window_panel::acting_frontend(lua, &cc);
|
||||
let id = match target {
|
||||
Some(raw) => window_panel::lookup_window(&cc, fid, raw)?,
|
||||
None => window_panel::selected_window(&cc, fid)?,
|
||||
// The no-arg arm deliberately stays on ambient
|
||||
// `active_buffer_id()`, and stays INFALLIBLE. This is not
|
||||
// the asymmetry it looks like: dispatch sets
|
||||
// `active_frontend` to the acting frontend before running a
|
||||
// command, so the two agree on every real path — while
|
||||
// `acting_frontend` can additionally name a frontend that
|
||||
// has no registered view, where a `views`-keyed lookup
|
||||
// raises instead of answering. `killring`, `syntax`,
|
||||
// `autosave`, `pair`, `indent` and `comment` all call this
|
||||
// on ordinary edits without `pcall`, so a raise here does
|
||||
// not surface as an error — it silently drops the
|
||||
// operation (it lost a whole kill in `kill_ring_acceptance`
|
||||
// when this arm was routed through `selected_window`).
|
||||
let Some(raw) = target else {
|
||||
return Ok(BufferIdLua(cc.borrow().active_buffer_id()));
|
||||
};
|
||||
let fid = window_panel::acting_frontend(lua, &cc);
|
||||
let id = window_panel::lookup_window(&cc, fid, raw)?;
|
||||
cc.borrow()
|
||||
.windows
|
||||
.get(&id)
|
||||
|
|
|
|||
|
|
@ -1369,6 +1369,58 @@ fn acc19b_recompile_reuses_the_panel_instead_of_duplicating_into_the_document()
|
|||
);
|
||||
}
|
||||
|
||||
/// `pmacs.window.buffer()` with NO argument must stay **infallible**.
|
||||
///
|
||||
/// The optional window argument this arc added is validated against the
|
||||
/// acting frontend's layout, and it is tempting to make the no-arg arm
|
||||
/// symmetric by resolving it the same way. That silently breaks the
|
||||
/// runtime: `acting_frontend` follows the interactive origin, which can
|
||||
/// name a frontend with **no registered view** (as a bare
|
||||
/// `dispatch_key` from a peer does), where a `views`-keyed lookup raises
|
||||
/// instead of answering — and `killring`, `syntax`, `autosave`, `pair`,
|
||||
/// `indent` and `comment` all call this on ordinary edits without
|
||||
/// `pcall`, so the raise does not surface as an error, it just drops the
|
||||
/// operation. Routing it through `selected_window` lost an entire kill in
|
||||
/// `kill_ring_acceptance`.
|
||||
#[test]
|
||||
fn acc19c_window_buffer_stays_infallible_for_an_acting_frontend_without_a_view() {
|
||||
let mut s = editor();
|
||||
let ambient = s.core.borrow().active_buffer_id();
|
||||
exec(
|
||||
&s,
|
||||
// A `buffer.after-edit` subscriber is the real shape: this is
|
||||
// where syntax.lua, pair.lua and comment.lua each call
|
||||
// `pmacs.window.buffer()` on every ordinary edit.
|
||||
"SEEN = nil; ERR = nil \
|
||||
pmacs.hook.add(\"buffer.after-edit\", function() \
|
||||
local ok, got = pcall(pmacs.window.buffer) \
|
||||
if ok then SEEN = got else ERR = tostring(got) end \
|
||||
end)",
|
||||
);
|
||||
|
||||
// A peer that never registered a view — the shape `dispatch_key`
|
||||
// produces for an unattached frontend, and what the kill-ring suite
|
||||
// drives with `ctrl_as`.
|
||||
let viewless = FrontendId(9);
|
||||
assert!(
|
||||
!s.core.borrow().views.contains_key(&viewless),
|
||||
"the premise: this frontend really has no view"
|
||||
);
|
||||
s.dispatch_key(viewless, key(KeyCode::Char('z'), KeyModifiers::NONE));
|
||||
|
||||
let err: Option<String> = eval(&s, "return ERR");
|
||||
assert_eq!(
|
||||
err, None,
|
||||
"pmacs.window.buffer() must not raise for a viewless acting frontend"
|
||||
);
|
||||
let seen: Option<pmacs::lua_bindings::BufferIdLua> = eval(&s, "return SEEN");
|
||||
assert_eq!(
|
||||
seen.expect("the command observed a buffer").0,
|
||||
ambient,
|
||||
"…it answers with the ambient active buffer"
|
||||
);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// 20 / 23 — quit: delete, restore chains, revalidation, and the cap
|
||||
// ---------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Reference in New Issue