From ed1a8242b7c78c55d8713e836d0f379c173ed7b3 Mon Sep 17 00:00:00 2001 From: Simon Dick Date: Sun, 9 Aug 2026 14:33:44 +0100 Subject: [PATCH] Add pointer-based MENUPICK fallback for shortcut-less menu items (#63) MENUPICK previously only worked for items with a real keyboard shortcut. Items without one are now picked via a genuine synthesized right-mouse-button down/move/move/release sequence instead of being rejected outright, matching the "real input.device events, not a shortcut" principle every other verb already follows. Two real findings from live testing under Copperline: - RMB-down alone only switches the screen's title bar into menu mode; the pointer must then be moved onto the target menu's own title text before the pulldown actually opens. - A one-level submenu's screen-absolute box left-anchors flush against its parent item's right edge (parentLeft + parentWidth) -- the sub-item's own LeftEdge plays no part in X placement, unlike its TopEdge which is a genuine per-item stacking offset. The RKRM doesn't document this placement at all. Verified end to end against two new shortcut-less items on fixtures/gadtools-app's own menu strip (a top-level item and a one-level submenu item), plus the full existing regression suite (click/type/menu/golden-tree checks, 234 host unit tests). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018CbPo4nMpnM5JQX3LKE5JP --- CLAUDE.md | 40 ++++++- fixtures/gadtools-app/src/main.c | 43 ++++++- server/README.md | 81 ++++++++++---- server/include/action_engine.h | 58 ++++++++-- server/src/action.c | 185 ++++++++++++++++++++++++++++++- server/src/amipilotserver/main.c | 33 +++++- tests/copperline/README.md | 39 +++++++ tests/copperline/menu-test.py | 47 ++++++-- tests/copperline/run.sh | 2 + userdocs/ARexx-Reference.md | 2 +- userdocs/Changelog.md | 8 ++ userdocs/Wire-Protocol.md | 36 ++++-- 12 files changed, 503 insertions(+), 71 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5701816..cc16527 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,6 +64,38 @@ genuinely standard, ordinary ARexx port -- nothing dedicated-port-only about this fix, any third-party implementer (e.g. AmiAuth) needs only the same one-condition change. +`MENUPICK`'s pointer-based fallback for shortcut-less items is real +too now (issue #63, not yet in a tagged release): previously, +`MENUPICK` only worked for items with a real keyboard shortcut +(`AmipMenuPickByShortcut()`); an item with none was rejected outright. +`AmipMenuPickByPointer()` (`server/src/action.c`) now drives the +pointer path automatically for such items -- a genuine synthesized +RMB-down/move/move/RMB-up sequence, the same "real input.device +events, not a shortcut" principle `CLICK`/`DRAG` already follow, not a +forged `IDCMP_MENUPICK` message (considered and rejected: it would +bypass real IDCMP/activation/`WFLG_RMBTRAP`/`IDCMP_MENUVERIFY` +handling entirely, a false-positive risk this project's own design +principle exists to prevent). Two real, non-obvious things had to be +found live (2026-08-09) before this worked: (1) RMB-down alone only +switches the screen's own title bar into menu mode -- the pulldown +itself doesn't open until the pointer is actually moved onto the +target menu's own title text, confirmed only after a plain RMB-down +produced zero observable effect (no pulldown render, no fallback +`IDCMP_MOUSEBUTTONS`) even with the pointer already positioned inside +the window and a full second of `Delay()`; and (2) the RKRM documents +no formula at all for a submenu's own screen position (only "overlaps +its parent item's own select box somewhere") -- the real placement +(`parentLeft + parentWidth` for X, ignoring the sub-item's own +`LeftEdge` entirely; `parentTop + item->TopEdge` for Y) was measured +pixel-for-pixel against a real screenshot captured mid-pick under +Copperline (`amipilot.screenshot`'s own PNG conversion, cropped and +scanned for the popup's actual border pixels) after an initial guess +(adding `item->LeftEdge` on top of the parent's right edge) overshot +the box entirely. See `server/README.md`'s Menus section for the full +mechanism and `fixtures/gadtools-app`'s new shortcut-less "Toggle" and +"Sub NoShortcut" items, verified via `run_menu_check`'s +`MENUPICK-TOGGLE-POINTER`/`MENUPICK-SUBITEM-POINTER` checks. + Phase 0.5 (reliability and reach into the wider ecosystem) before it: `WAITFOR` (including its `TEXT=` condition) and `CLICK`'s `EXPECT=` (wait/expectation @@ -141,10 +173,10 @@ public default password, no rate-limiting; LAN/trusted-network use only, see `server/README.md`'s TCP section. Phase 0.4 additions beyond TCP: `LAUNCH` (start a test subject over the wire), the allowlist-scoped file API (`FSLIST`/`FSSTAT`/`FSMKDIR`/`FSDELETE`/ -`FSGET`, `server/src/fs.c`), menu walking + shortcut-based selection -(`MENU`/`MENUPICK`, `intuition-model`'s `AmipWalkMenuStrip()` — -pointer-based selection for shortcut-less items not yet built), -multi-screen support (`SCREENS`/`SCREEN=`, keyed off +`FSGET`, `server/src/fs.c`), menu walking + selection (`MENU`/ +`MENUPICK`, `intuition-model`'s `AmipWalkMenuStrip()` — shortcut-based +and, since issue #63, genuine pointer-based selection too for items +with no shortcut), multi-screen support (`SCREENS`/`SCREEN=`, keyed off `Screen->DefaultTitle`, not the live `Title` field), tier-2 semantic locators (`ROLE=`/`LABEL=`/`INDEX=` on CLICK/TYPE/GETTEXT's classic form, resolved via a fresh `AmipWalkWindow()` walk — proximity-to- diff --git a/fixtures/gadtools-app/src/main.c b/fixtures/gadtools-app/src/main.c index 9863d92..f493c17 100644 --- a/fixtures/gadtools-app/src/main.c +++ b/fixtures/gadtools-app/src/main.c @@ -48,11 +48,19 @@ * string gadget's text so a MENUPICK-by-shortcut round trip is * observable via the existing GETTEXT path), a "Toggle" checkmark * item (CHECKIT|MENUTOGGLE, starts checked -- exercises the walker's - * checkit/checked fields), a permanently "Disabled" item (no - * shortcut, ITEMENABLED off), a separator bar, and a "More" item with - * one submenu entry "Sub Item" (shortcut S, also sets the Host text) - * -- exercises the one-level-deep submenu walk and its own shortcut - * addressing (menu 0, item 4, sub 0). + * checkit/checked fields, and is also the fixture's only enabled, + * shortcut-less leaf item, so it doubles as the MENUPICK + * pointer-based-fallback target, issue #63 -- its own IDCMP_MENUPICK + * marker write proves a real RMB-down/move/RMB-up round trip reached + * Intuition's own menu tracking, not just that a shortcut keystroke + * was struck), a permanently "Disabled" item (no shortcut, + * ITEMENABLED off), a separator bar, and a "More" item with two + * submenu entries: "Sub Item" (shortcut S, also sets the Host text -- + * exercises the one-level-deep submenu walk and its own shortcut + * addressing, menu 0, item 4, sub 0) and "Sub NoShortcut" (no + * shortcut, sub 1 -- the fixture's only shortcut-less sub-item, + * proving MENUPICK's pointer-based fallback reaches a genuine + * sub-item, not just a top-level one). */ #include @@ -81,17 +89,20 @@ struct Library *GadToolsBase; #define MENUNUM_PROJECT 0 #define ITEMNUM_ABOUT 0 +#define ITEMNUM_TOGGLE 1 #define ITEMNUM_MORE 4 #define SUBNUM_SUBITEM 0 +#define SUBNUM_SUBNOSHORTCUT 1 static struct NewMenu g_newMenu[] = { { NM_TITLE, (STRPTR)"Project", NULL, 0, 0, NULL }, { NM_ITEM, (STRPTR)"About", (STRPTR)"A", 0, 0, NULL }, - { NM_ITEM, (STRPTR)"Toggle", (STRPTR)"T", CHECKIT | MENUTOGGLE | CHECKED, 0, NULL }, + { NM_ITEM, (STRPTR)"Toggle", NULL, CHECKIT | MENUTOGGLE | CHECKED, 0, NULL }, { NM_ITEM, (STRPTR)"Disabled", NULL, NM_ITEMDISABLED, 0, NULL }, { NM_ITEM, NM_BARLABEL, NULL, 0, 0, NULL }, { NM_ITEM, (STRPTR)"More", NULL, 0, 0, NULL }, { NM_SUB, (STRPTR)"Sub Item", (STRPTR)"S", 0, 0, NULL }, + { NM_SUB, (STRPTR)"Sub NoShortcut", NULL, 0, 0, NULL }, { NM_END, NULL, NULL, 0, 0, NULL }, }; @@ -397,11 +408,31 @@ int main(void) && SUBNUM(msg->Code) == NOSUB) { GT_SetGadgetAttrs(hostGad, window, NULL, GTST_String, (ULONG)"about picked", TAG_DONE); + } else if (MENUNUM(msg->Code) == MENUNUM_PROJECT + && ITEMNUM(msg->Code) == ITEMNUM_TOGGLE + && SUBNUM(msg->Code) == NOSUB) { + /* Enabled, shortcut-less -- the fixture's only + * candidate for proving MENUPICK's pointer-based + * fallback (issue #63) genuinely reaches Intuition's + * own IDCMP_MENUPICK, not just that a shortcut + * keystroke was struck. */ + GT_SetGadgetAttrs(hostGad, window, NULL, + GTST_String, (ULONG)"toggle picked", TAG_DONE); } else if (MENUNUM(msg->Code) == MENUNUM_PROJECT && ITEMNUM(msg->Code) == ITEMNUM_MORE && SUBNUM(msg->Code) == SUBNUM_SUBITEM) { GT_SetGadgetAttrs(hostGad, window, NULL, GTST_String, (ULONG)"subitem picked", TAG_DONE); + } else if (MENUNUM(msg->Code) == MENUNUM_PROJECT + && ITEMNUM(msg->Code) == ITEMNUM_MORE + && SUBNUM(msg->Code) == SUBNUM_SUBNOSHORTCUT) { + /* Enabled, shortcut-less, one level deep -- the + * fixture's only candidate for proving + * MENUPICK's pointer-based fallback (issue #63) + * reaches a genuine sub-item, not just a + * top-level one (Toggle, above). */ + GT_SetGadgetAttrs(hostGad, window, NULL, + GTST_String, (ULONG)"subitem noshortcut picked", TAG_DONE); } break; default: diff --git a/server/README.md b/server/README.md index f6465d5..0c2ac64 100644 --- a/server/README.md +++ b/server/README.md @@ -537,26 +537,58 @@ Lands in phase 0.2 onward -- see reports (and what Intuition itself decodes an `IDCMP_MENUPICK` `Code` into via `MENUNUM()`/`ITEMNUM()`/`SUBNUM()`). - **Selection is keyboard-shortcut only for now.** `AmipMenuPickByShortcut()` - (`server/src/action.c`) activates the window, then strikes the - item's `Command` byte (inverted through the live keymap via - `MapANSI()`, same technique `AmipTypeString()` uses per character) - with the right-Amiga qualifier held -- the same input.device path a - human pressing Right-Amiga+key produces. Intuition resolves that - combination against the window's own live menu strip on its own; - this deliberately does not synthesize `IDCMP_MENUPICK` directly, so - a successful `RC 0` is genuine evidence the pick reached the app - through the real menu-shortcut path, not a shortcut around it. - **Honest limit:** an item with no keyboard shortcut (`COMMSEQ` - unset) can't be picked yet -- `RC 20` names this explicitly - ("pointer-based menu selection isn't built yet") rather than - silently failing or guessing a fallback. Pointer-based navigation - (open the menu, move across items/submenus, release over the target - -- `LayoutMenusA()` already precomputes every item's screen-absolute - geometry before the menu is ever opened, so this is buildable) is - real follow-up work, not invented here ahead of it. A disabled item - (`ITEMENABLED` unset) is rejected client-side, before any keystroke - is sent at all -- `RC 20`, distinct from the no-shortcut case. + **Selection has two paths, chosen automatically per item.** When + the target has a real keyboard shortcut (`COMMSEQ` set), + `AmipMenuPickByShortcut()` (`server/src/action.c`) activates the + window, then strikes the item's `Command` byte (inverted through the + live keymap via `MapANSI()`, same technique `AmipTypeString()` uses + per character) with the right-Amiga qualifier held -- the same + input.device path a human pressing Right-Amiga+key produces. + Intuition resolves that combination against the window's own live + menu strip on its own; this deliberately does not synthesize + `IDCMP_MENUPICK` directly, so a successful `RC 0` is genuine evidence + the pick reached the app through the real menu-shortcut path, not a + shortcut around it. + + **For an item with no shortcut** (issue #63), `AmipMenuPickByPointer()` + drives the exact sequence a human would: a genuine synthesized + RMB-down, then a move onto the target menu's own title text in the + screen's title bar (this is the part that isn't obvious from the + autodocs -- RMB-down alone only switches the title bar into menu + mode, showing menu titles instead of the window title; the pulldown + itself doesn't open until the pointer is actually moved over the + title, confirmed live), then a move onto the target item (which + highlights it and, for a sub-item, auto-opens the one-level submenu + purely as an Intuition-internal reaction to pointer position -- no + separate event exists to synthesize for either), then a final move + into the submenu if picking a sub-item, then RMB-up over the target + -- exactly what Intuition turns into `IDCMP_MENUPICK` (only the + most-subordinate item under the pointer is selectable; a top-level + item with a submenu can't itself be picked). Every box is resolved + live off the actual `struct Menu`/`MenuItem` fields immediately + before each move (`ResolveMenuItemBox()`), never cached or computed + from assumed pixel metrics -- menu layout depends on the user's own + screen/menu font. Confirmed live (2026-08-09) against real geometry: + `Menu->LeftEdge` is genuinely screen-absolute; a top-level item's own + box is `menu->LeftEdge + item->LeftEdge`, + `screen->TopEdge + screen->BarHeight + 1 + item->TopEdge` (the + screen's own live, font-derived `BarHeight` -- "Bar sizes for this + Screen... BarHeight is one less than the actual menu bar height", + `intuition/screens.h`); a sub-item's box left-anchors flush against + its parent item's own right edge (`parentLeft + parentWidth` -- + `item->LeftEdge` plays no part in a sub-item's X placement and must + NOT be added on top of that, confirmed the hard way: doing so + overshot the submenu's own box entirely), with `item->TopEdge` + genuinely its own per-item stacking offset from the submenu box's + top. The RKRM documents none of the sub-item placement -- this was + measured pixel-for-pixel against a real screenshot captured mid-pick + under Copperline (`amipilot.screenshot`'s own PNG conversion), not + guessed. **Honest limit:** a window with `WFLG_RMBTRAP` set opts + entirely out of Intuition's own menu-button handling -- no + synthesized RMB-down can ever open its menu strip, reported as its + own distinct `RC 20` reason rather than a confusing timeout. A + disabled item (`ITEMENABLED` unset) is rejected client-side, before + any input is sent at all, on either path. Verified end-to-end by `make test-target`'s MENU/MENUPICK check (`tests/copperline/menu-test.py`) against a menu strip added to @@ -564,11 +596,12 @@ Lands in phase 0.2 onward -- see every field the walker read live (including a `CHECKIT|MENUTOGGLE` item's starting `checked` state and a separator bar's blank, disabled entry), `MENUPICK`s a top-level item and a submenu item by - their shortcuts and confirms each pick genuinely reached the - fixture's own `IDCMP_MENUPICK` handler (which writes a distinct - marker into its Host string gadget, read back via the + their shortcuts, a shortcut-less top-level item and a shortcut-less + sub-item by the pointer path, and confirms each pick genuinely + reached the fixture's own `IDCMP_MENUPICK` handler (which writes a + distinct marker into its Host string gadget, read back via the already-verified `GETTEXT` path -- proof of real delivery through - Intuition, not just that a keystroke was injected), and confirms the + Intuition, not just that input was injected), and confirms the fixture's permanently-disabled item is rejected without ever sending a keystroke. diff --git a/server/include/action_engine.h b/server/include/action_engine.h index 6d0b0df..facceda 100644 --- a/server/include/action_engine.h +++ b/server/include/action_engine.h @@ -259,16 +259,41 @@ BOOL AmipIsWindowOpen(struct Window *target); * if window has no menu strip or any index is out of range. */ struct MenuItem *AmipFindMenuItem(struct Window *window, LONG menuNum, LONG itemNum, LONG subNum); +/* Same resolution as AmipFindMenuItem, but also hands back the + * resolved top-level struct Menu* (menuOut) and, when subNum >= 0, + * the top-level struct MenuItem* the sub-item hangs off of + * (parentItemOut) -- both optional (pass NULL to ignore). Needed for + * pointer-based picking (issue #63): a sub-item's on-screen box has no + * meaning without knowing which top-level menu/item it belongs to, + * which the plain GA_ID-shaped AmipFindMenuItem() doesn't expose. + * AmipFindMenuItem() itself is a thin wrapper over this with both outs + * NULL. */ +struct MenuItem *AmipFindMenuItemWithParents(struct Window *window, + LONG menuNum, LONG itemNum, LONG subNum, + struct Menu **menuOut, + struct MenuItem **parentItemOut); + typedef enum { AMIP_MENUPICK_OK = 0, AMIP_MENUPICK_DISABLED, /* item->Flags lacks ITEMENABLED */ - AMIP_MENUPICK_NO_SHORTCUT, /* no COMMSEQ/Command, or Command isn't a - * single keystroke under the active - * keymap -- pointer-based menu - * navigation isn't built yet (see - * server/README.md) */ - AMIP_MENUPICK_INJECT_FAILED /* keymap.library unavailable, or + AMIP_MENUPICK_NO_SHORTCUT, /* AmipMenuPickByShortcut only: no COMMSEQ/ + * Command, or Command isn't a single + * keystroke under the active keymap -- + * the caller should fall back to + * AmipMenuPickByPointer() instead of + * treating this as a final failure */ + AMIP_MENUPICK_INJECT_FAILED, /* keymap.library unavailable, or * input.device event injection failed */ + AMIP_MENUPICK_RMB_TRAPPED, /* AmipMenuPickByPointer only: window has + * WFLG_RMBTRAP set -- it opts out of + * Intuition's own menu-button handling + * entirely, so no synthesized RMB-down + * can ever open its menu strip. A real, + * permanent limit for that window, not + * a transient injection failure. */ + AMIP_MENUPICK_GEOMETRY_FAILED /* AmipMenuPickByPointer only: couldn't + * resolve a sane screen-absolute box + * for the target item (or its parent) */ } AmipMenuPickResult; /* Selects `item` via its keyboard shortcut: activates window, then @@ -277,9 +302,24 @@ typedef enum { * produce -- Intuition itself resolves that combination against the * window's live MenuStrip, so this doesn't need to (and doesn't) * synthesize IDCMP_MENUPICK directly. Does NOT open the menu or move - * the pointer -- pointer-based selection (for items without a - * shortcut) is planned but not built (docs/implementation-plan.md, - * "menu-pick"). */ + * the pointer -- for an item with no shortcut, use + * AmipMenuPickByPointer() instead (issue #63). */ AmipMenuPickResult AmipMenuPickByShortcut(struct Window *window, struct MenuItem *item); +/* Pointer-based fallback for items with no keyboard shortcut (issue + * #63): opens the menu strip with a genuine synthesized RMB-down, + * moves the pointer onto topItem (letting Intuition's own tracking + * highlight it and, if it has sub-items, auto-open the one-level + * submenu -- no separate "open submenu" event exists to synthesize), + * moves onto subItem if non-NULL, then releases (RMB-up) over the + * final target -- exactly what Intuition turns into IDCMP_MENUPICK. + * `menu` is topItem's own parent struct Menu* (from + * AmipFindMenuItemWithParents()); geometry for every box is resolved + * live, immediately before each move, never cached or precomputed -- + * menu layout depends on the user's own screen/menu font. Does NOT + * check whether topItem/subItem actually has a shortcut -- callers + * decide which pick function to use. */ +AmipMenuPickResult AmipMenuPickByPointer(struct Window *window, struct Menu *menu, + struct MenuItem *topItem, struct MenuItem *subItem); + #endif /* AMIPILOT_ACTION_ENGINE_H */ diff --git a/server/src/action.c b/server/src/action.c index 3c9b213..d4a89b7 100644 --- a/server/src/action.c +++ b/server/src/action.c @@ -332,12 +332,18 @@ BOOL AmipTypeString(CONST_STRPTR text) return TRUE; } -struct MenuItem *AmipFindMenuItem(struct Window *window, LONG menuNum, LONG itemNum, LONG subNum) +struct MenuItem *AmipFindMenuItemWithParents(struct Window *window, + LONG menuNum, LONG itemNum, LONG subNum, + struct Menu **menuOut, + struct MenuItem **parentItemOut) { struct Menu *menu; struct MenuItem *item; LONG i; + if (menuOut != NULL) *menuOut = NULL; + if (parentItemOut != NULL) *parentItemOut = NULL; + if (window == NULL || menuNum < 0 || itemNum < 0) { return NULL; } @@ -349,6 +355,7 @@ struct MenuItem *AmipFindMenuItem(struct Window *window, LONG menuNum, LONG item if (menu == NULL) { return NULL; } + if (menuOut != NULL) *menuOut = menu; item = menu->FirstItem; for (i = 0; i < itemNum && item != NULL; i++) { @@ -357,6 +364,7 @@ struct MenuItem *AmipFindMenuItem(struct Window *window, LONG menuNum, LONG item if (item == NULL || subNum < 0) { return item; } + if (parentItemOut != NULL) *parentItemOut = item; item = item->SubItem; for (i = 0; i < subNum && item != NULL; i++) { @@ -365,6 +373,11 @@ struct MenuItem *AmipFindMenuItem(struct Window *window, LONG menuNum, LONG item return item; } +struct MenuItem *AmipFindMenuItem(struct Window *window, LONG menuNum, LONG itemNum, LONG subNum) +{ + return AmipFindMenuItemWithParents(window, menuNum, itemNum, subNum, NULL, NULL); +} + AmipMenuPickResult AmipMenuPickByShortcut(struct Window *window, struct MenuItem *item) { UBYTE ch; @@ -548,6 +561,176 @@ BOOL AmipClickWindowRelative(struct Window *window, WORD x, WORD y, WORD w, WORD AMIP_BUTTON_LEFT); } +/* Resolves a menu item's current screen-absolute box, read directly off + * the live struct Menu/MenuItem fields -- never cached, since menu + * layout depends on the user's own screen/menu font (issue #63). + * + * Top-level item (parentItem == NULL): Menu->LeftEdge is documented + * screen-absolute (the screen's own LeftEdge plus its left border, + * already folded in -- RKRM Menu Data Structures). MenuItem->LeftEdge/ + * Width/Height for a top-level item are relative to Menu->LeftEdge. + * MenuItem->TopEdge is documented as relative to "the topmost position + * Intuition allows", resolved here against the screen's own live, + * font-derived BarHeight field (intuition/screens.h: "Bar sizes for + * this Screen... BarHeight is one less than the actual menu bar + * height" -- genuinely adapts to the user's screen font, unlike any + * fixed pixel constant). + * + * Sub-item (parentItem != NULL, the resolved TOP-LEVEL item it hangs + * off of): RKRM gives no formula for this, only that the sub-item box + * "overlaps the parent item's own select box somewhere" -- Intuition + * places it itself to avoid clipping off-screen. STARTING HYPOTHESIS, + * NOT YET CONFIRMED ON TARGET: anchor at the parent's own resolved + * absolute box (its right edge, its own top), then add the sub-item's + * own LeftEdge/TopEdge as an offset from that anchor -- the same "read + * the live fields relative to a known reference point" shape as the + * top-level case. Verify live under Copperline against a real submenu + * item (tests/copperline/menu-test.py's MENUPICK-SUBITEM-POINTER + * check) before trusting this -- this project's own house convention + * is real functions verified against a real fixture, not guessed + * heuristics (CLAUDE.md). Update this comment with whatever's actually + * confirmed. */ +static BOOL ResolveMenuItemBox(struct Window *window, struct Menu *menu, + struct MenuItem *parentItem, struct MenuItem *item, + WORD *leftOut, WORD *topOut, WORD *widthOut, WORD *heightOut) +{ + struct Screen *screen; + + if (window == NULL || menu == NULL || item == NULL) { + return FALSE; + } + screen = window->WScreen; + if (screen == NULL) { + return FALSE; + } + + if (parentItem == NULL) { + *leftOut = (WORD)(menu->LeftEdge + item->LeftEdge); + *topOut = (WORD)(screen->TopEdge + screen->BarHeight + 1 + item->TopEdge); + *widthOut = item->Width; + *heightOut = item->Height; + } else { + WORD parentLeft, parentTop, parentWidth, parentHeight; + + if (!ResolveMenuItemBox(window, menu, NULL, parentItem, + &parentLeft, &parentTop, &parentWidth, &parentHeight)) { + return FALSE; + } + /* Confirmed live (2026-08-09), via a real screenshot captured + * mid-pick and measured pixel-for-pixel against + * fixtures/gadtools-app's real "More" submenu: the submenu's + * own box left-anchors flush against the parent item's right + * edge (parentLeft + parentWidth) -- item->LeftEdge plays NO + * part in X placement for a sub-item and must NOT be added on + * top of that (doing so overshot the box entirely, landing + * clicks well past its right edge). item->TopEdge, by + * contrast, genuinely is each sub-item's own stacking offset + * from the submenu box's top (== the parent item's own + * resolved top) -- confirmed matching the visually measured + * row for a second-position sub-item. */ + *leftOut = (WORD)(parentLeft + parentWidth); + *topOut = (WORD)(parentTop + item->TopEdge); + *widthOut = item->Width; + *heightOut = item->Height; + } + + return (*widthOut > 0 && *heightOut > 0) ? TRUE : FALSE; +} + +/* ~500ms: time for the menu strip (or one-level submenu) to actually + * render and for Intuition's own input-handler task to catch up with + * the synthesized pointer position, before the next move/release + * depends on it -- confirmed live (2026-08-09) sufficient under + * Copperline for both the top-level pulldown and a one-level submenu + * to open reliably; same "empirically tuned, not guessed" precedent + * as AmipDragAt's own Delay(3) below. */ +#define AMIP_MENU_OPEN_TICKS 25 +#define AMIP_MENU_SUBOPEN_TICKS 25 + +AmipMenuPickResult AmipMenuPickByPointer(struct Window *window, struct Menu *menu, + struct MenuItem *topItem, struct MenuItem *subItem) +{ + struct MenuItem *target = (subItem != NULL) ? subItem : topItem; + WORD left, top, width, height, x, y; + + if (window == NULL || menu == NULL || topItem == NULL || target == NULL) { + return AMIP_MENUPICK_GEOMETRY_FAILED; + } + if (!(target->Flags & ITEMENABLED)) { + return AMIP_MENUPICK_DISABLED; + } + /* A window that traps the right mouse button opts entirely out of + * Intuition's own menu-button handling -- no synthesized RMB-down + * can ever open its menu strip, a real permanent limit for this + * window, not a transient injection failure. */ + if (window->Flags & WFLG_RMBTRAP) { + return AMIP_MENUPICK_RMB_TRAPPED; + } + + /* Same "bring the target forward first" rationale as + * AmipClickGadget -- input only reaches whatever's frontmost. */ + BringWindowForward(window); + + if (!SendRawMouseButton(IEQUALIFIER_RBUTTON, IECODE_RBUTTON)) { + return AMIP_MENUPICK_INJECT_FAILED; + } + /* RMB-down alone only switches the screen's own title bar into + * menu mode (showing menu titles instead of the window title) -- + * confirmed live (2026-08-09) that the pulldown itself does NOT + * open until the pointer is actually moved over the target menu's + * own title text in that bar. Move there first: Menu->LeftEdge is + * documented screen-absolute (see ResolveMenuItemBox's own doc + * comment); the title row is the screen's own live BarHeight, not + * a guessed pixel constant. */ + if (!AmipMoveMouseTo(window->WScreen, + (WORD)(menu->LeftEdge + 8), + (WORD)(window->WScreen->TopEdge + window->WScreen->BarHeight / 2))) { + SendRawMouseButton(0, (UWORD)(IECODE_RBUTTON | IECODE_UP_PREFIX)); + return AMIP_MENUPICK_INJECT_FAILED; + } + Delay(AMIP_MENU_OPEN_TICKS); + + if (!ResolveMenuItemBox(window, menu, NULL, topItem, &left, &top, &width, &height)) { + SendRawMouseButton(0, (UWORD)(IECODE_RBUTTON | IECODE_UP_PREFIX)); + return AMIP_MENUPICK_GEOMETRY_FAILED; + } + x = (WORD)(left + width / 2); + y = (WORD)(top + height / 2); + if (!AmipMoveMouseTo(window->WScreen, x, y)) { + SendRawMouseButton(0, (UWORD)(IECODE_RBUTTON | IECODE_UP_PREFIX)); + return AMIP_MENUPICK_INJECT_FAILED; + } + /* Highlights topItem and, if it has sub-items, auto-opens the + * one-level submenu -- purely Intuition-internal reactions to + * pointer position while RMB is held; no separate event exists to + * synthesize for either. */ + Delay(AMIP_MENU_OPEN_TICKS); + + if (subItem != NULL) { + if (!ResolveMenuItemBox(window, menu, topItem, subItem, &left, &top, &width, &height)) { + SendRawMouseButton(0, (UWORD)(IECODE_RBUTTON | IECODE_UP_PREFIX)); + return AMIP_MENUPICK_GEOMETRY_FAILED; + } + x = (WORD)(left + width / 2); + y = (WORD)(top + height / 2); + if (!AmipMoveMouseTo(window->WScreen, x, y)) { + SendRawMouseButton(0, (UWORD)(IECODE_RBUTTON | IECODE_UP_PREFIX)); + return AMIP_MENUPICK_INJECT_FAILED; + } + Delay(AMIP_MENU_SUBOPEN_TICKS); + } + + /* Release over the target -- this is literally what Intuition + * turns into IDCMP_MENUPICK (only the most-subordinate item under + * the pointer is selectable, per RKRM). Best-effort even on + * failure paths above already released; this final one is the + * real commit. */ + if (!SendRawMouseButton(0, (UWORD)(IECODE_RBUTTON | IECODE_UP_PREFIX))) { + return AMIP_MENUPICK_INJECT_FAILED; + } + return AMIP_MENUPICK_OK; +} + BOOL AmipDragAt(struct Screen *screen, WORD x1, WORD y1, WORD x2, WORD y2) { if (!AmipMoveMouseTo(screen, x1, y1)) { diff --git a/server/src/amipilotserver/main.c b/server/src/amipilotserver/main.c index e0b8ad8..7acbf7c 100644 --- a/server/src/amipilotserver/main.c +++ b/server/src/amipilotserver/main.c @@ -1273,6 +1273,8 @@ static int HandleCommand(AmipArexxParsed *cmd, const char **resultOut, case AMIP_AREXX_CMD_MENUPICK: { struct Window *w = AmipFindWindow((CONST_STRPTR)cmd->screenPattern, (CONST_STRPTR)cmd->windowPattern); + struct Menu *menu = NULL; + struct MenuItem *topItem = NULL; struct MenuItem *item; AmipMenuPickResult pickRc; @@ -1280,12 +1282,23 @@ static int HandleCommand(AmipArexxParsed *cmd, const char **resultOut, rc = AMIP_AREXX_RC_WARN; break; } - item = AmipFindMenuItem(w, cmd->menuNum, cmd->itemNum, cmd->subNum); + item = AmipFindMenuItemWithParents(w, cmd->menuNum, cmd->itemNum, cmd->subNum, + &menu, &topItem); if (item == NULL || !AmipIsWindowOpen(w)) { rc = AMIP_AREXX_RC_WARN; break; } - pickRc = AmipMenuPickByShortcut(w, item); + /* A real shortcut, when the item has one, is the fast/ + * proven path (AmipMenuPickByShortcut, unchanged) -- + * pointer-based picking (issue #63) is purely the fallback + * for items that don't. */ + if ((item->Flags & COMMSEQ) && item->Command != 0) { + pickRc = AmipMenuPickByShortcut(w, item); + } else { + pickRc = AmipMenuPickByPointer(w, menu, + (cmd->subNum >= 0) ? topItem : item, + (cmd->subNum >= 0) ? item : NULL); + } switch (pickRc) { case AMIP_MENUPICK_OK: break; @@ -1294,14 +1307,24 @@ static int HandleCommand(AmipArexxParsed *cmd, const char **resultOut, strncpy(g_resultBuf, "menu item is disabled", sizeof(g_resultBuf) - 1); result = g_resultBuf; break; - case AMIP_MENUPICK_NO_SHORTCUT: + case AMIP_MENUPICK_RMB_TRAPPED: rc = AMIP_AREXX_RC_FAIL; strncpy(g_resultBuf, - "item has no keyboard shortcut -- pointer-based menu " - "selection isn't built yet (see server/README.md)", + "window traps the right mouse button (WFLG_RMBTRAP) -- " + "pointer-based menu selection isn't possible for this window", + sizeof(g_resultBuf) - 1); + result = g_resultBuf; + break; + case AMIP_MENUPICK_GEOMETRY_FAILED: + rc = AMIP_AREXX_RC_FAIL; + strncpy(g_resultBuf, "couldn't resolve menu item geometry", sizeof(g_resultBuf) - 1); result = g_resultBuf; break; + case AMIP_MENUPICK_NO_SHORTCUT: /* AmipMenuPickByShortcut only; + * unreachable here given the + * branch above, kept for + * switch completeness */ case AMIP_MENUPICK_INJECT_FAILED: default: rc = AMIP_AREXX_RC_FAIL; diff --git a/tests/copperline/README.md b/tests/copperline/README.md index 95e2673..e7ed41f 100644 --- a/tests/copperline/README.md +++ b/tests/copperline/README.md @@ -267,6 +267,45 @@ comment on `HandleWhereMessage()` in but has never been observed to actually reject a real ARexx script's command, so left unchanged here — outside this issue's scope). +### `MENUPICK` pointer-based fallback (issue #63) + +`run_menu_check`'s `MENUPICK-TOGGLE-POINTER`/`MENUPICK-SUBITEM-POINTER` +assertions drive `fixtures/gadtools-app`'s two new shortcut-less menu +items — "Toggle" (its `NM_CommKey` was originally, incorrectly, set to +`"T"`; the fixture's whole point as this feature's test target only +works with it genuinely shortcut-less) and a new "Sub NoShortcut" +sub-item under "More" — through `AmipMenuPickByPointer()` +(`server/src/action.c`), a genuine synthesized RMB-down/move/move/ +release sequence, not the existing `AmipMenuPickByShortcut()` path. + +**Two real, non-obvious findings from getting this working live** +(2026-08-09), neither derivable from the NDK/RKRM alone: + +1. **RMB-down alone doesn't open the pulldown.** It only switches the + screen's own title bar into menu mode (menu titles instead of the + window title) — confirmed by a screenshot captured mid-pick + (`amipilot.screenshot`'s PNG conversion) showing zero visible + change even after a full second's `Delay()`, and independently by + the target window never receiving even a fallback + `IDCMP_MOUSEBUTTONS` for the synthesized press (a live diagnostic + added to the fixture specifically to rule this out). The pulldown + only opens once the pointer is moved onto the target menu's own + title text in the screen's bar — `AmipMenuPickByPointer()` now does + this as an explicit first move, before ever touching the item + itself. +2. **The RKRM documents no formula for a submenu's own screen + position** — only that it "overlaps its parent item's own select + box somewhere." The real placement was measured pixel-for-pixel: a + screenshot captured with the submenu genuinely open (RMB still + held, pointer hovering the parent item) was cropped and scanned for + the popup's actual border pixels, giving `parentLeft + parentWidth` + for the box's left edge (the sub-item's own `LeftEdge` plays no + part in X placement at all — an initial guess that added it on top + overshot the box entirely, landing clicks well past its right edge) + and `parentTop + item->TopEdge` for its top. See + `ResolveMenuItemBox()`'s own doc comment in `server/src/action.c` + for the confirmed formula. + ## Ad hoc smoke testing (debugging, new fixtures) For anything `run.sh` doesn't already assert on, write diff --git a/tests/copperline/menu-test.py b/tests/copperline/menu-test.py index 58d86c7..d402274 100644 --- a/tests/copperline/menu-test.py +++ b/tests/copperline/menu-test.py @@ -1,18 +1,22 @@ #!/usr/bin/env python3 -"""Drives AmiPilotServer's MENU/MENUPICK verbs (phase 0.4) end to end -for the on-target regression check (tests/copperline/run.sh), against +"""Drives AmiPilotServer's MENU/MENUPICK verbs (phase 0.4, plus the +pointer-based fallback from issue #63) end to end for the on-target +regression check (tests/copperline/run.sh), against fixtures/gadtools-app's menu strip (see its own header comment for the exact layout: Project > About(A) / Toggle(T, checkit) / Disabled / -separator / More > Sub Item(S)). +separator / More > Sub Item(S) / Sub NoShortcut). Walks the menu via MENU and asserts the structure the walker read off Intuition's live struct Menu/MenuItem chain, then MENUPICKs "About" -and "Sub Item" by their keyboard shortcuts and confirms each pick -genuinely reached the app (not just that a keystroke was injected) by -reading back the marker text GTApp's own IDCMP_MENUPICK handler writes -into its Host string gadget -- the same GETTEXT path every other check -already trusts. Also confirms the permanently-disabled item is -rejected client-side (no keystroke sent at all). +and "Sub Item" by their keyboard shortcuts, and "Toggle"/"Sub +NoShortcut" via the pointer-based fallback (a genuine synthesized +RMB-down/move/RMB-up, not a shortcut keystroke -- server/src/action.c's +AmipMenuPickByPointer()), confirming each pick genuinely reached the +app (not just that input was injected) by reading back the marker text +GTApp's own IDCMP_MENUPICK handler writes into its Host string gadget +-- the same GETTEXT path every other check already trusts. Also +confirms the permanently-disabled item is rejected client-side (no +input sent at all). Prints one greppable line per stage (run.sh asserts on them) and exits non-zero on any failure. @@ -73,6 +77,31 @@ def main() -> int: print(f"MENUPICK-SUBITEM FAIL RESULT={result}") return 1 + # "Toggle" is enabled but has no keyboard shortcut -- MENUPICK + # against it exercises the pointer-based fallback (issue #63: + # genuine RMB-down/move/RMB-up, not a shortcut keystroke). + client.menu_pick("GadTools", toggle.menu_num, toggle.item_num) + result = client.get_text("GadTools", 2) + if result == "toggle picked": + print(f"MENUPICK-TOGGLE-POINTER PASS RESULT={result}") + else: + print(f"MENUPICK-TOGGLE-POINTER FAIL RESULT={result}") + return 1 + + # "Sub NoShortcut" is enabled, one level deep, and has no shortcut + # -- the pointer-based fallback's genuinely uncertain case (issue + # #63: submenu box geometry is undocumented by the RKRM), unlike + # Toggle above (a top-level item). + sub_noshortcut = strip.find("Sub NoShortcut") + client.menu_pick("GadTools", sub_noshortcut.menu_num, + sub_noshortcut.item_num, sub_noshortcut.sub_num) + result = client.get_text("GadTools", 2) + if result == "subitem noshortcut picked": + print(f"MENUPICK-SUBITEM-POINTER PASS RESULT={result}") + else: + print(f"MENUPICK-SUBITEM-POINTER FAIL RESULT={result}") + return 1 + try: client.menu_pick("GadTools", disabled.menu_num, disabled.item_num) print("MENUPICK-DISABLED FAIL no exception raised") diff --git a/tests/copperline/run.sh b/tests/copperline/run.sh index 9b7234f..cf3e7dc 100755 --- a/tests/copperline/run.sh +++ b/tests/copperline/run.sh @@ -1390,6 +1390,8 @@ EOF 'MENU-FIELDS PASS' \ 'MENUPICK-ABOUT PASS' \ 'MENUPICK-SUBITEM PASS' \ + 'MENUPICK-TOGGLE-POINTER PASS' \ + 'MENUPICK-SUBITEM-POINTER PASS' \ 'MENUPICK-DISABLED PASS'; do if ! grep -qF "$pattern" "$BUILD/menu-result.txt" 2>/dev/null; then echo "run.sh: FAIL (menu): expected line not found: $pattern" diff --git a/userdocs/ARexx-Reference.md b/userdocs/ARexx-Reference.md index 3ab2a9a..1addc08 100644 --- a/userdocs/ARexx-Reference.md +++ b/userdocs/ARexx-Reference.md @@ -53,7 +53,7 @@ for disambiguating two same-titled windows on different screens; see | `FSGET` | `` | Returns a file's full contents (`RESULT` is the raw bytes, capped at the server's own small internal buffer — a test-staging channel, not a file manager). `FSPUT` (the opposite direction) is **not** listed here: it's wire-only, not answerable over ARexx at all — see [File API](Wire-Protocol.md#file-api). | | `SCREENS` | (none) | Lists every open screen: title, position, size, and whether it's frontmost. Title is each screen's own name (`DefaultTitle`), not the live title-bar text a window's `WA_ScreenTitle` can override. | | `MENU` | `` | Returns the matched window's full menu strip — every pulldown menu, its items, and (one level deep) their submenu items, with checkit/checked/enabled state and any keyboard shortcut, in the same text shape `AmiInspect` prints. | -| `MENUPICK` | ` []` | Selects a menu item via its keyboard shortcut (Right-Amiga + the shortcut character) — the numbers are the same 0-based chain positions `MENU`'s own output reports. `RC=20` if the item is disabled or has no keyboard shortcut (pointer-based selection for shortcut-less items isn't built yet). See [Wire Protocol](Wire-Protocol.md#menus) for the full contract. | +| `MENUPICK` | ` []` | Selects a menu item — the numbers are the same 0-based chain positions `MENU`'s own output reports. Items with a keyboard shortcut are picked via Right-Amiga + the shortcut character; items with none are picked via a genuine synthesized RMB-down/move/RMB-up sequence instead, automatically. `RC=20` if the item is disabled, or (pointer path only) if the window traps the right mouse button (`WFLG_RMBTRAP` — no synthesized RMB-down can ever open its menu). See [Wire Protocol](Wire-Protocol.md#menus) for the full contract. | | `DRAG` | ` ` or ` TO ( \| @)` | A genuine press/move/release drag. The offset form (` `) moves the gadget's current center by a pixel delta — the natural shape for a slider/scroller. The `TO` form drags onto a second gadget's center instead, both resolved live, for drag-and-drop/reorder — the destination must be in the same window as the source. `` is the same numeric `GA_ID`, `ROLE=`/`LABEL=`/`INDEX=`, or `@name` form CLICK/TYPE/GETTEXT accept. | | `WINDOWMOVE` | `[SCREEN=] ` | Moves the WHOLE window by a pixel offset — a genuine title-bar drag (`WFLG_DRAGBAR`), built on the same primitive `DRAG`'s gadget forms use. Classic locator form only, no `@name` — a window-level action, same scope as `TREE`/`MENU`. `RC=20` ("window has no drag bar") if the window never had one. No separate "get position" verb — `TREE`'s own `[left,top WxH]` header already carries it. | | `WINDOWSIZE` | `[SCREEN=] ` | Resizes the WHOLE window to an ABSOLUTE target size — a genuine sizing-gadget drag (`WFLG_SIZEGADGET`) from its current bottom-right corner. Doesn't pre-check against the window's own min/max — Intuition clamps the drag as it would a real one; confirm the actual result with a follow-up `TREE`. `RC=20` ("window has no sizing gadget") if the window never had one. Same classic-form-only scope as `WINDOWMOVE`. | diff --git a/userdocs/Changelog.md b/userdocs/Changelog.md index 5cee054..ed9a54f 100644 --- a/userdocs/Changelog.md +++ b/userdocs/Changelog.md @@ -25,6 +25,14 @@ for the full engineering detail and phase sequencing behind each one. own new `CAAPP.WHERE` port, whose three gadgets are now addressed entirely via `WHEREGADGET` (its manifest previously, deliberately, named none at all). See [ARexx Reference](ARexx-Reference.md#driving-layoutgadget-only-applications). +- **`MENUPICK`'s pointer-based fallback** (issue #63): a menu item + with no keyboard shortcut used to be rejected outright — now it's + picked via a genuine synthesized right-mouse-button-down/move/move/ + release sequence instead, chosen automatically whenever an item has + no shortcut, the same "real input.device events, not a shortcut" + principle every other verb already follows. Verified end to end + against two new shortcut-less items on `fixtures/gadtools-app`'s own + menu strip — a top-level item and a one-level-deep submenu item. ## v1.0 — 2026-08-09 diff --git a/userdocs/Wire-Protocol.md b/userdocs/Wire-Protocol.md index d93bfcd..6278b32 100644 --- a/userdocs/Wire-Protocol.md +++ b/userdocs/Wire-Protocol.md @@ -491,18 +491,30 @@ Intuition itself reports via `IDCMP_MENUPICK`'s `MENUNUM()`/ `ITEMNUM()`/`SUBNUM()` macros. `MenuStrip.find("some label")` looks up an item by its text instead of hand-counting positions. -**`menu_pick()` selects by keyboard shortcut only, for now.** It -activates the window, then strikes the item's shortcut character with -the right-Amiga qualifier held — the same input.device path a human -pressing Right-Amiga+key produces. Intuition resolves that -combination against the window's own live menu strip; AmiPilot -doesn't (and can't) synthesize the pick event directly, so `RC 0` is -real evidence the pick reached the app through the genuine -menu-shortcut path. Raises `ActionFailed` (`RC 20`) if the item is -disabled, or if it has no keyboard shortcut at all — pointer-based -navigation (open the menu, move across items, release over the -target) for shortcut-less items is planned but not built yet; see -`server/README.md`. +**`menu_pick()` chooses the pick mechanism automatically, per item.** +For an item with a keyboard shortcut, it activates the window, then +strikes the shortcut character with the right-Amiga qualifier held — +the same input.device path a human pressing Right-Amiga+key produces; +Intuition resolves that combination against the window's own live +menu strip, so `RC 0` is real evidence the pick reached the app +through the genuine menu-shortcut path. For an item with **no** +shortcut, it instead drives a genuine synthesized RMB-down, moves onto +the target menu's own title, moves onto the item (auto-opening its +one-level submenu if it has one, purely as Intuition's own reaction to +pointer position), moves into the submenu if picking a sub-item, then +releases (RMB-up) over the target — exactly what Intuition turns into +a real `IDCMP_MENUPICK`, the same "genuinely synthesized input, not a +shortcut" path `click()`/`drag()` already use. Every box this needs is +resolved live off the window's own current menu structure immediately +before each move, never cached — menu layout depends on the user's own +screen/menu font. Raises `ActionFailed` (`RC 20`) if the item is +disabled, or — pointer path only — if the window traps the right mouse +button (`WFLG_RMBTRAP`, a real Intuition window flag some apps set): +no synthesized RMB-down can ever open that window's menu strip, a +permanent limit for that window rather than a transient failure. See +`server/README.md` for the full mechanism, including the (largely +undocumented-by-Commodore) submenu geometry this was measured against +a real screenshot to confirm. ## Screens