From 9134299233af5834bf4f716824709914d5626024 Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Sun, 6 Sep 2026 10:35:22 +0200 Subject: [PATCH] Sketch value fields: content-based key arbiter + the gate that can judge it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported defect: sketch dimension labels are "not editable" — you draw a rectangle, its Width field opens, you type, and the as-drawn number is committed instead. It affects every sketch tool, not just the rounded rectangle. WHAT THIS ADDS 1. The arbiter (DesignPanel CHAR_HOOK -> DesignCanvas::inline_type_char -> SketchInlineEditor::type_char). Routes a key by what it IS, not by who the window manager focused: digits, sign, decimal separator and Backspace/Delete go to the open value field, Enter/Tab commit, letters stay tool shortcuts. This is FreeCAD Sketcher's rule (DrawSketchKeyboardManager:: detectKeyboardEventHandlingMode), and the reason its sketcher behaves the same on every desktop: it never asks who has focus. 2. The [UX] trace (SNAPORCA_UXTRACE) in SketchInlineEditor: open/commit/refused/ cancel, with the prefill and what the control actually held at Enter. It did not exist — the ladder below was written against a surface no build emitted, so it could only ever report "nothing opened". typed == prefill on a commit is the defect's signature and nothing else makes it visible. 3. A draw-then-edit trace in DesignSketchTool: four early returns can swallow the value-field chain and from outside they are indistinguishable. 4. scripts/CAD/check-gui-click-edit.py — types WITHOUT clicking the field, as a person does, across Line/Rectangle/Circle/Slot/Polygon/Ellipse/Arc plus label click-to-edit, and asserts committed == typed != prefill. 5. scripts/CAD/focus-loop.sh — sync/build/assert on behemoth. NOT the orcacad-gui rig: its image pins deps 216 non-CAD files behind cad-mainline, so today's CAD sources cannot build there without a deps rebuild. WHAT IS PROVEN, AND WHAT IS NOT Green under openbox: 28 checks, every tool, committed == typed != prefill. But openbox CANNOT adjudicate this bug and the ladder says so in place. There the field always wins the keyboard, so the same ladder also passes against a binary with the arbiter compiled out — measured twice. Two ways of removing the keyboard were tried and both are recorded as dead ends: XSetInputFocus loses to the field's own re-focus CallAfter, and XSendEvent (xdotool --window) is dropped by GTK, which made every run red regardless of the code. Under metacity — same focus-stealing-prevention lineage as the user's mutter — the mechanism appears in the WM's own log: Buggy client sent a _NET_ACTIVE_WINDOW message with a timestamp of 0 That is the activation being refused, which is exactly the reported symptom. present_toplevel() already asks for a server timestamp, so a path is still falling through to frame->Raise(), which sends time 0. That is the next thing to fix, and it is tracked; the arbiter alone does not close it. metacity also aborts on this window (frames.c:1239), so the gate needs a WM that survives before it can return a verdict. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011FbJKJAJxxkhDTs9XdZzKA --- .claude/loopspec/sketch-focus-arbiter.spec.md | 86 +++ scripts/CAD/check-gui-click-edit.py | 636 ++++++++++++++++++ scripts/CAD/focus-loop.sh | 85 +++ src/slic3r/GUI/CAD/DesignCanvas.cpp | 5 + src/slic3r/GUI/CAD/DesignCanvas.hpp | 2 + src/slic3r/GUI/CAD/DesignPanel.cpp | 19 + src/slic3r/GUI/CAD/DesignSketchTool.cpp | 19 +- src/slic3r/GUI/CAD/SketchInlineEditor.cpp | 80 +++ src/slic3r/GUI/CAD/SketchInlineEditor.hpp | 16 + 9 files changed, 946 insertions(+), 2 deletions(-) create mode 100644 .claude/loopspec/sketch-focus-arbiter.spec.md create mode 100755 scripts/CAD/check-gui-click-edit.py create mode 100755 scripts/CAD/focus-loop.sh diff --git a/.claude/loopspec/sketch-focus-arbiter.spec.md b/.claude/loopspec/sketch-focus-arbiter.spec.md new file mode 100644 index 0000000000..f0c1287307 --- /dev/null +++ b/.claude/loopspec/sketch-focus-arbiter.spec.md @@ -0,0 +1,86 @@ +# DELEGATION SPECIFICATION: HARNESS-DRIVEN VALIDATION LOOP +slug: sketch-focus-arbiter · repo: /home/tommaso/projects/apps/orca_cad · branch: cad-mainline + +## 1. TARGET GOAL + +**Functional Objective.** Keyboard input in the Design tab is routed by WHAT THE KEY IS, not by +which widget the window manager decided to focus. Adopted from FreeCAD's +`DrawSketchKeyboardManager::detectKeyboardEventHandlingMode` +(src/Mod/Sketcher/Gui/DrawSketchKeyboardManager.cpp), which never queries focus at all: + + - digit, `-`, `.`, `,` -> the open value field + - Backspace / Delete -> the open value field (when one is open) + - Enter / Return / Tab -> commit the field, control returns to the view + - a letter -> the sketch-tool shortcut map, as today + - Esc -> the existing CadLevel LIFO (DesignInteraction.hpp), unchanged + - anything else -> sticky: whoever had it keeps it + +Observable postcondition: for EVERY sketch tool that opens a value field, a value typed +immediately after the field appears — with NO click into the field — is the value committed. +Today the prefill is committed instead whenever the WM withholds focus. + +**Target Files / Scope (writable).** + src/slic3r/GUI/CAD/DesignPanel.cpp (the arbiter lives in the existing wxEVT_CHAR_HOOK) + src/slic3r/GUI/CAD/DesignCanvas.cpp/.hpp (forwarding entry points only) + src/slic3r/GUI/CAD/SketchInlineEditor.cpp/.hpp (accept a programmatically delivered character) + scripts/CAD/check-gui-click-edit.py (F2P oracle — authoring exception, see §4) +Everything else read-only. No dependency additions, no reformatting. + +**Open Bindings.** + - The in-canvas ImGui field on wip/in-canvas-value-field is NOT in scope. Default: the arbiter + is implemented against the CURRENT wxFrame field on cad-mainline, because content-based + routing makes the window's focus irrelevant either way. If it later moves in-canvas the + arbiter is unchanged. + - Tools whose field is opened by a toolbar button rather than a gesture (Constrain path) are + covered by the same arbiter but are not in the F2P tool list. Default: assert them in P2P only. + +## 2. HARNESS ENVIRONMENT & GROUND TRUTH + +The rig container `orcacad-gui` on nativedev IS the harness. Xvfb `:11` + openbox, the app under +test, `xdotool` for synthetic input, and an MCP socket at `/tmp/mcp.sock` that reports sketch +state as JSON. It is a closed loop: drive input, read geometry back, assert. No window manager +politics, no human. + + Harness interface (ordered; each slot one invocation, one exit code): + S1 sync docker cp orcacad-gui:/OrcaSlicer/ + S2 build docker exec orcacad-gui ninja -C /OrcaSlicer/build orca-slicer + S3 restart docker exec orcacad-gui /OrcaSlicer/scripts/CAD/start-headless-gui.sh + S4 F2P docker exec -e DISPLAY=:11 orcacad-gui python3 /tmp/check-gui-click-edit.py --attach + S5 P2P docker exec -e DISPLAY=:11 orcacad-gui python3 /tmp/check-gui-sketching.py + +**F2P.** `scripts/CAD/check-gui-click-edit.py`. For each of Line, Rectangle, Circle, Slot, +Polygon, Ellipse and Rounded rectangle: arm the tool, draw it, and type a value that differs +from the prefill WITHOUT clicking the field. Assert the committed value equals the typed value. +The ladder must FAIL against unmodified cad-mainline — that is what proves it asserts something. + +**P2P.** `scripts/CAD/check-gui-sketching.py`, the existing gesture ladder, minus anything red at +baseline. NOTE: it calls `focus_field()` — one click into the field before typing — which is the +workaround this whole task removes. It stays green as a regression guard; it is NOT evidence. + +**Test Integrity Constraint.** `focus_field()` in check-gui-sketching.py must NOT be deleted to +make things pass, and check-gui-click-edit.py must NOT be weakened. Either invalidates the run. + +## 3. VERIFICATION COMMANDS +1. Static: `docker exec orcacad-gui ninja -C /OrcaSlicer/build orca-slicer` (warnings delta only; + this repo configures no linter — the compiler is the static gate. Absolute-zero is NOT the gate.) +2. Harness: `docker exec -e DISPLAY=:11 orcacad-gui python3 /tmp/check-gui-click-edit.py --attach` +3. Regression: `docker exec -e DISPLAY=:11 orcacad-gui python3 /tmp/check-gui-sketching.py` + +## 4. CONVERGENCE LOOP — ceiling 8 iterations +EDIT (scoped) -> EXECUTE S1..S5 -> PARSE the ladder's per-tool assertions and the [UX]/[KEYTRACE] +lines -> PATCH from the parsed cause. On ceiling without convergence: stop, report the last diff +and the unresolved failure set. Do not report success. + +F2P authoring exception: check-gui-click-edit.py is writable, and must be shown RED against +unmodified source before any source edit counts. + +## 5. TERMINATION CRITERIA +- [ ] S2 exits 0, and introduces no compiler warning absent from the baseline. +- [ ] S4 ALL_PASSED — every tool commits the typed value, no click into the field. +- [ ] S5 shows zero regressions against its recorded baseline pass count. +- [ ] F2P proven red without the fix (source stashed, ladder re-run, must FAIL). + +## 6. GUARDRAILS +Zero-assumption: no completion claim without captured stdout and exit codes. Oracle supremacy: +the ladder's verdict overrides my judgement. Blast radius: §1 files only. Baseline obligation: +run §3 once before the first edit and record it. diff --git a/scripts/CAD/check-gui-click-edit.py b/scripts/CAD/check-gui-click-edit.py new file mode 100755 index 0000000000..23d9266b47 --- /dev/null +++ b/scripts/CAD/check-gui-click-edit.py @@ -0,0 +1,636 @@ +#!/usr/bin/env python3 +"""The click-edit contract: a value field that opens must accept what is TYPED into it. + +WHY THIS EXISTS SEPARATELY FROM check-gui-sketching.py. That ladder draws geometry and grades the +result, and to make its values land it calls focus_field() — one synthetic click INTO the field +before typing. Its own docstring says why: + + WITHOUT THIS THE TYPED VALUE IS SILENTLY DISCARDED. The field is shown and raised but the + window manager does not give it the keyboard, so xdotool's digits go to the canvas and Return + commits the value the field opened with — the pre-filled as-drawn number. + +That click is a workaround for a defect, and a suite that performs it can never see the defect +again. A user cannot be told to click the field first; when they do not, they get the as-drawn +number and report "the label value is not editable". So this ladder types IMMEDIATELY after the +field opens, exactly as a person does, and fails if the prefill is what gets committed. + +WHAT IT GRADES. The app emits one line per event under SNAPORCA_UXTRACE=1: + + [UX] open title=Length prefill=154.76 + [UX] commit title=Length typed=80 value=80.0000 + [UX] refused title=Length typed=8O + [UX] cancel title=Length + +For every field the driver opens it asserts: a commit arrived, what the field received is what we +typed, the parsed value equals it, and it differs from the prefill. The last clause is the one +that matters — a field that is on screen but deaf commits its prefill, and every other signal +(the field is visible, a constraint is created, the solve succeeds) looks perfectly healthy. + + scripts/CAD/check-gui-click-edit.py --display :10 --bin build/src/Release/orca-slicer + +With --attach it drives an already-running app instead of launching one; the app must have been +started with SNAPORCA_UXTRACE=1 and its stderr redirected to --trace. +Exit 0 = every field took what was typed. +""" +import argparse, json, os, re, shutil, signal, socket, subprocess, sys, tempfile, time + +AP = argparse.ArgumentParser() +AP.add_argument("--display", default=os.environ.get("DISPLAY", ":10")) +AP.add_argument("--bin", default="build/src/Release/orca-slicer") +AP.add_argument("--datadir", default="") +AP.add_argument("--trace", default="") +AP.add_argument("--sock", default="/tmp/mcp-uxcheck.sock", + help="the app's MCP socket: the oracle for whether a sketch is really open") +AP.add_argument("--attach", action="store_true", help="drive a running app; do not launch one") +AP.add_argument("--keep", action="store_true", help="leave the app running afterwards") +AP.add_argument("--no-defocus", action="store_true", + help="do NOT take focus off the field before typing (weakens the gate; see below)") +AP.add_argument("--seed-from", default=os.path.expanduser("~/.config/OrcaCAD/OrcaSlicer.conf"), + help="an existing OrcaSlicer.conf to copy presets/settings from") +A = AP.parse_args() + +DISP = A.display +TRACE = A.trace or os.path.join(tempfile.gettempdir(), "ux-click-edit.log") +_fail = 0 +_checks = 0 + + +_n = 0 + + +def call(method, **params): + """One MCP request over the app's unix socket. The socket is the only witness that cannot + lie about sketch state: the keytrace says a key ARRIVED, a screenshot says something is on + screen, and neither distinguishes an open sketch from sketch mode with the plane offer up.""" + global _n + _n += 1 + s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + s.settimeout(30) + s.connect(A.sock) + s.sendall((json.dumps({"jsonrpc": "2.0", "id": _n, "method": method, + "params": params}) + "\n").encode()) + buf = b"" + while b"\n" not in buf: + d = s.recv(65536) + if not d: + break + buf += d + r = json.loads(buf.decode().strip()) + if "error" in r: + raise RuntimeError(f"{method}: {r['error']['message']}") + return r["result"] + + +def try_call(method, **params): + try: + return call(method, **params) + except Exception: + return None + + +def sh(cmd): + # bash -c, NOT -lc: a login shell sources the profile on every xdotool call, and this driver + # makes hundreds. On a GNOME box that meant im-config running per call, thousands of journal + # lines, and a window poll slow enough to time out before the app had finished starting. + return subprocess.run(["bash", "-c", cmd], capture_output=True, text=True).stdout + + +def xdo(args): + sh(f"DISPLAY={DISP} xdotool {args}") + + +def key(k, pause=0.35, window=None): + xdo(f"key {'--window ' + str(window) + ' ' if window else ''}{k}") + time.sleep(pause) + + +def typ(s, pause=0.35, window=None): + # --clearmodifiers so a modifier left down by an earlier synthetic key cannot turn digits + # into something else; --delay 60 because ImGui reads one character per frame. + # + # `window` targets a specific window with XSendEvent instead of following the input focus. + # That is the whole gate: see type_into_open_field. + tgt = f"--window {window} " if window else "" + xdo(f"type {tgt}--clearmodifiers --delay 60 -- '{s}'") + time.sleep(pause) + + +def die(msg): + print(f"FATAL {msg}", file=sys.stderr) + sys.exit(2) + + +# ---------------------------------------------------------------- the app + +_proc = None + + +def seed_datadir(datadir): + """The Design tab does not exist unless enable_cad_feature is on, and it needs a RESTART. + + A fresh datadir has it off, so a driver that just points the app at an empty directory gets + Prepare/Preview/Device/Project, no Design tab, and every rung fails for a reason that has + nothing to do with what is being tested. Seed the flag before the first launch. + """ + os.makedirs(datadir, exist_ok=True) + conf = os.path.join(datadir, "OrcaSlicer.conf") + data = {} + if os.path.exists(A.seed_from): + try: + with open(A.seed_from) as f: + data = json.load(f) + except Exception: + data = {} + app = data.setdefault("app", {}) + app["enable_cad_feature"] = True + # Deterministic starting state for the rungs that follow: the bed drawn, loops welded as the + # ~90% case expects. A ladder whose result depends on the developer's own preferences is not + # a gate. + app["auto_close_sketch_loops"] = True + with open(conf, "w") as f: + json.dump(data, f, indent=1) + for sub in ("user", "system", "presets", "vendor"): + src = os.path.join(os.path.dirname(A.seed_from), sub) + dst = os.path.join(datadir, sub) + if os.path.isdir(src) and not os.path.exists(dst): + shutil.copytree(src, dst) + + +def launch(): + global _proc + datadir = A.datadir or os.path.join(tempfile.gettempdir(), "orcacad-uxcheck") + seed_datadir(datadir) + env = dict(os.environ) + # WAYLAND_DISPLAY MUST GO, and GDK_BACKEND must say x11. GTK prefers Wayland whenever + # WAYLAND_DISPLAY is set and ignores DISPLAY entirely, so a driver launched from a systemd + # user unit (which inherits it) started the app on the DESKTOP session instead of the rig: + # the process was alive, `xdotool search` on the rig display found nothing, and the window + # was sitting on the user's own screen. Silent, and it drives a stray app at someone's face. + env.pop("WAYLAND_DISPLAY", None) + env.update(DISPLAY=DISP, GDK_BACKEND="x11", SNAPORCA_UXTRACE="1", + LIBGL_ALWAYS_SOFTWARE="1", GALLIUM_DRIVER="llvmpipe", + # The rig's Xvfb has no input-method daemon, and a dead ibus context makes a + # GtkEntry drop every character while the app looks fine. It cannot affect the + # in-canvas field (ImGui needs no IM) but the app has other text fields, and a + # display full of IBUS warnings has cost a whole misdiagnosis before. + GTK_IM_MODULE="gtk-im-context-simple", XMODIFIERS="@im=none", + # The key tracer is this driver's only positive signal that a keystroke reached + # the Design panel at all. Without it "the field never opened" is indistinguishable + # from "we never got into sketch mode", and the first run of this ladder reported + # seven product failures that were really one driver racing a still-loading app. + SNAPORCA_KEYTRACE="1", SNAPORCA_MCP=A.sock, + SSL_CERT_FILE="/etc/ssl/certs/ca-certificates.crt", + WEBKIT_DISABLE_DMABUF_RENDERER="1", WEBKIT_DISABLE_COMPOSITING_MODE="1") + if os.path.exists(A.sock): + os.unlink(A.sock) # a stale socket from a dead run answers nothing, slowly + log = open(TRACE, "wb") + _proc = subprocess.Popen([A.bin, "--datadir", datadir], env=env, + stdout=subprocess.DEVNULL, stderr=log) + for _ in range(120): + if win_id(): + return + time.sleep(1) + die("the app never showed a window on " + DISP) + + +def window_pid(w): + """_NET_WM_PID for a window, or 0. The property is how we tell a live app from its ghost.""" + out = sh(f"DISPLAY={DISP} xprop -id {w} _NET_WM_PID 2>/dev/null") + m = re.search(r"= *(\d+)", out) + return int(m.group(1)) if m else 0 + + +def pid_alive(pid): + return pid > 0 and os.path.isdir(f"/proc/{pid}") + + +def win_id(): + """The main window: OURS if we launched it, otherwise the biggest LIVE top-level. + + Two rules here, each paid for. + + By PID, not by size, whenever we launched the app. An X window outlives its client if the + connection is not torn down cleanly, and a killed OrcaSlicer can leave a full-screen ghost + mapped on the display. It answers geometry queries exactly like the real thing, it wins "the + biggest window" every time, and every synthetic keystroke sent to it goes nowhere. That is + indistinguishable, from the driver's side, from an app that ignores the keyboard — which is + the very defect this ladder exists to measure. One run reported the entire contract broken + while the real app sat beside the ghost, untouched. + + Never by title: a saved project renames the main window. + """ + if _proc is not None: + for w in sh(f"DISPLAY={DISP} xdotool search --pid {_proc.pid} --onlyvisible --name '.'").split(): + g = dict(l.split("=", 1) for l in + sh(f"DISPLAY={DISP} xdotool getwindowgeometry --shell {w}").strip().splitlines() + if "=" in l) + if "WIDTH" in g and int(g["WIDTH"]) * int(g["HEIGHT"]) > 400 * 400: + return (w, int(g["X"]), int(g["Y"]), int(g["WIDTH"]), int(g["HEIGHT"])) + return None + best = None + for w in sh(f"DISPLAY={DISP} xdotool search --onlyvisible --name '.'").split(): + g = dict(l.split("=", 1) for l in + sh(f"DISPLAY={DISP} xdotool getwindowgeometry --shell {w}").strip().splitlines() + if "=" in l) + if "WIDTH" not in g: + continue + if not pid_alive(window_pid(w)): # a ghost: no client is behind it any more + continue + a = int(g["WIDTH"]) * int(g["HEIGHT"]) + if a > 400 * 400 and (best is None or a > best[0]): + best = (a, w, int(g["X"]), int(g["Y"]), int(g["WIDTH"]), int(g["HEIGHT"])) + return best[1:] if best else None + + +_win = None + + +def win(): + global _win + if _win is None: + w = win_id() + if w is None: + die("no app window on " + DISP) + sh(f"DISPLAY={DISP} xdotool windowactivate --sync {w[0]}") + sh(f"DISPLAY={DISP} xdotool windowsize {w[0]} 1920 1080") + sh(f"DISPLAY={DISP} xdotool windowmove {w[0]} 0 0") + time.sleep(1.0) + _win = (w[0], 0, 0, 1920, 1080) + return _win + + +def click(px, py, pause=0.5, btn=1): + _, X, Y, _, _ = win() + xdo(f"mousemove {X+int(px)} {Y+int(py)} click --delay 120 {btn}") + time.sleep(pause) + + +def visible_windows(): + """(id, name, w, h) for every MAPPED top-level, main window included. + + `--onlyvisible` is what makes this usable. Without it xdotool also returns the app's unmapped + helper windows — a 10x10 and a 200x200 that exist for the whole session — and a caller that + tries to reason about "extra windows" from that list is reasoning about furniture. + """ + out = [] + for w in sh(f"DISPLAY={DISP} xdotool search --onlyvisible --name '.'").split(): + g = dict(l.split("=", 1) for l in + sh(f"DISPLAY={DISP} xdotool getwindowgeometry --shell {w}").strip().splitlines() + if "=" in l) + if "WIDTH" not in g: + continue + if not pid_alive(window_pid(w)): # see win_id(): a ghost cannot be closed, only ignored + continue + n = sh(f"DISPLAY={DISP} xdotool getwindowname {w}").strip() + out.append((w, n, int(g["WIDTH"]), int(g["HEIGHT"]))) + return out + + +def dismiss_modals(timeout=30): + """Close every modal over the main window, and PROVE none is left. + + This is the rung that decides whether any of the others mean anything. A fresh datadir opens + "Bambu Network Plug-in Required" — 440x259, centred at 742,450 — which sits exactly on top of + the point every drawing gesture in TOOLS starts from. The whole ladder then reports eleven + product failures, all of them the driver clicking a dialog. + + The old version pressed Escape and moved on. This dialog ignores Escape, so it "dismissed" + nothing and said so to no one; the run that found this was red for a reason that had nothing + to do with the contract under test. Escape is still tried first because it is the gentlest + thing that works on the wizard, then WM_DELETE_WINDOW, and then the function asserts what it + was supposed to have achieved instead of assuming it. + """ + # NEVER run without knowing which window to spare. The first version took `keep = main[0] if + # main else None`, so a win_id() that raced the app's mapping made keep None and every visible + # window a modal — this function then sent WM_DELETE to the app's own main window. The app + # survived as a process, printed "GdkWindow unexpectedly destroyed", and answered nothing + # afterwards; the ladder reported "no sketch opened" for 180s. Losing the main window is not a + # state to recover from silently. + deadline = time.time() + timeout + keep = None + while keep is None and time.time() < deadline: + main = win_id() + keep = main[0] if main else None + if keep is None: + time.sleep(0.5) + if keep is None: + die(f"no main window to protect after {timeout}s — refusing to close anything") + scr = sh(f"DISPLAY={DISP} xdotool getdisplaygeometry").split() + full = int(scr[0]) * int(scr[1]) if len(scr) == 2 else 1920 * 1080 + while time.time() < deadline: + # A modal is small. Anything covering half the screen is the app, whatever id win_id() + # happened to return this instant — a second belt on the rule above, because the cost of + # being wrong here is an app that looks alive and answers nothing. + extra = [x for x in visible_windows() if x[0] != keep and x[2] * x[3] < full * 0.5] + if not extra: + return + for (w, n, _, _) in extra: + sh(f"DISPLAY={DISP} xdotool windowactivate {w}") + time.sleep(0.4) + key("Escape", 0.4) + if any(x[0] == w for x in visible_windows()): + sh(f"DISPLAY={DISP} xdotool windowclose {w}") + time.sleep(0.6) + time.sleep(0.5) + left = [f"{n!r} ({w}x{h})" for (i, n, w, h) in visible_windows() + if i != keep and w * h < full * 0.5] + die("a modal is still covering the canvas after " + str(timeout) + "s: " + ", ".join(left) + + " — every drawing gesture would land in it, so nothing below this line could be trusted") + + +def dismiss_first_run(): + dismiss_modals() + + +# ---------------------------------------------------------------- the trace + +def trace_lines(): + try: + with open(TRACE, "r", errors="replace") as f: + return [l.strip() for l in f if l.startswith("[UX] ")] + except OSError: + return [] + + +def trace_mark(): + return len(trace_lines()) + + +def parse(line): + m = re.match(r"\[UX\] (\w+) title=(.*?) (.*)$", line) + if not m: + return None + ev, title, rest = m.group(1), m.group(2), m.group(3) + kv = dict(re.findall(r"(\w+)=(\S*)", rest)) + return ev, title, kv + + +# ---------------------------------------------------------------- grading + +def check(cond, what): + global _fail, _checks + _checks += 1 + if cond: + print(f" ok {what}") + else: + print(f" FAIL {what}", file=sys.stderr) + _fail += 1 + + +def type_into_open_field(value, mark): + """Type `value` into whatever field is open, WITHOUT clicking it first, and grade the pair. + + No click: the click is the workaround this ladder exists to refuse. If the field cannot take + the keyboard on its own, `typed` will be the prefill and this fails — which is the report. + """ + # POLL for the field. It opens from a CallAfter that runs after a re-solve, so on llvmpipe it + # is simply not there yet when a fast driver looks — and "no field opened" is the same message + # whether the product never opened one or the driver asked too early. Wait, then decide. + opens = [] + deadline = time.time() + 8.0 + while time.time() < deadline: + opens = [e for e in (parse(l) for l in trace_lines()[mark:]) if e and e[0] == "open"] + if opens: + break + time.sleep(0.25) + if not opens: + check(False, f"a value field opened (nothing did; cannot type {value})") + return mark + title = opens[-1][1] + prefill = opens[-1][2].get("prefill", "") + m2 = trace_mark() + # REPRODUCE THE FAILING CONDITION ON PURPOSE, rather than hoping the window manager supplies + # it. The defect is "the value field is open but does not hold the keyboard", which is what + # mutter does on the user's GNOME desktop and what openbox — the rig's WM — never does. A + # ladder that just types on openbox is green at baseline and proves nothing: the gate could + # not fail, so it could not pass meaningfully either. + # + # So take the keyboard AWAY from the field first, deliberately, by activating the main + # window. That is exactly the state mutter leaves behind, reproduced deterministically on any + # WM. A build that routes keys by content is unaffected; a build that routes by focus commits + # its prefill, which is the bug, and the assertion below catches it. + if not A.no_defocus: + w, _, _, _, _ = win() + # windowfocus, NOT windowactivate. `windowactivate` sets _NET_ACTIVE_WINDOW — it ASKS the + # window manager, and openbox obliges by marking the field's toplevel inactive while + # leaving the X input focus exactly where it was. Measured: the trace read + # "active=0 toplevel_focus=0" and every keystroke still reached the field, so the ladder + # passed against a binary with the arbiter compiled out. A gate that cannot fail cannot + # pass meaningfully either, and that run nearly shipped as proof. + # + # `windowfocus` calls XSetInputFocus, which is what actually decides where the server + # delivers keys. That reproduces the real defect — field on screen, keyboard elsewhere — + # on any WM, instead of hoping the local one volunteers it. + # DELIVER THE KEYS TO THE MAIN WINDOW, not to whatever holds the focus. + # + # This is the defect, reproduced exactly and without a focus fight. On the user's GNOME + # desktop mutter's focus-stealing prevention refuses the borderless field toplevel the + # keyboard, so the digits are delivered to the Design panel — which is precisely what + # DesignPanel's CHAR_HOOK comment describes. Only routing by CONTENT can get them from + # there into the field; a build that routes by focus commits its prefill. + # + # Stealing the focus instead does NOT work and must not be reinstated: measured on + # openbox, the app re-asserts SetFocus from open()'s CallAfter and wins every race — four + # retries of XSetInputFocus all lost, `xdotool getwindowfocus` came back as the field's + # own toplevel every time. Two full runs passed against a binary with the arbiter + # compiled out because of it. Targeting the window sidesteps the question entirely. + target = None + if not A.no_defocus: + target, _, _, _, _ = win() + typ(str(value), 0.4, window=target) + key("Return", 0.9, window=target) + after, commits, refused, commit_at = [], [], [], None + deadline = time.time() + 5.0 + while time.time() < deadline: + after = [parse(l) for l in trace_lines()[m2:]] + commits = [(i, e) for i, e in enumerate(after) if e and e[0] == "commit"] + refused = [e for e in after if e and e[0] == "refused"] + if commits: + commit_at = m2 + commits[-1][0] + commits = [e for _, e in commits] + if commits or refused: + break + time.sleep(0.25) + if refused and not commits: + check(False, f"{title}: field REFUSED {value!r} (typed={refused[-1][2].get('typed')!r})") + key("Escape", 0.5) + return trace_mark() + if not commits: + check(False, f"{title}: typed {value} but nothing committed — the field took no keys") + key("Escape", 0.5) + return trace_mark() + typed = commits[-1][2].get("typed", "") + got = commits[-1][2].get("value", "") + check(typed == str(value), + f"{title}: field received what was typed (typed={typed!r} wanted={value!r}" + f"{' <-- it committed its PREFILL, so it never got the keyboard' if typed == prefill else ''})") + # A value that will not parse is a FAILED CHECK, never an exception. An unguarded float() + # here met a locale-formatted "61,0000" and took the whole run down immediately after the + # first check in the ladder's history had passed — the seven rungs below it were never tried + # and the report read as a total failure. + try: + ok_val = abs(float(got) - float(value)) < 1e-6 + except (TypeError, ValueError): + ok_val = False + check(ok_val, f"{title}: committed value is {got!r} (wanted {value})") + check(str(value) != prefill, f"{title}: the test value differs from the prefill {prefill!r}") + # RESUME JUST AFTER THE COMMIT, not at the end of the trace. A queued chain opens its next + # field from the commit callback, so by the time trace_mark() is read here that "open" line + # is already written — and the next call, searching only after this mark, never sees it. The + # rectangle's Height, the slot's Radius and the label reopen all failed as "nothing did" + # while the trace plainly showed the field open and waiting. + return (commit_at + 1) if commit_at is not None else trace_mark() + + +# ---------------------------------------------------------------- the ladder + +def enter_sketch(timeout=180): + """Open a real sketch on a real plane, and PROVE it with the socket before drawing anything. + + THE SEQUENCE MATTERS AND IT IS NOT OBVIOUS. Shift+S enters sketch MODE and pops the plane + offer; the offer must be dismissed; and the plane itself is chosen by clicking it in the + viewport BEFORE Shift+S. check-gui-sketching.py has always done all four steps. This ladder + did two of them — Design tab, then Shift+S — and went straight to the tool letters. + + That intermediate state is the trap. `is_sketching` reads 1, every tool key is accepted and + traced, and not one click draws anything, because there is no plane under them. The ladder + then reports eleven product failures, all of them "a value field opened (nothing did)", and + every one is the driver's. Two whole runs were spent on it. + + So the gate is the ORACLE, not the keytrace: sketch_describe answers only when a sketch is + genuinely open. Waiting on a mode flag is what allowed the wrong state to pass for the right + one in the first place. + """ + deadline = time.time() + timeout + while time.time() < deadline: + click(132, 53) # Design tab + time.sleep(2.0) + dismiss_modals() + click(*PLANE_PX) # pick the plane IN THE VIEWPORT — before Shift+S + key("shift+s", 1.0) + key("Escape", 0.5) # entering sketch mode pops the offer; dismiss it + key("p", 0.6) # any sketch tool starts the session on that plane + if try_call("sketch_describe") is not None: + # NO Escape here. Every rung already opens with one to drop whatever tool the last + # one left armed, and Escape in the Design tab walks a LIFO: first press drops the + # armed tool, second LEAVES THE SKETCH. Pressing it here made that second press the + # rung's own, so the ladder exited the sketch before drawing anything and then + # reported all eleven checks failed with "nothing opened" — the tools were arming + # into an empty Feature-mode document. + return + die("no sketch opened after plane click + Shift+S within " + f"{timeout}s — sketch_describe never answered on {A.sock} (trace {TRACE})") + + +# tool key, the clicks that draw it, and one distinct value per queued field. The values are +# deliberately nothing like the as-drawn size, so a committed prefill cannot coincide with them. +# Where the plane label sits in the viewport before a sketch is open. Same constant the gesture +# ladder uses; it is a label on the 3D view, not a widget, so it moves only if the camera does. +PLANE_PX = (913, 359) + +# Every coordinate below stays inside 1000..1400 x 500..760 — the box check-gui-sketching.py's +# calibration probes land four Points in, i.e. the region PROVEN to be live canvas on a 1920x1080 +# window. Earlier values started at x=950, which is left of that box and also, on a fresh datadir, +# underneath the "Bambu Network Plug-in Required" modal. +TOOLS = [ + ("L", "Line", [(1030, 540), (1360, 540)], [61]), + ("R", "Rectangle", [(1030, 540), (1360, 730)], [62, 43]), + ("C", "Circle", [(1180, 620), (1330, 620)], [64]), + ("S", "Slot", [(1030, 580), (1300, 580), (1300, 640)], [66]), + ("G", "Polygon", [(1180, 620), (1320, 620)], [67]), + ("E", "Ellipse", [(1180, 620), (1370, 620), (1180, 720)], [68]), + ("A", "Arc", [(1040, 660), (1340, 660), (1190, 560)], [69]), +] + + +def rung_tool(k, name, clicks, values): + print(f" {name}") + key("Escape", 0.6) # back to Select, whatever the last tool left armed + # Every rung re-establishes that a sketch is STILL open. One stray Escape too many leaves it, + # and from then on every tool arms into a Feature-mode document that cannot open a value + # field — which the checks below report as eleven independent product failures. + if try_call("sketch_describe") is None: + die(f"{name}: the sketch is no longer open before this rung — an earlier rung left it") + key(k, 0.8) + # MARK BEFORE THE CLICKS, not after. The field is opened from a CallAfter scheduled by the + # render that follows the last click, so it can already be open by the time a mark taken + # afterwards is read — and type_into_open_field, which only looks at events AFTER its mark, + # then finds none and reports "a value field opened (nothing did)" for a field that is on + # screen, open, and waiting. That message accused the product of the exact defect the ladder + # exists to detect, from a bug in the ladder's own bookkeeping. + mark = trace_mark() + for (x, y) in clicks: + click(x, y) + for v in values: + mark = type_into_open_field(v, mark) + + +def rung_label_click(): + """The user's own report: click an existing dimension label and type a new value into it. + + KEEP THE SHAPE AS DRAWN. An earlier version committed 55 and 47 into the queued chain first, + which resized the rectangle — and then clicked the pixel where the label had been before the + resize. It missed, every time, and reported the reopen broken. The shape's on-screen position + is only predictable if nothing has moved it, so Escape the chain instead: the rectangle stays + exactly between the two corners we clicked. + + FIND THE LABEL, do not assume its offset. A dimension label is drawn beside its edge at an + offset that depends on zoom and text metrics, so a single hardcoded pixel is a guess that + silently becomes wrong. Walk a short band across the top edge instead and stop at the first + click that opens a field; if none of them does, that is a real failure and it says so. + """ + print(" label click-to-edit") + key("Escape", 0.6) # Select mode + key("R", 0.8) + click(1020, 530) + click(1350, 740) + time.sleep(1.5) + key("Escape", 0.8) # keep as drawn: abandon the queued value chain + time.sleep(0.8) + key("Escape", 0.6) # back to Select so a click picks rather than draws + + mid_x, top_y = (1020 + 1350) // 2, 530 + candidates = [(mid_x, top_y + dy) for dy in (-26, -20, -14, -8, 0, 8, 14)] + for (cx, cy) in candidates: + mark = trace_mark() + click(cx, cy) + deadline = time.time() + 2.0 + while time.time() < deadline: + if [e for e in (parse(l) for l in trace_lines()[mark:]) if e and e[0] == "open"]: + check(True, f"clicking a dimension label reopened its value field (at {cx},{cy})") + type_into_open_field(71, mark) + return + time.sleep(0.2) + check(False, "clicking a dimension label reopened its value field " + f"(tried {len(candidates)} points across the top edge at x={mid_x})") + + +def main(): + if not A.attach: + if not os.path.exists(A.bin): + die(f"no binary at {A.bin}") + open(TRACE, "w").close() + launch() + dismiss_first_run() + win() + print(f"click-edit ladder on {DISP}, trace {TRACE}") + enter_sketch() + for (k, name, clicks, values) in TOOLS: + rung_tool(k, name, clicks, values) + rung_label_click() + print() + if _fail: + print(f"CLICK-EDIT LADDER FAILED — {_fail} of {_checks} checks", file=sys.stderr) + else: + print(f"CLICK-EDIT LADDER HELD — {_checks} checks") + if _proc is not None and not A.keep: + _proc.send_signal(signal.SIGTERM) + try: + _proc.wait(20) + except subprocess.TimeoutExpired: + _proc.kill() + return 1 if _fail else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/CAD/focus-loop.sh b/scripts/CAD/focus-loop.sh new file mode 100755 index 0000000000..28cfec37ab --- /dev/null +++ b/scripts/CAD/focus-loop.sh @@ -0,0 +1,85 @@ +#!/usr/bin/env bash +# One turn of the keyboard-focus convergence loop, start to verdict, with no human in it. +# +# scripts/CAD/focus-loop.sh # full turn: sync -> build -> restart -> assert +# SKIP_BUILD=1 scripts/CAD/focus-loop.sh # re-assert against the binary already on the host +# +# Exit 0 only when every gate holds. Any other exit is a failing gate and names which. +# +# WHY THIS EXISTS. The focus defects in the Design tab were chased for days by hand: build, launch +# the GUI, drive it with xdotool, read a screenshot, guess, repeat. That needs a person at every +# step and it is where the days went. This does not: behemoth carries an agent-owned Xvfb :10 with +# openbox, the app, xdotool and an MCP socket that reports sketch state as JSON, so a turn is +# sync -> build -> restart -> assert, and the ASSERTION is the verdict, not my reading of a picture. +# +# WHY BEHEMOTH AND NOT THE orcacad-gui RIG CONTAINER. The rig was the obvious host and it does not +# work for this: its image pins a dependency set 216 non-CAD source files behind cad-mainline +# (assimp among them), so today's CAD sources call GUI_App::is_auto_close_sketch_loops and +# MainFrame::ensure_design_panel, which that tree has never heard of. Syncing all of src/ to fix +# that needs a deps rebuild measured in hours. behemoth already builds this exact tree, already +# runs a WM on :10, and is the machine the user actually runs the product on — so the loop asserts +# against the shipping artefact rather than a stale twin. Reviving the rig means rebuilding its +# deps image first; until then it cannot adjudicate anything about this code. +set -uo pipefail + +HOST="${HOST:-tommaso@100.103.234.2}" +DISP="${DISP:-:10}" +SRC="${SRC:-\$HOME/projects/orca/orcacad-native/src}" +TRACE="${TRACE:-/tmp/ux-focus-loop.log}" +REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +BIN="build/src/Release/orca-slicer" + +say() { printf '\n=== %s\n' "$*"; } +die() { printf 'GATE FAILED: %s\n' "$*" >&2; exit 1; } + +ssh -o ConnectTimeout=10 "$HOST" true || die "cannot reach $HOST" + +# ---------------------------------------------------------------- S1 sync +# Only the CAD paths and the ladders. behemoth's tree is a full cad-mainline checkout kept in step +# by its own realign; pushing unrelated files from here would make the build host disagree with +# git for reasons no later session could reconstruct. +say "S1 sync" +rsync -q "$REPO"/src/slic3r/GUI/CAD/*.{cpp,hpp} "$HOST:$SRC/src/slic3r/GUI/CAD/" || die "sync GUI/CAD" +rsync -q "$REPO"/src/libslic3r/CAD/*.{cpp,hpp} "$HOST:$SRC/src/libslic3r/CAD/" || die "sync libslic3r/CAD" +rsync -q "$REPO"/scripts/CAD/check-gui-click-edit.py "$REPO"/scripts/CAD/check-gui-sketching.py \ + "$HOST:/tmp/" || die "sync ladders" +echo " sources + ladders in place" + +# ---------------------------------------------------------------- S2 build +# flock: two concurrent Orca builds once OOM'd this machine for 2h28m. Every build script on the +# fleet takes this same lock. +# +# Grade the BINARY'S TIMESTAMP, never the build command's exit code. This is a Ninja Multi-Config +# tree whose default rules are Debug while the artefact under test is Release, so a wrong-config +# invocation returns success in seconds having touched nothing — it cost a wasted cycle here +# before anyone thought to look at the file. +if [ -z "${SKIP_BUILD:-}" ]; then + say "S2 build" + before=$(ssh "$HOST" "stat -c %Y $SRC/$BIN 2>/dev/null || echo 0") + ssh "$HOST" "flock /tmp/orca-rig-build.lock \$HOME/projects/orca/orcacad-native/rebuild.sh > /tmp/focus-build.log 2>&1" + rc=$? + after=$(ssh "$HOST" "stat -c %Y $SRC/$BIN 2>/dev/null || echo 0") + if [ "$rc" != 0 ] || [ "$after" = "$before" ]; then + ssh "$HOST" "grep -m5 -B2 'error:' /tmp/focus-build.log; tail -5 /tmp/focus-build.log" + die "S2 build (exit $rc, binary $( [ "$after" = "$before" ] && echo unchanged || echo rebuilt ))" + fi + echo " built" +fi + +# ---------------------------------------------------------------- S3 F2P +# The ladder launches and tears down the app itself, in its own datadir, so nothing here has to +# manage a process. It types WITHOUT clicking the field first, which is the whole contract. +say "S3 fail-to-pass: type without clicking the field" +ssh "$HOST" "cd $SRC && DISPLAY=$DISP python3 /tmp/check-gui-click-edit.py \ + --display $DISP --bin $BIN --trace $TRACE" +f2p=$? + +# ---------------------------------------------------------------- S4 P2P +say "S4 pass-to-pass: the existing gesture ladder" +ssh "$HOST" "cd $SRC && DISPLAY=$DISP python3 /tmp/check-gui-sketching.py 2>&1 | tail -3" +p2p=$? + +say "VERDICT" +[ "$f2p" = 0 ] || die "F2P: a tool did not take the typed value (exit $f2p)" +[ "$p2p" = 0 ] || die "P2P: the gesture ladder regressed (exit $p2p)" +echo "ALL GATES HELD" diff --git a/src/slic3r/GUI/CAD/DesignCanvas.cpp b/src/slic3r/GUI/CAD/DesignCanvas.cpp index ef37cb055a..ff2d27ad66 100644 --- a/src/slic3r/GUI/CAD/DesignCanvas.cpp +++ b/src/slic3r/GUI/CAD/DesignCanvas.cpp @@ -1339,6 +1339,11 @@ bool DesignCanvas::inline_has_focus() const return m_inline_editor && m_inline_editor->has_focus(); } +bool DesignCanvas::inline_type_char(int key) +{ + return m_inline_editor && m_inline_editor->type_char(key); +} + void DesignCanvas::inline_commit() { if (m_inline_editor) m_inline_editor->commit(); diff --git a/src/slic3r/GUI/CAD/DesignCanvas.hpp b/src/slic3r/GUI/CAD/DesignCanvas.hpp index 1da8a17cf4..cb1e517f7a 100644 --- a/src/slic3r/GUI/CAD/DesignCanvas.hpp +++ b/src/slic3r/GUI/CAD/DesignCanvas.hpp @@ -244,6 +244,8 @@ public: void delete_selected_sketch_entities(); bool inline_busy() const; // a sketch value field is open (guard keys) bool inline_has_focus() const; // the field itself holds keyboard focus + // Hand one character to the open value field, bypassing focus. See DesignPanel's arbiter. + bool inline_type_char(int key); void inline_commit(); // accept the typed value (Enter/Tab) void inline_cancel(); // discard the typed value (Esc) // The layered Esc: abandon the points of the gesture in progress, else drop the armed tool diff --git a/src/slic3r/GUI/CAD/DesignPanel.cpp b/src/slic3r/GUI/CAD/DesignPanel.cpp index 4165d752db..4d767ddd5e 100644 --- a/src/slic3r/GUI/CAD/DesignPanel.cpp +++ b/src/slic3r/GUI/CAD/DesignPanel.cpp @@ -4205,6 +4205,25 @@ DesignPanel::DesignPanel(wxWindow* parent) m_viewport->inline_commit(); return; } + // THE ARBITER. Route by what the key IS, not by who the window manager focused. + // + // This is FreeCAD's rule, from Sketcher's DrawSketchKeyboardManager:: + // detectKeyboardEventHandlingMode: a digit, a sign, a decimal separator or a + // Backspace/Delete is unambiguously meant for the number the user is entering; a + // letter is unambiguously a tool shortcut; Enter/Tab hand control back to the view. + // FreeCAD never queries focus anywhere in that decision, and that is precisely why + // its sketcher behaves the same on every desktop. + // + // Ours asked "who has focus?" instead — a question whose answer is the window + // manager's opinion. openbox grants this borderless top-level the keyboard, mutter + // refuses it, so the same binary took typed values on one machine and silently + // committed the pre-filled as-drawn number on another. Seven workarounds fought that + // and one of them cost a macOS regression. The question was wrong, not the answers. + // + // The has_focus() guard above keeps this from double-typing where the toolkit DID + // give the field the keyboard: there the field's own binding will get the key too. + if (!ctrl && m_viewport->inline_type_char(key)) + return; // Esc is NOT special-cased here any more: escape() routes it, and the open field is // exactly what CadLevel::Transient means, so it closes the field and stops there. } diff --git a/src/slic3r/GUI/CAD/DesignSketchTool.cpp b/src/slic3r/GUI/CAD/DesignSketchTool.cpp index b0e557cd6c..45c215ff34 100644 --- a/src/slic3r/GUI/CAD/DesignSketchTool.cpp +++ b/src/slic3r/GUI/CAD/DesignSketchTool.cpp @@ -1383,10 +1383,22 @@ std::string DesignSketchTool::dimtype_title(DimType k) const { // Draw-then-edit dispatcher: mirror the Select-mode quote-click logic, but target the // freshly-drawn selection's PRIMARY value and use the tentative (clean-cancel) path for // scalar quotes. Runs after render_live_quotes, so the live-quote state is populated. +// Why a draw-then-edit chain did not start. Four early returns can swallow it, and from outside +// they are indistinguishable: the shape appears, no field opens, and nothing says which guard +// fired. check-gui-click-edit.py reports that as "a value field opened (nothing did)" for every +// tool at once, which reads like a total product failure and is not necessarily one. +static void trace_autoedit(const char* why, size_t n) +{ + if (!std::getenv("SNAPORCA_UXTRACE")) return; + fprintf(stderr, "[UX] autoedit %s steps=%zu\n", why, n); + fflush(stderr); +} + void DesignSketchTool::open_primary_autoedit() { - if (!on_inline_edit || m_awaiting_length) return; // no host, or a field is already open - if (!m_active) return; // session ended before the deferred tick + if (!on_inline_edit) { trace_autoedit("skip: no on_inline_edit host", 0); return; } + if (m_awaiting_length) { trace_autoedit("skip: a field is already open", 0); return; } + if (!m_active) { trace_autoedit("skip: session ended before the deferred tick", 0); return; } // Build ONE ordered list of edit steps covering EVERY characteristic dimension of the // freshly-drawn shape — scalar quotes (constraint-based) AND geometric editors — so every @@ -1494,6 +1506,8 @@ void DesignSketchTool::open_primary_autoedit() [this, fi](double v){ set_rect_angle(fi, v); }, span(fi), "Angle" }); } + trace_autoedit(m_autoedit_dims.empty() ? "built NO steps (no live quote matched)" : "opening", + m_autoedit_dims.size()); if (!m_autoedit_dims.empty()) { m_autoedit_dim_idx = 0; open_next_autoedit_dim(); @@ -8857,6 +8871,7 @@ void DesignSketchTool::render(GLCanvas3D& canvas) // next render_live_quotes(), so the deferred open still sees this frame's values. if (m_autoedit_pending) { m_autoedit_pending = false; + trace_autoedit("pending -> deferring open", 0); wxGetApp().CallAfter([this] { open_primary_autoedit(); }); } if (is_edit_op_mode()) diff --git a/src/slic3r/GUI/CAD/SketchInlineEditor.cpp b/src/slic3r/GUI/CAD/SketchInlineEditor.cpp index 738d6d5470..c1321e2511 100644 --- a/src/slic3r/GUI/CAD/SketchInlineEditor.cpp +++ b/src/slic3r/GUI/CAD/SketchInlineEditor.cpp @@ -90,6 +90,23 @@ constexpr bool keep_mapped_between_fields = false; #endif +// The click-edit contract, made observable. scripts/CAD/check-gui-click-edit.py grades a build +// on these four lines and nothing else, because they are the only place the distinction it cares +// about is visible: a field that is on screen but deaf commits its PREFILL, and every other +// signal — the field drew, a constraint appeared, the solve succeeded — looks perfectly healthy +// either way. `typed` is what the control actually held when Enter arrived; `prefill` is what +// open() put there. typed == prefill on a commit means the keyboard never reached the field. +// +// stderr, one line, no buffering, only under SNAPORCA_UXTRACE: this is a test surface, not +// logging, and it must cost nothing in a normal run. +void trace_ux(const char* event, const std::string& title, const std::string& kv) +{ + if (!std::getenv("SNAPORCA_UXTRACE")) return; + fprintf(stderr, "[UX] %s title=%s%s%s\n", event, title.c_str(), + kv.empty() ? "" : " ", kv.c_str()); + fflush(stderr); +} + void trace_inline_focus(wxFrame* frame, const std::string& title) { if (!std::getenv("SNAPORCA_KEYTRACE")) return; @@ -214,6 +231,8 @@ void SketchInlineEditor::open(const wxPoint& screen_px, double value, m_ctrl->SetFocus(); m_ctrl->SelectAll(); m_open = true; + m_prefill = m_ctrl->GetValue(); + trace_ux("open", title, "prefill=" + std::string(m_prefill.utf8_str())); trace_inline_focus(m_frame, title); // Re-assert on the next tick too: the GL canvas can reclaim focus while it finishes // handling the click/render that opened us, so a single immediate SetFocus may be stolen. @@ -235,6 +254,8 @@ void SketchInlineEditor::do_commit() // Silence here read as a freeze: Enter did nothing, the text re-selected itself, and // nothing on screen said the value had been refused or what would be accepted. Every // other CAD names the problem in place; so do we. + trace_ux("refused", std::string(m_title_text.utf8_str()), + "typed=" + std::string(m_ctrl->GetValue().utf8_str())); flag_invalid(m_ctrl->GetValue().Strip(wxString::both).IsEmpty() ? _L("Enter a number") : _L("Not a number")); @@ -242,6 +263,17 @@ void SketchInlineEditor::do_commit() m_ctrl->SelectAll(); return; } + { + // LOCALE-INVARIANT on purpose. printf honours the app's locale, which on an Italian + // desktop makes this "61,0000" — and the ladder that reads it does float(), which raises + // on a comma and takes the whole run down one check after the first success. A machine + // surface must not change shape with the user's regional settings. + char buf[64]; + snprintf(buf, sizeof(buf), "%.4f", v); + for (char* c = buf; *c; ++c) if (*c == ',') *c = '.'; + trace_ux("commit", std::string(m_title_text.utf8_str()), + "typed=" + std::string(m_ctrl->GetValue().utf8_str()) + " value=" + buf); + } auto cb = m_commit; m_open = false; // logically closed; whether it stays MAPPED is per-toolkit m_commit = nullptr; @@ -262,6 +294,53 @@ void SketchInlineEditor::do_commit() }); } +// Deliver one character into the field without the window manager's permission. +// +// This is the whole content-based-routing idea in one function: the caller has already decided, +// from the KEY ITSELF, that this keystroke belongs to a number field, so the field takes it — +// whether or not any window manager saw fit to give it focus. FreeCAD's sketcher works exactly +// this way and never asks who is focused. +bool SketchInlineEditor::type_char(int key) +{ + if (!m_open || m_ctrl == nullptr) return false; + + if (key == WXK_BACK || key == WXK_DELETE) { + long from = 0, to = 0; + m_ctrl->GetSelection(&from, &to); + if (from != to) { + m_ctrl->Remove(from, to); + } else { + const long ip = m_ctrl->GetInsertionPoint(); + if (key == WXK_BACK) { if (ip > 0) m_ctrl->Remove(ip - 1, ip); } + else { if (ip < m_ctrl->GetLastPosition()) m_ctrl->Remove(ip, ip + 1); } + } + clear_invalid(); + return true; + } + + // The numeric keypad reports its own key codes, and a keypad is exactly what someone typing + // dimensions all day uses. + int ch = key; + if (key >= WXK_NUMPAD0 && key <= WXK_NUMPAD9) ch = '0' + (key - WXK_NUMPAD0); + else if (key == WXK_NUMPAD_DECIMAL) ch = '.'; + else if (key == WXK_NUMPAD_SUBTRACT) ch = '-'; + + const bool numeric = (ch >= '0' && ch <= '9') || ch == '-' || ch == '+' || ch == '.' || ch == ','; + if (!numeric) return false; + + // A decimal COMMA is normalised to a point on the way in: this field feeds a CAD kernel and + // the rest of the file already promises a point whatever the locale (see fmt_value). + if (ch == ',') ch = '.'; + + // WriteText replaces the current selection — and open() left the whole prefill selected, so + // the FIRST character typed replaces the as-drawn value and the rest append. That is the + // behaviour a person expects from a pre-selected field, obtained for free rather than + // reimplemented. + m_ctrl->WriteText(wxString(wxUniChar(ch))); + clear_invalid(); + return true; +} + void SketchInlineEditor::cancel() { if (m_open) do_cancel(); @@ -345,6 +424,7 @@ void SketchInlineEditor::clear_invalid() void SketchInlineEditor::do_cancel() { if (!m_open) return; + trace_ux("cancel", std::string(m_title_text.utf8_str()), ""); auto cb = m_cancel; close(); if (cb) cb(); diff --git a/src/slic3r/GUI/CAD/SketchInlineEditor.hpp b/src/slic3r/GUI/CAD/SketchInlineEditor.hpp index e0339b28e0..b5df1cb46f 100644 --- a/src/slic3r/GUI/CAD/SketchInlineEditor.hpp +++ b/src/slic3r/GUI/CAD/SketchInlineEditor.hpp @@ -49,8 +49,23 @@ private: public: // True when the field itself holds keyboard focus. Callers use this to decide whether the // field will handle a key on its own or needs it forwarded — see DesignPanel's CHAR_HOOK. + // + // NOTE what this is NOT for any more: deciding whether the field may receive a character. + // Whether a borderless top-level window is granted focus is the window manager's call and + // differs per desktop — openbox grants it, mutter refuses it — so a routing rule built on + // this question gives a different product on every machine. Routing is now by CONTENT + // (DesignPanel's arbiter); this stays only to avoid forwarding a key the field is already + // going to get for itself, which would type it twice. bool has_focus() const { return m_ctrl != nullptr && wxWindow::FindFocus() == m_ctrl; } + // Deliver one character into the field programmatically, bypassing focus entirely. + // `key` is a wx key code: a printable character is inserted, WXK_BACK/WXK_DELETE edit. + // Returns true if the field consumed it. Modelled on FreeCAD, whose sketcher decides where a + // key belongs from the key itself and never queries focus: + // DrawSketchKeyboardManager::detectKeyboardEventHandlingMode routes digits, '-', '.', ',' + // and Backspace/Delete to the on-view parameter and everything else to the view. + bool type_char(int key); + private: void do_commit(); void do_cancel(); @@ -70,6 +85,7 @@ private: bool m_open{false}; bool m_closing{false}; wxString m_title_text; // the real title, restored after an error message + wxString m_prefill; // what open() put in the field; see trace_ux }; }} // namespace Slic3r::GUI