Add host-backed emoji key assignments#81
Conversation
|
Thanks for the detailed draft. Since #50 is already merged into After that I can do the real implementation review. The main areas I want to verify are:
I would keep this as Draft until the rebase is done and we have a fresh CI run plus manual host-output coverage for the paths above. |
ad09a3f to
8e6b182
Compare
|
Thank you, I rebased #81 onto current main after #50 and force-pushed the cleaned branch. I also tightened Vial unlock cancellation and expanded automated coverage for exact macro reuse, shared-macro preservation, slot exhaustion, exact buffer capacity, picker error retention, flags, and GUI+F20 transport collision. Release build, i18n, Python syntax, and formatting checks pass. PR body was updated too to reflect rebased scope and verification. |
1678293 to
51fb76e
Compare
kissetfall
left a comment
There was a problem hiding this comment.
The branch is now focused and clean, and the pure logic is well covered: exact generated-macro reuse, shared-slot preservation, capacity boundaries, complex Unicode sequences, unlock eligibility, and decoder framing all look sound. Local verification passed 256 tests, Clippy, i18n, scoped rustfmt, diff-check, Python syntax, and the hosted CI is green on all four Rust builds. The end-to-end assignment and host transport still have blockers.
-
Emoji assignment is not an atomic firmware operation. assign_emoji marks the macro buffer dirty and immediately returns Macro(n); apply_picker_results writes that key or encoder before the later macro-buffer write runs. If the buffer write fails or disconnects, the firmware key points to an empty, old, or partially written macro. The current main path then clears macros_dirty on set/get failure, so there is no retry. Implement one serialized transaction that writes and reads back the complete macro buffer first, then writes and reads back the target key/encoder. On any failure keep the picker/error state and either roll back or leave neither side promoted. Cover buffer failure, partial chunk failure, target-write failure, disconnect, and retry.
-
Reassigning one key repeatedly leaks macro slots. emoji_target_keycode records the old assignment but is never used. Because an existing generated slot is never mutated, changing one emoji allocates a new slot every time; after enough changes the picker reports no space while the earlier generated macros are orphaned. Reuse the old slot only when a complete reference scan proves that no other key, encoder, Combo, Tap Dance, Key Override, or other dynamic entry uses it; otherwise allocate a new slot. Add shared and unshared reassignment tests plus cleanup/rollback coverage.
-
GUI+F20 is not proven safe on Linux. The X11 backend grabs and consumes only F13-F20; Super down/up still reaches the desktop environment and can open the launcher or overview. IBus and Fcitx likewise process the F-key event, not the Super modifier event, and a Wayland compositor may reserve the chord before the input method sees it. The current no-static-symbol unit test does not cover this OS behavior. Use a carrier that does not leak a system modifier, or implement backend-specific neutralization, then validate on GNOME and KDE for X11, Wayland+IBus, and Wayland+Fcitx5 with launcher/menu state unchanged.
-
Assignment eligibility checks firmware macro capacity only, not whether a compatible host decoder is active. A Wayland user without the configured input method, or with the previously installed Fcitx5 addon, can create a macro that emits raw Super/F-key events and never produces emoji. IBus is refreshed automatically, but the compiled Fcitx5 addon has no automatic upgrade or protocol-version handshake. Gate assignment on a ready compatible backend or route the user through setup/update before committing the firmware transaction. Include migration from the previous IBus/Fcitx5 installations.
-
The Fcitx5 C++ change is not compiled by the current GitHub Rust build matrix, and the Rust test only searches source text for constant names. Add a CI target that builds the addon against Fcitx5 and behavior tests that feed the same valid, invalid, timeout, and interrupted frames to the Rust, IBus, and Fcitx decoders. I could validate the IBus Python syntax locally, but this environment does not have the Fcitx5 development toolchain.
-
This compound operation should join the corrected shared HID lifecycle from #88 instead of adding another synchronous macro/key sequence. Validate real USB and Bluetooth hardware for default emoji, skin tones, flags, ZWJ sequences, locked Vial, exact buffer boundary, all rollback cases, and host output on Windows, macOS, Linux X11, IBus, and Fcitx5 before approval.
|
Addressed all six points:
|
There was a problem hiding this comment.
Good progress on the original blockers: the firmware write now orders macro-buffer write/readback before target write/readback; the reference scan and generated-slot reuse are substantially better; the Super modifier is gone; and both Linux backend jobs are now real green builds/tests. The new head passes 257 local tests, Clippy, i18n, diff-check, and IBus syntax. Four blockers remain.
-
The emoji worker is a new parallel HID owner, not the shared lifecycle claimed in the update: #88 is not part of this branch, and this PR only adds emoji_assignment_task to the active-owner predicate. That pauses normal device scans/switches while the task runs, but EmojiAssignmentResult still has no device identity/generation, clear_connected_keyboard_state does not cancel/invalidate the task, and poll_emoji_assignment unconditionally restores its handle and commits/restages UI state. A hung worker also blocks scan, switch, and close indefinitely. Rebase onto the corrected shared owner/handoff, make task completion connection-generation safe, and cover disconnect, completion after state clear, stale completion, and a worker that never returns.
-
backend_ready still does not prove that a compatible decoder is active. X11 only checks DISPLAY. IBus checks env/command/current installed bytes but not that the Entropy engine is active. Fcitx checks a marker file written by the installer, while fcitx5 -r failure is explicitly ignored; the marker can therefore remain current while the addon is not loaded. In all three cases the picker can commit a firmware macro that emits raw F20 frames with no consumer. Gate on an actual active-backend handshake/status (or keep assignment disabled until verified), and test stale marker, failed reload, wrong/inactive input-method engine, and backend startup failure.
-
The transactional and lifecycle failure paths requested in the previous review still have no tests. src/ui/emoji_assignment.rs has no test module; the 257 tests cover allocation and decoder logic, not the HID transaction. Extend the test HID backend and verify macro-buffer failure/partial chunk/readback mismatch, target failure, rollback success/failure, disconnect, explicit retry, exactly-once commit, and stale worker completion. These are the paths that decide whether firmware and UI diverge.
-
Runtime failures are presented as the wrong error and are not localized. Any emoji_assignment_error renders key_picker.emoji_no_free_macro_slot, so a HID disconnect, rollback failure, unavailable target, or backend problem tells the user that no macro slot is free. The setup prompt and all new saving/saved/failure status strings are hardcoded English. Preserve a typed error reason and add EN/RU catalog entries for the actual states.
Please also attach real host-output results for Windows, macOS, Linux X11, IBus, and Fcitx5 (including flags/ZWJ/skin tone and no raw F20 leakage) after the lifecycle and readiness fixes.
80ed316 to
046da97
Compare
- move refresh HID reads to a device-scoped background task\n- distinguish failed and skipped QMK setting reads\n- suppress refresh tooltips during settings scrolling
Preserve Vial unlock cancellation for eligible Symbols assignments and add explicit coverage for flags, picker error retention, and exact macro-buffer capacity.
kissetfall
left a comment
There was a problem hiding this comment.
Head 046da97 makes meaningful progress on the last review: assignment errors are now typed/localized, transaction failures preserve explicit retry state, readback/rollback paths have real test-HID coverage, and stale results are generation-checked. Local verification passed 282/282 tests, Clippy, i18n, diff-check, and Python syntax. Four blockers remain.
-
Timeout/state-clear does not terminate the old firmware writer.
poll_emoji_assignment()andclear_connected_keyboard_state()dropemoji_assignment_task, sohid_write_task_active()immediately becomes false and device scan/reconnect may open the same keyboard while the detached thread still owns the previous HID handle and can continue writing. The generation check only prevents a stale result from mutating UI/restoring its handle; it does not prevent concurrent firmware I/O. Please make timeout/cancellation terminal in the owning HID abstraction, or retain a poisoned/busy owner until the old operation actually exits. Add a regression where the old worker completes after timeout while a reconnect is attempted and prove no overlapping requests occur. -
Backend readiness is still not an active decoder guarantee. On Linux
emoji_assignment_backend_ready()now always returns false, so the feature advertised by this PR cannot be assigned at all on X11, IBus, or Fcitx5. On Windows it remains unconditionally true even thoughSetWindowsHookExWcan fail and the hook thread only logs the failure. Please expose a real runtime-ready status/handshake (or explicitly narrow the supported scope) and cover backend startup/loss before committing firmware macros. -
The new "disconnect" transaction tests do not model a persistent disconnect or rollback failure. The test backend fails exactly one request and then clears
disconnect_after_requests, allowing all rollback requests to succeed. That does not prove behavior when the transport stays unavailable after a partial macro or target write. Add a persistent-failure mode, rollback-failure/reconnect retry coverage, and a successful exactly-once transaction assertion. -
This head is still stacked on the unmerged
#88lifecycle and now conflicts with currentmainafter#89merged. Please rebase after the corrected shared lifecycle lands and integrate Emoji with the merged Settings owner/handoff rather than carrying the parallel old base. The new head also has no hosted checks, and scoped rustfmt currently fails on its changed Rust files.
No new Entropy design-language violation was found in the changed picker UI. This review remains blocked on HID ownership safety, usable backend readiness, real terminal-failure coverage, and a clean current base.
046da97 to
0bfb1d3
Compare
kissetfall
left a comment
There was a problem hiding this comment.
Re-reviewed 0bfb1d3. The rebase onto #89, the Settings-worker ownership integration, and the expanded hosted CI are good improvements. The branch is now merge-clean and all 6 hosted checks are green. The remaining blockers are:
-
The emoji HID worker can still outlive its ownership state. On timeout or
abandon_emoji_assignment(), the task handle is dropped and the connected state is cleared, but the blocking OS thread may still hold the HID handle and continue writes.hid_write_task_owner_active()can then become false and the scanner may reconnect another worker concurrently; the generation guard only suppresses the stale UI result. Keep transport ownership until the old worker has actually exited (or make it cancellable/joinable), and test timeout/abandon followed by reconnect with no concurrent writes. -
Host-backend readiness is still not verified. Linux returns
falseunconditionally, so emoji assignment is disabled even when the installed IBus/Fcitx5 backend is usable. Windows returnstruewithout confirming that the hook started successfully. Please add a real readiness/health handshake and cover both unavailable and startup-failure paths. -
The disconnect transaction test still injects only a one-shot request failure (
pending.take()); the rollback then succeeds. It does not cover a persistent disconnect, rollback failure, or prove exactly-once success after retry. Those are the failure modes this transaction needs to guarantee. -
This branch still contains the full, unmerged #88 series. That also imports unresolved lifecycle data-loss paths: a normal close only defers when a worker is already active, so queued dirty writes can be lost before the worker starts; and a
RecordOldLayer operation is dropped when its worker disconnects. Please fix/land #88 separately, then rebase or split #81 so this PR contains only the emoji feature and its own lifecycle integration.
Local verification on this head: cargo test --all-targets passed 297/297; Clippy, i18n, git diff --check, and Python syntax checks passed. Scoped rustfmt for the PR range still fails (including src/ui/combo_write.rs and src/ui/emoji_assignment.rs).
|
Addressed:
The branch has been rebased onto current main after merged #89. The shared Settings/HID ownership path is integrated, and review-touched Rust files were formatted. |
kissetfall
left a comment
There was a problem hiding this comment.
Re-reviewed head 24d4ef7. The reclaim owner is a real improvement: timeout/state clear now keeps HID ownership busy until the abandoned worker exits, and persistent-disconnect plus exactly-once transaction tests were added. The six hosted checks are green and local tests pass 299/299. Blocking issues remain.
-
X11 readiness is still not an ownership guarantee. LINUX_X11_BACKEND_READY becomes true when any F13-F20 keycode exists, not specifically F20, and it is set immediately after XGrabKey calls. XGrabKey reports BadAccess asynchronously; another process can own the grab while Entropy still enables assignment. Require the actual F20 transport key and verify successful grab/event ownership before reporting ready. Add missing-F20 and grab-conflict coverage.
-
This branch still contains the older embedded #88 stack rather than the current reviewed #88 head. That carries the unresolved deferred-settings/close and picker-retry behavior into the emoji feature. Land #88 separately, then rebase/split #81 so the PR contains only emoji integration on the final shared lifecycle.
-
Scoped rustfmt still reports changes in this PR, including emoji_assignment.rs and key_assignment.rs.
Please keep the reclaim-task and persistent-disconnect work; those parts address the previous review.
|
Thank you for the work on this and for exploring direct emoji assignments in depth. After extensive consideration, we have decided not to implement the one-key-per-emoji approach. Spending a physical key and a macro slot on a single emoji is not a good tradeoff for keyboards with limited key space, and the host-assisted transport and platform-specific backend make the implementation disproportionately complex for that interaction. We have decided to support emoji through the Text Expander instead. Text triggers allow many emoji to remain available without consuming dedicated keys and fit the existing product model more naturally. This is a product-direction decision rather than a request for further revisions, so we are closing the PR without merging. No additional work is needed on this branch. Thank you again for the contribution and the research behind it. |
EN
Summary
This PR completes the end-to-end option from the latest review on #50: emoji selected in the Symbols picker can be assigned to a physical key or encoder and emitted by Entropy on the host.
The branch is rebased onto current
mainafter #50 merged. Its history and diff now contain only host-backed emoji output work.M0..M255keycode to the selected key or encoder.Implementation details
Verification
cargo test --locked: 209 tests passed.cargo build --locked --release: passed on macOS.python3 scripts/check_i18n.py: passed.rustfmtandgit diff --check: passed.RU
Кратко
Этот PR реализует полный end-to-end вариант из последнего review для #50: эмодзи из вкладки «Символы» можно назначить физической клавише или энкодеру, после чего Entropy выводит его на хосте.
Ветка перебазирована на текущий
mainпосле merge #50. История и diff теперь содержат только изменения host-backed emoji output.M0..M255выбранной клавише или энкодеру.Детали реализации
Проверка
cargo test --locked: прошли 209 тестов.cargo build --locked --release: прошел на macOS.python3 scripts/check_i18n.py: прошел.rustfmtиgit diff --check: прошли.