fix: 431 always-bundled emoji font + 453 pane-divider phantom selection - #456
Conversation
Freminal ranked system-installed emoji fonts by name priority (EMOJI_CANDIDATES) and, failing that, by scanning every font's cmap for emoji-block coverage, only falling back to the bundled Noto Color Emoji when nothing capable was found. But a system font's aggregate coverage count is not a stable guarantee that it maps any *particular* codepoint the terminal needs: a system font could win the ranking and still render tofu for a codepoint it happens not to carry. Concretely, a user's starship prompt showed the NixOS snowflake glyph (U+2744) as tofu because the ranked system "Noto Color Emoji" build was an older/narrower snapshot that had dropped the codepoint -- two "Noto Color Emoji"-named builds on one machine were observed to differ by 9 codepoints under the identical family name. Always use the bundled Noto Color Emoji face for the terminal grid. The set of codepoints that render correctly is now fixed and known at build time, independent of what is (or isn't) installed on the host, and the bundled face can be refreshed/replaced centrally if a gap ever appears. Removes the now-dead ranking machinery (EMOJI_CANDIDATES, best_system_emoji_source, resolve_emoji_source, EMOJI_SOURCE_CACHE, enum EmojiSource, load_emoji_face_from_source). has_color_glyphs, emoji_coverage, and EMOJI_BLOCKS survive only as #[cfg(test)] regression helpers (they had no production callers left). discover_emoji_face() no longer takes a font database and unconditionally loads the bundled face. The egui chrome path (fonts.rs) is deliberately untouched -- egui's grayscale atlas cannot render color glyphs, so it keeps its own separate monochrome-capable fallback list; only its stale doc reference to the deleted terminal-side ranking is corrected. Adds a regression test asserting the always-bundled face maps U+2744 (the snowflake from this report) to a real color glyph, not tofu.
Dragging a mux pane divider produced brief flashes of inverted (selection-coloured) pixels on a row of the adjacent pane. Root-caused via frame-by-frame diagnostic logging: the invisible border-drag sensor (egui `Sense::click_and_drag()`) sits geometrically inside the adjacent pane's `terminal_rect`, so the same primary press+drag that resizes the split ALSO starts and extends a real `ViewState` text selection in that pane. One endpoint pinned at the mouse-down anchor, the other tracked the drag; the selection survived past drag-end. (The earlier suspicion that this was a GPU deco-vbo / cursor-blink regression from the #432 fix was disproved by bisect + logging -- the deco-vbo path is self-consistent and this input-side gap predates it.) Fix: thread a `border_drag_active` flag (from `win.border_drag.is_some()`) into `FreminalTerminalWidget::show`, fold it into `suppress_input`, and fully `clear()` the selection while a border drag is active (rather than `finalize_interrupted_drag()`, which keeps a real range -- exactly the phantom we must discard, since the gesture was a resize, not a selection). Also, from review + user-requested behaviour: - Freeze focus-follows-mouse during a divider drag so the active-pane highlight/cursor no longer flickers between panes as the pointer moves over them mid-drag; on release, focus the pane the pointer ends in (new pure helper `panes::pane_at_pos` + the standard FocusChange dance). - Defensive per-frame reset of `win.border_drag` whenever the primary button is not held: the drag-sensor loop that normally clears it is gated behind `!ui_overlay_open`, so an overlay opening mid-drag (a keybind firing while the button is held) could otherwise strand `border_drag = Some` forever and -- now that it feeds global input suppression -- permanently freeze all input in the window. - One-frame suppression tail for border drag (`border_drag_was_active_ last_frame`, a separate latch mirroring `overlay_was_open_last_frame`) so the drag-release click cannot leak into the terminal on the frame the drag ends (matters for mouse-tracking-aware PTY apps). Tests: `pane_at_pos` hit-test (inside A / inside B / outside / empty); `border_drag_active_suppresses_input`; `border_drag_clears_phantom_ selection` (clear() discards vs finalize_interrupted_drag() keeping); `border_drag_one_frame_release_tail`.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesBundled Emoji Selection
Pane Resize Input Handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pointer
participant FreminalGui
participant pane_at_pos
participant TerminalWidget
participant PTY
Pointer->>FreminalGui: release divider drag
FreminalGui->>pane_at_pos: resolve release position
pane_at_pos-->>FreminalGui: target PaneId or None
FreminalGui->>TerminalWidget: render with border_drag_active
TerminalWidget->>TerminalWidget: suppress input and selection updates
FreminalGui->>PTY: drop queued raw keys during drag
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
freminal/src/gui/fonts.rs (1)
347-368: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CANDIDATESorder doesn't match the chrome fallback goal.find_candidatereturns the first installed match, but the list prefers several color emoji faces beforeSymbola, so egui can still end up picking a font its grayscale atlas won’t use properly. Move a monochrome-safe face first, or narrow the list to fonts that work for chrome text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@freminal/src/gui/fonts.rs` around lines 347 - 368, Update the CANDIDATES priority used by add_emoji_fallback so a monochrome-renderable face such as Symbola is selected before color-only emoji fonts. Reorder or narrow the list to preserve reliable egui chrome rendering while retaining find_candidate’s first-installed-match behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@freminal/src/gui/app_impl.rs`:
- Around line 1886-1914: Update the release-focus transfer in the active-pane
handler to refresh the local active-pane ID and active-pane-changed state after
assigning tab.active_pane, so later rendering and damage calculations use the
target pane immediately. Re-arm the target pane’s blink anchor when focus
transfers, using the existing blink state mechanism and symbols near the
active-pane snapshot.
- Line 2278: Update the pending_raw_keys clearing and draining logic surrounding
the GUI show flow to suppress queued raw-key input while border_drag_active is
true and for the existing one-frame release tail. Ensure events accumulated
during divider resizing are discarded rather than forwarded to the PTY, while
preserving normal draining once the release tail completes.
---
Outside diff comments:
In `@freminal/src/gui/fonts.rs`:
- Around line 347-368: Update the CANDIDATES priority used by add_emoji_fallback
so a monochrome-renderable face such as Symbola is selected before color-only
emoji fonts. Reorder or narrow the list to preserve reliable egui chrome
rendering while retaining find_candidate’s first-installed-match behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d4ccbd9-9ae8-4881-9389-fdadb906c154
📒 Files selected for processing (6)
ATTRIBUTIONS.mdfreminal/src/gui/app_impl.rsfreminal/src/gui/font_manager.rsfreminal/src/gui/fonts.rsfreminal/src/gui/panes/mod.rsfreminal/src/gui/terminal/widget.rs
| // On release, focus the pane the pointer ends in (focus | ||
| // was frozen during the drag, issue #453). Use the | ||
| // release pointer position; fall back to leaving focus | ||
| // unchanged if it isn't over any pane. | ||
| if let Some(pos) = ui.ctx().pointer_hover_pos() | ||
| && let Some(under_id) = panes::pane_at_pos(&pane_layout, pos) | ||
| { | ||
| let tab = win.tabs.active_tab_mut(); | ||
| if tab.active_pane != under_id { | ||
| let old_active = tab.active_pane; | ||
| if let Some(old_pane) = tab.pane_tree.find(old_active) | ||
| && let Err(e) = | ||
| old_pane.input_tx.send(InputEvent::FocusChange(false)) | ||
| { | ||
| error!( | ||
| "Failed to send FocusChange(false) to pane {old_active}: {e}" | ||
| ); | ||
| } | ||
| tab.active_pane = under_id; | ||
| if let Some(new_pane) = tab.pane_tree.find(under_id) | ||
| && let Err(e) = | ||
| new_pane.input_tx.send(InputEvent::FocusChange(true)) | ||
| { | ||
| error!( | ||
| "Failed to send FocusChange(true) to pane {under_id}: {e}" | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Refresh current-frame active-pane state after release.
Line 1574 snapshots active_pane_id before this handler, but Lines 1904-1912 change tab.active_pane. The rest of this frame still renders the old pane as active and computes damage from that stale ID; with no later repaint request, the cursor/border can remain visually focused on the wrong pane until another event. Update the local active-pane and active-pane-changed state when transferring focus, and re-arm the target pane’s blink anchor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@freminal/src/gui/app_impl.rs` around lines 1886 - 1914, Update the
release-focus transfer in the active-pane handler to refresh the local
active-pane ID and active-pane-changed state after assigning tab.active_pane, so
later rendering and damage calculations use the target pane immediately. Re-arm
the target pane’s blink anchor when focus transfers, using the existing blink
state mechanism and symbols near the active-pane snapshot.
| &pane.clipboard_rx, | ||
| &pane.search_buffer_rx, | ||
| ui_overlay_open, | ||
| border_drag_active, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Suppress queued raw-key input during divider drags.
Line 2278 suppresses input handled inside show, but the pending_raw_keys drain after show does not check border-drag state or its release tail. Raw keypad/media events typed during a resize can therefore still reach the PTY. Apply the same drag suppression—including the one-frame release tail—when clearing or draining that queue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@freminal/src/gui/app_impl.rs` at line 2278, Update the pending_raw_keys
clearing and draining logic surrounding the GUI show flow to suppress queued
raw-key input while border_drag_active is true and for the existing one-frame
release tail. Ensure events accumulated during divider resizing are discarded
rather than forwarded to the PTY, while preserving normal draining once the
release tail completes.
Follow-up to PR #456 review: the Task-114 raw-key queue (`win.pending_raw_keys`, keypad/media/print/pause/menu keys that egui blocks) was drained straight to the PTY even while a pane divider was being dragged, so those keys could leak mid-resize. `show()` already receives `border_drag_active` and suppresses normal input, but this separate out-of-`show()` drain path only gated on overlay state. Add `border_drag_active` to the drain gate so queued raw keys are dropped (not forwarded) during a divider drag, upholding the same "no terminal input while a border drag is active" invariant. No one-frame release tail is needed here (unlike the pointer path): a raw key is only queued if physically pressed, and drag-end synthesizes no key event, so gating on the current-frame flag is sufficient. Doc on `drain_pending_raw_keys` updated to record the border-drag suppression cause and why it needs no tail.
Follow-up to PR #456 review (pre-existing bug surfaced by the diff): the egui chrome-text emoji fallback (`add_emoji_fallback`) picked the first system emoji font by NAME with no color-vs-monochrome check. egui's chrome atlas is grayscale-only and cannot rasterize color glyphs (COLR/CPAL, CBDT/CBLC, sbix) — it locks glyph ownership by cmap and then renders a color-only glyph as blank. So on a common setup (Noto Color Emoji installed, no Symbola) chrome emoji/symbols rendered blank even though the doc comment claimed the list was ordered to prefer a monochrome-renderable face. Enforce that claim: `find_candidate` now parses each name-matching face with swash and skips any that carries color glyph tables (mirroring the terminal-side `has_color_glyphs` technique), so only a face egui can actually rasterize is returned; the color-font entries in the candidate list become harmless no-ops. Fail-closed: a face swash cannot parse is also skipped. This only affects the egui chrome path; the terminal grid keeps its own color-capable swash/atlas pipeline (bundled Noto Color Emoji, issue #431). Tests: `is_color_font` classifies the bundled Noto Color Emoji as color (skip) and bundled CaskaydiaCove as monochrome (keep).
CodeRabbit review — dispositionsThanks for the review. All three findings verified against the code; two fixed, one intentionally left unchanged with rationale. ✅ Fixed — raw-key drain during divider drag (
|
1 participant
Two related-in-scope-only bug fixes, one atomic commit each.
Closes #431 — always use the bundled emoji font (
e5bafe12)Freminal ranked system-installed emoji fonts (by name priority, then by
scanning each font's
cmapfor emoji-block coverage) and only fell backto the bundled Noto Color Emoji when nothing capable was found. But a
system font's aggregate coverage is not a stable guarantee that it maps
any particular codepoint: a font could win the ranking and still render
tofu for a codepoint it happens not to carry. Observed concretely with a
starship prompt's NixOS snowflake glyph (U+2744) — and two
"Noto Color Emoji"-named builds on one machine were measured to differ by
9 codepoints under the identical family name.
Now the terminal grid always uses the bundled
res/NotoColorEmoji.ttf:the set of codepoints that render is fixed and known at build time,
independent of the host. Removes the dead ranking machinery
(
EMOJI_CANDIDATES,best_system_emoji_source,resolve_emoji_source,EMOJI_SOURCE_CACHE,EmojiSource,load_emoji_face_from_source);has_color_glyphs/emoji_coverage/EMOJI_BLOCKSsurvive only as#[cfg(test)]regression helpers. The egui chrome path (fonts.rs)is deliberately untouched (its grayscale atlas can't render colour glyphs
so it keeps its own monochrome fallback list). Regression test asserts
the bundled face maps U+2744 to a real colour glyph.
Closes #453 — pane-divider drag painted a phantom selection (
95a9182b)Dragging a mux pane divider flashed inverted (selection-coloured) pixels
on a row of the adjacent pane. Root-caused via frame-by-frame diagnostic
logging (after a bisect proved noisy and an initial GPU-path theory was
disproved): the invisible border-drag sensor sits inside the adjacent
pane's
terminal_rect, so the same press+drag that resizes the splitalso starts and extends a real text selection there.
Fix threads a
border_drag_activeflag into the terminal widget, folds itinto input suppression, and fully clears the phantom selection during a
drag. Plus, from review + user request:
drag (no active-pane flicker between panes); on release, the pane under
the pointer becomes active.
win.border_dragis force-cleared any frame theprimary button isn't held, so an overlay opening mid-drag can't strand it
Someand freeze all input.terminal (matters for mouse-tracking PTY apps).
Verification
cargo test --all— all pass (new tests for emoji bundling,pane_at_poshit-test, border-drag suppression/clear, one-frame release tail)
cargo clippy --all-targets --all-features -- -D warnings— cleancargo machete— cleancargo xtask check-windows— cleanSummary by CodeRabbit
Bug Fixes
Improvements
Documentation