fix(select): keep the edge columns when dragging off the grid [4] - #206
Draft
AbysmalBiscuit wants to merge 73 commits into
Draft
fix(select): keep the edge columns when dragging off the grid [4]#206AbysmalBiscuit wants to merge 73 commits into
AbysmalBiscuit wants to merge 73 commits into
Conversation
A build saturating every core starves the shell the user is typing into: the line editor redrawing its prompt waits behind sixteen compilers at the same scheduling class, and the keystroke round trip stretches into seconds. Raising the shell alone does not reach that, because a Windows priority class does not spread to the processes a raised process starts. A job object does: a process joins the job it is created in and comes up at the job's class, at any depth and with no scanning. The session takes the job at spawn rather than on focus, since anything started before the job exists escapes it for good. Only the session on screen, and only while the window has focus, is raised; the GUI follows so it does not lose to the tree it draws. Behind `[ui] focus_priority_boost`, off by default, and Windows only. A Unix nice value is already inherited and cannot be lowered back without privilege, so the platform module there is a no-op. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A job's priority class is a ceiling, not a setting. Releasing the boost named normal but left the limit standing, so every process an unfocused session held was capped there and could not raise itself — a build or an agent running under a background tab lost the class it asked for, and kept losing it until that tab was focused again. Release now names normal to lower the members already running, then clears the limit so they own their class again. Drop is the same two steps, so it needs nothing beyond releasing the boost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The console reaps only the programs attached to it, so anything that leaves it — an editor's background search, anything started detached — outlives the terminal that asked for it and piles up until the machine is rebooted. Nothing else holds those processes: the job object is the only thing that has every descendant at every depth. `[ui] reap_descendants_on_close`, off by default, gives the job kill-on-close, so the kernel ends its members when the last handle goes. That covers a killed or crashed alacritree as well as a closed tab, since no cleanup path has to run. Breakaway rides with it: a process that means to outlive the terminal asks with `CREATE_BREAKAWAY_FROM_JOB` and is let go, which is a request the kernel refuses without the flag. The lifetime limits travel with the priority one on every set, because `SetInformationJobObject` replaces `LimitFlags` whole and a change of focus would otherwise drop them. One job serves both options, so it is created when either wants it. The tests tear down real pseudoconsoles and check what outlived them, against an unjobbed baseline that pins any failure on the harness rather than the feature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reaping option shipped without a test that the console misses anything, so nothing said whether the job was fixing a real leak. The escaping child is now a process started with `DETACHED_PROCESS`, which has no console and so is nobody's console client — the shape of a completion helper an editor spawns. Only a process already inside the session can start one, so the session runs this binary again and an ignored test does the spawning; that also keeps the desktop clear, where a new console would have opened a window. Two arms make the claim falsifiable: unjobbed, the child outlives the teardown, and a failure there would mean there is no leak to fix; jobbed, it does not. The session is jobbed before its tree is waited for, as a real session does it, because a process joins a job when it is created and anything already running stays outside it for good. Members carry their name and their kernel-reported job membership, so a survivor says on its own whether the model or the flag is at fault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The cmd.exe subjects and the taskkill sweep carried no CREATE_NO_WINDOW, so each one flashed a console window during a test run, and the subjects holding a `ping` open kept theirs for a minute. command_ext exists for this; the tests pipe their stdio and assert on job membership and priority, neither of which the flag touches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A test binary has no `main`, so hardening was left to each test that
opened a pseudoconsole. The focus-priority tests added one that does not,
and once any PTY opens first the module it loaded answers every later
`LoadLibraryW("conpty.dll")` in the process. That is the three-second
console-host stall the handshake test exists to catch, and it started
failing on CI for tests that never touched it.
Harden immediately before each `tty::new` instead, guarded by a `Once` so
the repeat costs nothing and `main` keeps its startup call. The per-test
calls go away with the convention they were upholding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pid came from a subject the test had already killed and reaped, so Windows was free to hand that number to anything. Under a parallel run it handed it to another test's freshly spawned subject, which then read ABOVE_NORMAL where it expected NORMAL. Use a number the kernel never allocates, and assert that it names no process rather than trusting the arithmetic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every subject was born at whatever Windows chose, and the boost tests then asserted that choice was normal. Windows hands a new process its creator's job class when the creator is in a job, and half of these tests build jobs, so under a parallel run a subject sometimes came up above normal and the first assertion failed before the boost was even applied. Pass the class in the creation flags. The one test whose subject exists to report Windows' choice keeps making none, and says what to suspect if it ever comes up raised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`tree_of` walked the process table by parent pid alone. Windows frees a pid for reuse the moment the last handle to it closes, so an orphan goes on naming the number its dead parent had and whoever is given that number next inherits it as a child. The teardown arms taskkill every survivor they find, which turned a wrong answer here into an unrelated process on the machine being killed: a build's stray vctip.exe was reaped as part of a session tree it never belonged to. No child predates its parent, so a start time earlier than the parent's rules the candidate out. A process the snapshot cannot open reports no start time at all, and those keep their place rather than being read as born at the epoch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Font fixtures went into the shared system temporary directory under fixed names. Unique within one binary, but two binaries running at once write the same paths, and a fixture one of them has mapped cannot be rewritten by the other: Windows fails the write with ERROR_USER_MAPPED_FILE. Five fonts tests failed together in one run here, which is what a second binary mapping its whole fixture set looks like. Hang the fixtures off a directory named for the process instead, and sweep the ones whose process has gone. The names lose the prefix that was disambiguating them inside the shared directory. Closes #46 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The probe for a foreign console host asserted that a pane's child
exited inside two seconds, then ten. Both bounds are wall-clock, so a
Windows runner that descheduled the polling thread failed the test with
nothing wrong: once on each bound, both passing on re-run.
What the probe is there to catch is `LoadLibraryW("conpty.dll")`
resolving out of PATH, and that is observable directly. A file named
conpty.dll that is not a module tells the two loader outcomes apart: a
loader that reaches PATH finds it and reports ERROR_BAD_EXE_FORMAT, a
hardened one reports ERROR_MOD_NOT_FOUND. Both arms run as child
processes because SetDefaultDllDirectories is process-wide and cannot
be undone, so the parent cannot host the unhardened arm.
The control arm pins the unhardened code as well, so a plant the loader
never reaches fails the test rather than passing it by default.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`the_probe_hands_the_scan_to_the_refresher` asserted the call returned within 5 ms. A bound that tight fails whenever the runner deschedules the thread, and it cannot distinguish that from the failure it is aiming at, since a probe that scanned inline would blow it for the same reason a busy machine does. `probe` reads the published map and registers interest; it never scans. The assertion above it already pins the answer to the default, which only an unscanned probe can return, and the cost the bound stood for is what `report_process_probe_cost` reports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`each_phase_measures_only_its_own_span` slept 20 ms to give the first phase a span, then asserted the second one came in under 10 ms. The bound is what fails when a runner deschedules the thread between two adjacent statements, and 10 ms is close enough to be reachable. Back-dating `since` gives the first phase five seconds without waiting for them, and the claim is stated against it: a second phase still dating from the frame's start would be at least as long as the first, so `second < first` is the property, not an absolute ceiling. The test also stops costing 20 ms of every run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AbysmalBiscuit
marked this pull request as draft
September 2, 2026 15:58
The diff-pane title tests spawned a real `cmd /c exit` through ConPTY purely to obtain a `Session`, then polled up to ten seconds for the child to exit so its startup title could not race the injected one. A loaded runner reaches that bound with nothing wrong, which is what broke CI. The session under test needs no PTY: the tests inject their sequence straight into the terminal and drain it. Building it as a struct literal removes the child, the wait and the racing title together. Co-Authored-By: Claude Opus 5 (1M Context) <noreply@anthropic.com>
Issue #17 asked where SIMD could help the paint path. Answering it needs the frame broken into pieces that can be timed on their own, and a prototype of the alternative to time against. Four reporting harnesses, none of them gates: - report_build_breakdown splits the build phase into the grid read, the galley lookup, and the run walk. - report_lookup_shapes compares the hashed galley cache against a table indexed by character. - report_fill_variants writes the same vertices four ways, down to unchecked stores, against a plain memcpy of the finished buffer. - report_mesh_frame paints the whole grid as one Shape::Mesh built from a character-indexed table, with geometry following `show` so its numbers compare directly against report_paint_cost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Handing epaint a mesh still costs a full copy of every vertex: the CPU
writes 105k vertices for a 2560x1440 screen and epaint copies all of
them into a fresh buffer before the GPU sees anything. Almost all of
that work is position arithmetic a vertex shader does for free.
`[ui] gpu_grid` routes the grid to a paint callback instead. One
twelve-byte record per cell replaces four twenty-byte vertices, the
vertex shader derives each quad, and backgrounds become a single
triangle over the viewport indexing a cell-sized texture, so neither
draw's geometry grows with the grid. epaint sees one shape carrying no
vertices.
Nothing here owns a glyph atlas: `egui_glow::Painter::texture` hands
over the texture epaint already packed, so the shader samples the same
artwork, and the fragment shader repeats epaint's gamma-space multiply
so glyph weight matches the rest of the UI.
Records sit at a fixed column stride, so a frame that rewrote three
rows uploads three rows rather than the grid.
Off by default. It needs a GL 3 context for instanced arrays and
bypasses the renderer every other panel goes through; a context too old
logs once and draws nothing rather than falling over. Emoji,
box-drawing glyphs, underlines and the cursor stay on egui's painter,
emitted after the callback.
Measured with the harnesses in terminal_view, 318x83 cells, median of
three passes:
today 4.55 ms (build 2.29 + tessellate 2.16)
Shape::Mesh 1.01 ms (build 0.31 + tessellate 0.63)
gpu_grid 0.18 ms (build 0.18 + tessellate 0.001)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both error paths in `link` abandoned the program and every shader they had created. On its own that would be a one-shot leak, but the callback left the resource slot empty after a failure and so rebuilt on the next frame, and the next: a driver that rejects the shaders leaked a program plus two shader objects at the repaint rate, with nothing on screen to say why memory was climbing. Release what a failed link made, and latch the failure so the build is attempted once. A context that cannot compile the shaders now logs one line and draws nothing, which is what the config flag being off already does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every frame re-walked the whole grid and rewrote every cell record,
which is why a full screen cost 179us of CPU before the GPU saw
anything. Almost none of that work was needed: a terminal frame
usually changes one row.
The snapshot now stores each viewport row separately, so re-walking one
row never moves another's bytes, and `collect_damage` decides which rows
to walk. It mirrors `Display::update_damage` upstream: take
`Term::damage`, then add what that method documents as the caller's to
track. The selection and the link highlight restyle cells the terminal
never wrote, so both edges of a change to either are re-walked; a
resize, a scroll, a palette change and the first capture all come back
as the full range.
One snapshot serves every session, so a change of session invalidates
every row too — otherwise a switch would show whatever the previous
terminal left in the rows the new one did not touch.
The GPU path builds run views and writes records for that span alone,
and marks exactly it dirty, so the partial upload the fixed row stride
was built for finally gets a narrow range to send.
Measured with the harnesses in terminal_view, 318x83 cells, repeat
frames where only the cursor row is damaged:
build 179us -> 16.6us
capture 89us -> 5.3us
frame 181us -> 17.4us
A frame that genuinely changes everything still pays the old price;
this buys the common case, not the worst one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The GPU path walked every run on screen twice per frame, once to build the record views and once to re-emit underlines, then discarded all but the damaged row. That walk alone cost more than everything else in the build put together. Narrow the record walk to the damaged span, and give each row a flag for whether it carries an underline or a strikeout so the decoration pass skips rows that have neither. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Everything outside ASCII resolved through a HashMap with the default hasher, so a CJK or Nerd-Font screen paid a cryptographic hash for a key no attacker chooses. Pack the face and the character into one integer and hash it with a multiply-shift, which drops the lookup from about 9.6 ns per cell to 1.4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Everything outside ASCII resolved through a HashMap, so a CJK or Nerd-Font screen paid a hash and a probe per cell where Latin text paid an array index. Replace both paths with one two-level table: the low byte of the character picks an entry within a page, the rest picks the page, and pages live in an arena so an untouched one costs no allocation and the lookup has nothing to branch on. Non-ASCII drops from about 9.6 ns per cell to 0.7; ASCII gives up 0.1 ns for the second load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Backgrounds travelled as their own RGBA8 texture, so every frame paid two uploads for one grid and a cell's colours sat in two places. The record also carried cell coordinates it did not need: records sit at a fixed row stride, so the cell is the instance index. Drop the coordinates, put the background in the space they freed, and draw backgrounds as a quad instanced from the same buffer. A full screen now sends 309 KiB in one upload where it sent 309 KiB plus a 103 KiB texture in two, and a blank row clears with a fill. The strip past the last whole cell is covered by clearing inside the callback, which egui has already scissored to the grid. Padding the record to sixteen bytes for cache-line alignment was measured and rejected: dead padding costs 1.24 to 1.40 ns per cell for nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
egui-winit raises Event::Copy alongside the key press for ctrl+c, and a program whose only exit is the interrupt cannot be stopped if the pair resolves to nothing. Assert the whole event stream yields ETX rather than the key event alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every timing harness here drives dense_screen, which is written once and then never changes, so from the second frame on the terminal reports no damage. A renderer that skips clean rows measures as doing almost nothing, and the numbers come out nearly independent of grid size. termbench_frame transcribes FGPerChar and FGBGPerChar from termbench.cpp: a truecolour SGR on every cell, colours derived from the frame index so consecutive frames share no cell. Damage is full every frame, which is what streaming output actually produces. Parsing runs off the clock and both paths see byte-identical frames. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The frame harness timed `ctx.run` end to end, so every reading of it had to guess which part of the paint the number belonged to. A per-cell figure derived from that total was attributed to whatever the reader already suspected, and one such guess put the blame on a run vector without evidence. Split the frame with `phase!`, which compiles to its body alone outside a test build, and count allocations through the counter `steady_state` already provides. Give the workload a stride knob so run count moves independently of cell count, and step both painters through the same frames in the same round rather than one after the other — read in sequence, a quiet minute and a busy one land on different paths and read as a difference between the paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`write_rows` took a slice, so the caller collected its runs first. The source is a `flat_map` whose `size_hint` floors at zero, so the vector grew by doubling: a full-screen redraw with a colour per cell allocated about three megabytes every frame for a sequence read once, front to back. On Windows a block that size skips the heap front end for VirtualAlloc, which charges a soft page fault per page on first touch. Take `impl IntoIterator` instead and hand the mapped iterator straight through. Per-frame allocation for the GL path drops from 3.01 MB to 8 kB, and paint of a 318x83 screen of per-cell colour from 1.25 ms to 0.75 ms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A slot holds texel coordinates into epaint's font atlas, and the table returns a cached slot without laying the character out again. When epaint repacks or rebuilds that atlas the coordinates keep pointing at whatever landed in their place, and nothing in the GPU path ever notices: the only invalidation was on font size. Give the table the atlas guard the mesh path's GlyphCache already has, so a rebuild drops every slot and the next frame re-reads them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Changing the font size left the terminal drawing the wrong glyphs, or none, and it did not recover. A cell record names a slot, and a slot is texels into egui's atlas, so both have to be the ones live at paint time. Three ways they were not: The atlas size was read while the frame was still being built. Laying a glyph out doubles the atlas when it runs out of room, and egui normalizes every uv against the size the atlas ended the frame at, so the shader was dividing texels by half an atlas. It is read in the paint callback now. Clearing the table renumbers it from nothing, but only the rows the terminal reported damaged were rewritten. Every other cell kept an index into the old numbering and drew whatever took the index over. The clear is now `begin_frame`'s alone and it says when it happened, which makes the frame rewrite the whole grid. Slots were a texture row each, capped at the 2048 rows a driver is obliged to offer, while the table handed out indices up to 65534. A screen of CJK or Nerd Font icons reaches that and fetches out of range. Packing slots across the row puts the ceiling back on the u16 index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every timing harness here starts the clock after `Parser::advance` has already run, so a frame's cost reads as though the bytes arrived parsed. On the GL path that hides the larger half: an SGR pair per cell costs 5.7x the whole painter, and even plain characters with no escapes at all cost as much again as painting them. `report_parse_share` times both sides of the same frame on the GL path and prints the split. Its three workloads bracket real output instead of describing it, and `ALACRITREE_BENCH_WORKLOAD` narrows a run to one so a sampled profile has a single thing in it. `colored_frame` grows a `Colors::None` for the no-escape floor, replacing the `backgrounds` flag that could not express it. `report_colored_frame` keeps its own workloads and its numbers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The callback clears the whole grid rect to the default background and then draws an opaque quad over every cell, so a screen where most cells carry that background repaints it once per cell for no visible change. On an idle grid that is nearly the entire pass. Collapse such a cell to a point in the vertex shader, the way the decoration pass already collapses an undecorated one. Alacritty reaches the same end from the other side, giving the cell zero alpha and discarding it in the fragment shader. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`gpu_deco_ab` could only price the decoration gate. Replace it with `gpu_ab`, naming the skip under test, so the background collapse gets the same paired comparison inside one process. The baseline arm draws every background quad by comparing against a colour no normalized byte can hold, which no cell matches. Both arms then take the same code path and differ only in a uniform, where a branch around the draw would have differed in more than the skip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Negative result. Do not merge; the branch is the record. The glyph shader ran egui's general conversion, recovering coverage from the colour channels with three pow() calls per fragment. That work is redundant here: epaint writes every font texel as `from_rgba_premultiplied(a, a, a, a)`, colour glyphs never reach this pass, and alpha survives an sRGB atlas untouched. The decoration pass already reads its mask that way. Removing all three pow() changes nothing the instrument can see. Over 148 pairs the glyphs stage came out at ratio 1.010, p 0.242, against a null control of exactly 1.000. The pass is bound by writing blended fragments, not by what it computes for each one, which is also why removing background quads won so much and this wins nothing. It is not free either: the sRGB round-trip it skips moves 1.5% of pixels by one 255th. A change that costs output and buys no time should not ship. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The four per-stage brackets were the only GPU measurement and `total` was their sum, which is not the frame. The clear belongs to the frame but to no stage, so nothing counted it. And a bracket that ends at bottom-of-pipe charges its stage for a drain the next stage would otherwise have overlapped, which no null control can detect: an empty bracket has nothing in flight and reads near zero while every real one stays inflated. GL_TIME_ELAPSED cannot nest, so the whole-callback bracket alternates with the per-stage queries frame by frame instead of wrapping them. One report window carries both, and the gap between them bounds how much of a stage median is the pass rather than the instrument. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A microsecond figure from the paint callback cannot be judged without knowing which device drew it, and this machine has two. Log the GL vendor, renderer and version once per context, and put the grid dimensions and each median's sample count on the report line so an availability miss stops hiding inside a bare median. The comment beside the total accumulation claimed a frame short of a stage keeps no total. Only an unavailable result does that; a gated decoration pass leaves a complete three-stage frame. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A glyph's atlas rectangle is a box around the ink, and most of that box
is transparent. Those fragments run a premultiplied blend whose result
equals the destination they overwrite, and the pass has never been
measured without them. `gpu_ab = "fill"` links a second glyph program
that discards exactly-zero coverage and alternates it against the
shipped one.
`gpu_ab = "blend"` does the same for the background pass, whose colours
all come from `Color32::from_rgb` and so are opaque: premultiplied
blending there computes the source it already had.
The per-instance colours also become `flat`. Every vertex of a quad
carries the same value, so interpolating it computes a constant;
alacritty qualifies the same values the same way in
`res/glsl3/text.{v,f}.glsl`. It rides in both arms of every experiment
here, so it cannot bias one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`paint_grid_gpu` sent every character through the monochrome atlas, so an emoji came out as whatever silhouette egui's font chain had for it, multiplied by the cell's foreground. Box-drawing characters lost their built-in shapes the same way. Only the mesh path consulted `ColorGlyphCache` and `BuiltinGlyphCache`, and `[ui] gpu_grid = true` never reaches it. Cells those caches own now record an overlay instead of a slot: the grid leaves the cell blank and the painter draws the glyph over the callback, where it lands above the grid the way the mesh path draws it above its own backgrounds. Overlays are kept per row and rebuilt with the row, so a frame that damages three rows rescans three rows. ASCII short-circuits before either cache, because it is most of what a terminal holds and none of it is box drawing or emoji. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The whole-callback bracket exceeded the four stage brackets by about 88us, and nothing measured where that went. The clear was the suspect because it sits outside every stage, but a suspect is not a number. Add it as a fifth timed stage. It reads 100us of a 359us callback, and the residual drops to -12us, which prices the instrument's own bracket tax at roughly 3us per bracket. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The clear costs 100us of a 363us callback, which is far too slow for a constant-colour write and points at the scissor egui leaves enabled keeping it off the fast path. Add three A/B arms against it: lifting the scissor, painting the rect with a full-rect quad, and skipping the grid's clear on the grounds that eframe already cleared the framebuffer to the same colour. The quad and the skip each save about 95us; lifting the scissor saves 38us and relocates the rest into the background pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things were wrong with the colour behind the grid. It came from the configured palette while cell colours resolved against the terminal's live one, so OSC 11 left a border of the old colour and stopped the background collapse from ever matching -- every cell drew a quad it did not need. The capture now records the resolved background and both paint paths read it from there. The alpha was not premultiplied. egui_glow hands the clear colour to glClearColor untouched and the compositor reads the framebuffer as premultiplied, so a translucent window added the background at full strength instead of scaling it, and no opacity setting could fade it out. Alacritty writes (rgb * alpha, alpha) in renderer::clear. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
eframe clears the whole framebuffer to the terminal's background before the callback runs, so the callback cleared a rect that already held the colour it was painting. Under egui's scissor that second clear missed the driver's fast path and cost 100us of a 363us callback, and because glClear ignores blend state it also stamped alpha 1.0 over the grid, leaving the terminal opaque on a translucent window while the sidebars were not. A static screen renders identically without it, 0 pixels of 1,764,636. The A/B arms that priced the alternatives -- lifting the scissor, painting a full-rect quad, skipping the clear -- go with it, along with the timing stage that bracketed it. Their measurements are written up in the bench harness. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The compositor reads the framebuffer as premultiplied, so scaling the background by the opacity is what alacritty's renderer::clear does and is the correct encoding. Applying it here would darken every [window] opacity already tuned against the unscaled colour, which is a visible change to a setting nobody asked to have changed. Keep the current encoding and record why in a comment, so the divergence from upstream is a decision rather than an oversight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A wide character owns two cells, and the terminal writes both from one cursor template: the spacer carries the same colours and the same SGR flags as the character it belongs to, differing only in holding WIDE_CHAR_SPACER where the lead holds WIDE_CHAR. Capture read that one bit as style and dropped the spacer, so a run ended between the halves of a single character and the right half of every CJK glyph came out with no background and no decoration. Both painters take a run's extent from its character count, so both showed it. Keeping the spacer as a character of its run and masking the two bits out of the style comparison leaves every cell carrying its own record, which is how kitty holds a wide glyph: it copies the lead cell's GPU record into the spacer when the character is parsed and never asks how wide anything is while rendering. alacritty takes the other route, dropping spacers from its renderable cells and adding the column back when it draws a line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The renderer was chosen by comparing candidates: three record layouts, four vertex-fill loops, three galley-lookup shapes, a whole-grid `Shape::Mesh` against per-glyph `TextShape`, and two alternative writer bodies run against one captured snapshot. Each of those comparisons decided something that is now in the shipped code, and none of them can fail, so they cost review attention and give nothing back. The harnesses that measure the shipped path stay, as they did before this work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`paint_phases` split a frame into capture, glyph lookup and record write so a ranking could say which of them dominated. The answer shaped the renderer and nothing reads the counters now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The timers carried a second job: alternating one of the callback's skips between report windows so both arms met the same driver and the same grid. That priced the decoration gate, the background quad, the glyph shader's coverage read and the two blend skips, and every one of those questions is settled. Its baseline arms drew the frame wrong on purpose, which is not something a release binary should be able to do. `[debug] gpu_timing` and its per-stage report stay. `[debug] gpu_ab` goes with the arms it selected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The doc comment on both `gpu_timing` fields held the same paragraph twice, and schemars concatenates a field's doc into the hover text the published schema carries, so an editor showed it twice too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The DOUBLE arm split the descent area between its two stems without a floor, so a stroke reaching a quarter of the descent closed the gap and the pair rendered as the single rule the style exists to differ from. A thickness knob turned up reaches that on an ordinary face. Clamp the stems' spacing to twice the stroke, leaving a stroke's worth of blank between them whatever the face reports. The percentage-scaling test could not fail for its own bug either: the default face at a 16pt ascent resolves to a whole-pixel stroke, where rounding before and after a scale agree. Anchor it on an ascent that makes the stroke fractional. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`resolve_fallbacks` substituted a default only when a metric was zero or non-finite. A face reporting a non-negative descender or a non-positive ascender slipped past that check, and each inverts a downstream calculation: `descent = -metrics.descender * px_per_em` goes negative and collapses the curl underline to its amplitude floor while pushing both double-underline stems above the baseline, and a non-positive ascender flips `px_per_em` outright. Add `correctly_signed`, a sibling to `nonzero`, and use it for these two fields only — the other four are legitimately signed (a negative `underline_position` is correct) and stay on the zero-only check. The ascender rejection also has to feed the resolved (possibly default) ascender into the strikeout-position fallback rather than the rejected raw value, since that fallback already reads the local `ascender` variable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`underline_position` claimed to move "the underline", but the double and curly styles anchor entirely on baseline and descent in `decoration_sprites.rs` and never read it — that placement mirrors upstream alacritty's `renderer/rects.rs` and is intentional. Document which three styles the knob actually reaches instead of implying all five. `underline_thickness`, confirmed by reading `draw_underline` and `curl`, does reach every style, so its doc gets a matching note for symmetry. `Adjust`'s type doc claimed kitty parity outright. kitty derives its double and curly underline positions from the face's underline position, so a value that behaves identically in kitty is a no-op for those two styles here. Qualify the claim so it is true. Regenerated schema/alacritree-config.json via ALACRITREE_UPDATE_SCHEMA=1 cargo test -p alacritree --test config_schema. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Geometry::resolve wires four knobs by hand, and the strikeout pair had no test: a copy-paste swap onto the underline knobs would compile and pass the whole suite. Mirror the existing underline knob tests for strikeout_position and strikeout_thickness, each also asserting the underline field stays put so a swap in either direction fails here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The DOUBLE arm pulled the lower stem up when it would leave the cell but left `upper` unclamped, so a descent band much larger than the cell room drove its whole span below row 0. `rect_x` then drew nothing for it, leaving a single rule where the style promises a pair. Clamp `upper` to stay at or below `t / 2.0` from the top, matching kitty, which clamps both ends the same way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ascent probe built a String and a LayoutJob every frame before reaching egui's galley cache, and it ran in show() unconditionally, so the mesh path paid for it too. GlyphCache already holds the same galley at the same size, so the probe becomes a map lookup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A binding's `action` was a bare string, so an editor offered no completion for it and a typo stayed silent until runtime. A closed `enum` would be wrong the other way: the shared `alacritty.toml` legitimately carries actions only the real alacritty implements, which alacritree ignores rather than rejects, and an editor would paint those red. An `anyOf` pairs an `enum` of every name alacritree implements with an open string branch. taplo completes from the names; every other value still validates. The names come from `bindable_actions`, moved next to `NamedAction` now that the command palette is not its only caller, so the suggestions cannot drift from the enum they describe. Co-Authored-By: Claude Opus 5 (1M Context) <noreply@anthropic.com>
cell_at_pos took Side from the unclamped pointer position, so left of the grid col_f went negative and col_f - col_f.floor() read >= 0.5 across the half-cell strip next to column 0. That anchored the selection to the right of column 0 and dropped its character, flipping back to Left another half cell further out — which is why the selection only landed when the pointer was placed precisely. Past the right edge the same arithmetic reported Left and dropped the last column. Clamp the position into the grid before deriving the column and the side from it, as alacritty's saturating Mouse::point and the x >= end_of_grid arm of its cell_side do. Co-Authored-By: Claude Opus 5 (1M Context) <noreply@anthropic.com>
AbysmalBiscuit
force-pushed
the
fix-selecting-text-near-the-left-side-of-the
branch
from
September 2, 2026 17:29
b8ec201 to
17329ff
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR (human written)
Selecting text with the mouse would drop the leftmost character unless you placed the pointer just right. Dragging off the left edge of the window, which is the natural way to grab a whole line, was the worst case.
The cell-under-pointer helper worked out which half of a cell you were on before clamping the pointer into the grid, so the strip just left of column 0 reported "right half of column 0" and excluded that column from the selection. The right edge had the same bug mirrored. Clamping first fixes both.
Closes AbysmalBiscuit#31
Claude summary below:
The bug
cell_at_posinterminal_view.rsis the one place a pointer position becomes a gridPointplus aSide. Every selection, mouse-report and link-hover path goes through it.It clamped the column into the grid but derived the side from the raw, unclamped fraction:
Left of the grid
col_fis negative, socol_f.floor()is-1andfraclands in[0.5, 1.0)across the half-cell strip nearest the grid. That yields(Column(0), Side::Right), whichSelectionreads as "anchor after column 0" and drops the leftmost character. Move another half cell further out andfraccrosses below0.5again, so the side flips back toLeftand the selection works. That flip-flop is the "have to be really precise about where I click" symptom in the issue.Past the right edge the same arithmetic fails the other way:
fracis small, so the side isLeftand the last column falls out of the selection.A drag captures the pointer in egui, so
interact_pointer_poskeeps reporting positions outside the widget rect. Out-of-grid positions are the normal case here, not an edge case.The fix
Clamp the position into the grid first, then take both the column and the side from the clamped value:
Anything left of the grid now pins to
(Column(0), Side::Left)and anything right of it to(Column(cols - 1), Side::Right), monotonically, with no flip-flop.This mirrors alacritty rather than inventing behaviour. Its
Mouse::pointuses a saturating subtract before the divide (alacritty/src/event.rs), and itscell_sidetreatsx >= end_of_gridas the right side outright (alacritty/src/input/mod.rs). Same two rules, expressed as one clamp.Tests
Four tests in
terminal_view.rs, using a grid rect whose origin is away from0so a pointer left of the grid produces a genuinely negative local offset:dragging_past_the_left_edge_keeps_the_first_columnsweeps the pointer from a tenth of a cell to forty cells left of the grid and asserts every position anchors at(Column(0), Side::Left). This is the one that fails on the old code, at the-0.1and-0.3offsets.selection_dragged_off_the_left_edge_includes_the_first_characteris the end-to-end check: press insidehello, drag off the left edge, and read backTerm::selection_to_string(). Old code returnsSome("ello"), new codeSome("hello").dragging_past_the_right_edge_keeps_the_last_columnis the mirror.cell_side_splits_each_cell_at_its_midpointpins the ordinary in-grid behaviour so the clamp cannot quietly move the midpoint.cargo nextest run -p alacritreeis green.What is not covered
I verified this through
SelectionandTerm::selection_to_string(), not by dragging in a running window. The mapping is the whole of the bug, so the unit level is where it belongs, but a manual drag is still worth one pass before merge.Written by Claude Opus 5 (1M Context) in Claude Code.