Skip to content

feat(input): map Ctrl+, to Cmd+, (Preferences) under --map-ctrl-to-cmd - #184

Merged
clintcan merged 5 commits into
clintcan:mainfrom
antonmos:claude/ctrl-comma-preferences
Oct 1, 2026
Merged

feat(input): map Ctrl+, to Cmd+, (Preferences) under --map-ctrl-to-cmd#184
clintcan merged 5 commits into
clintcan:mainfrom
antonmos:claude/ctrl-comma-preferences

Conversation

@antonmos

@antonmos antonmos commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Problem

Ctrl+, did nothing under --map-ctrl-to-cmd. Comma wasn't in the curated Ctrl→Cmd shortcut set (is_remappable_shortcut), so Ctrl+, passed through as a literal Ctrl+comma — and macOS binds Preferences to Cmd+,, not Ctrl+comma, so no app reacted.

Change

Add comma (vk 0x2B) to is_remappable_shortcut, so Ctrl+, → Cmd+, (Preferences), consistent with the rest of the remap. Doc comment + unit test updated; CLI/features/quirks docs list comma in the curated set.

Testing

cargo build + cargo test input:: (passed). Live-verified: Ctrl+, opens Preferences over RDP.

Stacked on #183 — merge that first

This branch is rebased on top of #183's branch (claude/mouse-modifier-flags), so the two no longer conflict on the shared doc lines. Its own change is the single commit 2f61a34 (4 files, 8+/7−).

Because #183's branch lives on the fork, GitHub won't accept it as this PR's base (it must be a branch in clintcan/macrdp), so the base stays main. Consequence: until #183 merges, "Files changed" here also shows #183's commits. Once #183 lands, GitHub recomputes and this PR collapses to just the comma commit — no rebase needed. Please merge in order: #183, then #184.

🤖 Generated with Claude Code

clintcan added a commit that referenced this pull request Sep 15, 2026
@clintcan
docs: record the 2026-09-16 upstream and open-PR state
72f57f4 
TODO.md
- In flight: review state of @antonmos's open PRs. #183's blocker is
  resolved in c8712e9 with one follow-up requested (the connection reset
  also fires on every reactivation; the latch survives a reset when no
  modifier is held; a pre-existing phantom drag after a mid-drag disconnect,
  latent on main too). #182 asked for a per-step deadline, with a ship-now
  fallback, and collides with the mic branch on divergence (24). #181 and
  #184 not yet reviewed. Links issue #186.
- Upstreaming watch: a dated update superseding the stale "zero open PRs"
  note. Divergence (23) is upstream via #1476 + #1913 (ConnectionPolicy) with
  its traps; IronRDP#1969 blocks de-vendoring (22)/(23) and upstreaming (18)
  and ships in 0.14.0 unless fixed; #1483 closed; overlapping upstream work
  to evaluate at the next bump (#1951/#1953/#1954 multitransport stack,
  ironrdp-rdpeai #1645 + #1946 for the mic divergence).

vendor/ironrdp-server/CLAUDE.md
- (18): upstream status — #1484 green-lit by a peer, held patch 392 commits
  stale, and blocked on #1969 because upstream Preempt takes the handler for
  the race. Notes the --fork-workers scope boundary no longer applies.
- (12): the #1951 -> #1953 -> #1954 stack as a possible partial de-vendor
  path, and how it differs from this divergence.

Docs only.
@antonmos
antonmos force-pushed the claude/ctrl-comma-preferences branch from 8e2d296 to 2f61a34 Compare September 16, 2026 02:17
@clintcan clintcan mentioned this pull request Sep 17, 2026
Merged
clintcan added a commit that referenced this pull request Sep 17, 2026
@clintcan
docs(todo): #183 round 3 state and the divergence (24)/(25)/(26) numb…
1c34a2b 
…ering

- Records the numbering decision: three branches claim vendored divergence
  (24), so by expected merge order #183 keeps (24), #182 takes (25) and the
  mic divergence becomes (26); otherwise take the next free number at merge.
- #183: round 3 addressed all three follow-ups (verified against 334cc3c,
  CI green). Remaining asks: document and live-test the synthetic button-up,
  which lands at the pre-disconnect cursor position and can complete an
  interrupted drag as a drop; and tests for the connection-edge path.
- #182: no reply yet; must renumber to (25).
- #184: stacked on #183, review after it merges.
- Upstreaming watch: mic divergence (24->26).

Docs only.
@antonmos
antonmos force-pushed the claude/ctrl-comma-preferences branch from 2f61a34 to b11a679 Compare September 30, 2026 02:46
antonmos added a commit to antonmos/macrdp that referenced this pull request Sep 30, 2026
@antonmos @claude
Merge branch 'main' into claude/mouse-modifier-flags
012ab86 
Same divergence-log conflict as the stacked clintcan#184 merge: main added (25)
(MS-RDPEAI microphone redirection) at the append point this branch uses for
(24) (the per-served-connection input-reset handle). Kept BOTH, (24) then (25)
— main's own (25) entry reserves (24) for this PR, so no renumbering is needed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
antonmos and others added 4 commits September 29, 2026 23:55
@antonmos @claude
fix(input): carry held modifiers on mouse events + Ctrl+click→Cmd+cli…
993fc12 
…ck remap

Synthesized mouse click/drag events never called set_flags, so a click while
a modifier was held arrived with empty modifierFlags — a genuine Cmd+click
didn't open a link in a new tab, Shift+click didn't range-select, and
Ctrl+click wasn't a secondary click. button()/post_move() now stamp the
held-modifier state onto every mouse event, mirroring the keyboard path in
key(), which has always set_flags for exactly this reason.

Additionally, under --map-ctrl-to-cmd a plain Ctrl+click is now delivered as
Cmd+click (open link in a new tab) instead of a secondary/context click — the
mouse analogue of the existing keyboard Ctrl→Cmd remap, via a shared
ctrl_to_cmd_flags() bit-swap. Gated identically (Ctrl held, no Cmd/Alt,
non-excluded app); the frontmost-exclusion check sits behind the cheap
Ctrl-held guards so an ordinary unmodified drag never pays for it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@antonmos @claude
fix(input): resync stale held modifiers; latch + scope the Ctrl+click…
e8a7782 
… remap

Addresses review on clintcan#183.

Blocking — stale held modifiers now cost the mouse, so resync them.
Once clicks carry modifier flags, a modifier whose key-up never arrived
(the client lost focus while Ctrl was held) turns every left click into a
secondary click, and the blast radius was the whole process: `Inner` owns
`mods` and is constructed once, nothing reset it, `synchronize()` reconciles
Caps Lock only, and the Synchronize PDU carries lock keys only — so it
survived disconnect AND reconnect and cleared only on a server restart.

`resync_modifiers_if_stale` (top of keyboard()/mouse()) now clears every held
non-lock modifier (Caps Lock preserved — a toggle synchronize() owns), posts one
FlagsChanged with the emptied set, and drops any outstanding remapped_keys /
click latch, on either trigger:
  * a new connection — capture.rs's per-connection ScreenCaptureUpdates::start
    calls input::request_modifier_reset(), the same seam that already resets
    display_suppressed (RdpServerInputHandler has no per-connection hook), so a
    reconnect is a deterministic clean slate;
  * an idle gap >= MACRDP_MODS_RESYNC_IDLE_MS (default 10 s; 0 disables), which
    covers the common case with no disconnect at all. Generous on purpose: a
    false clear costs one keystroke, a missed one costs every click.

Should-fix 1 — latch the verdict at button-down. `mouse_event_flags()` was
re-evaluated for the down and the up, and the down itself refreshes
LAST_FOCUS_BUNDLE off-thread via update_focus_from_click, so Ctrl+clicking INTO
an excluded app could post a Cmd-flagged down and a Ctrl-flagged up. The verdict
is now decided once at left-button-down (`left_click_remapped`, the mouse
analogue of remapped_keys) and reused for the drag and the up.

Should-fix 2 — primary button only. Right/middle clicks carry the real held
modifiers; a Ctrl+right-click no longer becomes Cmd+right-click.

Should-fix 3 — scroll() now carries the held modifiers too (Shift+scroll,
Cmd+scroll), so the "every synthesized mouse event" claim is true. No Ctrl→Cmd
swap on scroll: Ctrl+scroll is macOS's own screen-zoom accessibility gesture.

Minor — the per-motion cost is gone: post_move() reuses the latch and never
calls frontmost_is_excluded(). The gating predicate is split into the pure
`should_remap_click(remap_on, ctrl, cmd, alt, excluded)` and unit-tested as a
full decision table (only one row remaps), plus a test pinning that the resync
preserves Caps Lock. Also fixes a doc comment that had been attached to the
wrong fn by the earlier refactor.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@antonmos @claude
fix(input): reset per-connection input state once per SERVED connection
c9655ac 
Follow-up to review on clintcan#183 (three items).

1. The connection-edge reset fired on every reactivation. It was requested
from ScreenCaptureUpdates::start, but RdpServerDisplay::updates re-runs after
every deactivation-reactivation — so a held modifier was cleared on every live
resize and on blank recovery, with no user action. Moved to a once-per-SERVED-
connection seam in the vendored server (divergence 24): a new
`set_input_reset_handle` flag raised at the top of run_connection AND
serve_negotiated, next to divergence 13's identical-lifecycle
`auto_reconnect_sent = false`. Each calls accept_finalize exactly once with
reactivations looping inside it, so it never fires for a reactivation. Not
on_accept — that runs for preemption candidates while the live session is
still served, so a mere connection attempt could clear the live session's
modifiers. capture.rs no longer touches it; main.rs installs the handle via
input::modifier_reset_handle().

2. The previous connection's latch survived a reconnect: the resync returned
early when clear_non_lock() reported nothing held, before clearing
remapped_keys / left_click_remapped. Restructured so the connection-edge clears
are unconditional.

3. Pre-existing phantom drag: left_down/right_down were only ever written by a
real button event, so a drop between a left-down and its up posted every move
on the next connection as LeftMouseDragged. The connection-edge reset now
RELEASES any button still down — a synthetic Up through button(), so it carries
the same latched flags the Down did and the click bookkeeping stays paired —
before clearing modifiers, rather than merely forgetting the state (macOS
itself still believed the button held).

Triggers now clear different amounts, per review: the connection edge clears
everything; the idle gap clears modifiers ONLY — a click/drag in progress inside
a live connection is legitimate state, and clearing the latch there would undo
the down/up consistency fix. FlagsChanged is posted only when a modifier was
actually held.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@antonmos @claude
feat(input): map Ctrl+, to Cmd+, (Preferences) under --map-ctrl-to-cmd
0b358d1 
Comma wasn't in the curated Ctrl→Cmd shortcut set, so Ctrl+, passed through
as a literal Ctrl+comma — which macOS apps don't bind (Preferences is Cmd+,),
so nothing happened. Add comma (vk 0x2B) to is_remappable_shortcut so
Ctrl+, → Cmd+, opens Preferences, matching the rest of the remap.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@antonmos
antonmos force-pushed the claude/ctrl-comma-preferences branch from 6637927 to 0b358d1 Compare September 30, 2026 04:56
@clintcan

clintcan commented Sep 30, 2026

Copy link
Copy Markdown
Owner

#183 is merged (58db18c), so this one needs a rebase onto current main. It still carries its own rebased copies of #183's three commits, and those now conflict with main in docs/features.md and docs/known-quirks.md (including the interrupted-drag caveat I added to the quirk note in b43a047). Rebasing onto main should drop the duplicates and leave just the Ctrl+, change — I'll review it once that's up.

@antonmos @claude
Merge branch 'main' into claude/ctrl-comma-preferences
6f1cdd6 
clintcan#183 is merged (58db18c), so main now carries this branch's copies of its three
commits. Merging main in collapses the effective diff to just the Ctrl+, change
(4 files, 8+/7-) — the outcome the review asked for — without rewriting history.

Conflicts were the two long summary lines in docs/features.md and
docs/known-quirks.md, where this branch's rebased copies collided with the
merged clintcan#183 text plus the interrupted-drag caveat added in b43a047. Resolved by
taking main's text (caveat retained) and re-applying only the comma additions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@antonmos

antonmos commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Conflicts resolved in 6f1cdd6 — the PR now shows 4 files, +8/−7: just the Ctrl+, change, which I believe is the end state you were after.

One deliberate deviation worth flagging: I did this as a merge of main, not a rebase. Now that #183 is merged, main already contains this branch's copies of those three commits, so merging main in collapses the effective diff to the comma change on its own — no history rewrite needed. I avoided the rebase because it would require a force-push, which is outside what this session is authorized to do unprompted; if you'd specifically prefer a linear branch over the merge commit, say so and @antonmos can green-light the force-push.

The conflicts were the two long summary lines in docs/features.md and docs/known-quirks.md, exactly as you predicted. Resolved by taking main's text — so the interrupted-drag caveat from b43a047 is retained verbatim — and re-applying only the comma additions on top (plus comma for Cmd+, Preferences / …/find-next/preferences). Verified both are present and that nothing from #183 or your caveat was clobbered.

cargo clippy --all-targets -- -D warnings, cargo fmt --check, and the full suite (262 passed) are clean on the merged tree.

🤖 Addressed by Claude Code

clintcan
clintcan approved these changes Oct 1, 2026
View reviewed changes

@clintcan clintcan left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the net change against main is just the comma key added to the remap set, with its test and doc lines, and CI is green. Squash-merging, since the branch also carries duplicate copies of #183's commits (already on main).

@clintcan
clintcan merged commit 8a18f09 into clintcan:main Oct 1, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clintcan clintcan clintcan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@antonmos @clintcan