Repository navigation
fix(ui): draw the results graph as a solid line on a zoomed wpm axis - #31
Merged
Merged
Conversation
The graph was a Chart of braille dots on a wpm axis that started at 0, with two labels per axis, one of them reading 9.62. It is now drawn with ratatui's Canvas and quadrant blocks: one continuous line, the wpm axis zoomed to the data on round numbers (never under 10 wpm) with a faint rule at each number, and seconds labels placed under the second they name. A test with one data point draws a short dash instead of nothing. Also trim the screen snapshots to the seven that guard a layout, and check the other five with a plain assertion on what each is about.
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
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 si
306B
ngle 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.
Intent
About the results graph of thokr, a typing test that runs in the terminal:
"also, could you see if there is another charting library that might do ab etter job that what we see here?
#22 (or fix this)"
Context for reading that ask:
9.62.Canvasand quadrant block characters: one solid, continuous line, the wpm axis zoomed to the data on round numbers with a faint rule across at each number, seconds labels placed exactly under the second they name, 0 new crates, about 140 lines in place of today's 38; B, the sameChartwidget used better; C, themalevichcrate; D, leave it as it is. Two extras were listed as separate calls and were not picked: an opt-in thinner line made of octant characters, and plotting per-second speed.The answer to those options:
"A is fine."
Separately, about the 14 screen snapshot tests that pull request 29 added:
"are all these snapshot tests in thokr necessary? I'm okay if the answer is yes but be critical"
After being told that 7 of the 14 earn their place, that 2 are duplicates and that 5 test one thing with a whole screen and could be one-line checks, and asked whether to keep the 7 or drop all:
"keep the 7"
What Changed
src/ui.rsdraws the results graph with ratatui'sCanvasand quadrant block characters in place of theChartwidget and its braille dots: one continuous magenta line, and a short dash when the results hold a single point. No crate is added.secat the left edge. The old two-labels-per-axis layout, thewpmandsecondstitles and labels like6.00are gone.running_fresh,timed_test_in_a_narrow_terminal), and five whole-screen snapshots become plain assertions (pace caret offset, the countdown text, the stats line, the peak's line drawn in every column). The three results-screen snapshots are regenerated for the new graph, and unit tests are added fornice_step,wpm_scale, label placement, and for labels landing on the same cell the canvas draws the line in.Risk Assessment
✅ Low: The change is confined to how the results graph is drawn in src/ui.rs, both round 3 fixes hold against the canvas source and a simulation of the placement code, and the snapshot set is the 7 the intent keeps.
Testing
No baseline command output was handed to this step. cargo is not on PATH, so I built the release binary of the change and of base 61f8fad with an ephemeral
nix shell nixpkgs#cargo nixpkgs#rustcinto a temp directory, then drove the real TUI in a private tmux server with its own HOME (so log.csv was a throwaway). I typed tests with timed keystrokes (a default word test with a typo, a 10 s timed test shaped like issue 22, an 8 s test with two spikes, a 0.4 s test, a paste, a test with no correct key, a 45 s test in a 40 column terminal), resized the results screen from 200x60 down to 1x1 and back, and read every screen cell by cell: line continuity at dot level, label values, rows and columns against where the line itself is drawn, colours, and no crash. All 12 live scenarios passed. The four PNGs are redrawn cell by cell from the colour capture of the tmux panes (no headless terminal screenshot tool was available), and each scenario has a text transcript with the raw screens. The snapshot count was checked with theui::tests only (26 passed), using insta's own list of snapshots read; that is a test run, not a live drive, so the snapshot scenario is marked untested. All temp files, the tmux server and the browser session were removed; the worktree is clean.thokr -w 6) at 80x24: the results graph is one unbroken line of quadrant blocks, with no braille dots80rule is on the row where the line passes 80 wpm at second 3 (the heights the review found one row off)3and6labels are in the column of each spike's tip (80x24: plot columns 18 and 47; also 100x30, 90x24, 64x20)thokr -p hi, 0.4 s): the one point is drawn as a three-cell dash at the moment the test ended, and the seconds row is labelled (0.5, 1)thokr -s 1, one wrong key): empty chart keeps its axes (0, 5, 10 and 0.5, 1), shows 0 wpm, survives resizes, exits 0secstarts at the chart's left edge (column 5) for a result under 100 wpm, at 100 and at six digits, and the plot starts in the same column under 100 and at 100cargo test --locked ui::(26 passed) and comparing the snapshots insta reported as read with the files in src/snapshots (transcript 09-sna…Evidence: Issue 22 shape on the real binary, 80x24 (after)
Evidence: Same keys on base 61f8fad, 80x24 (before)
Evidence: Transcript: word test (screens and checks)
Evidence: Transcript: issue 22 shape at six sizes
Evidence: Transcript: resize sweep
Evidence: Transcript: seconds labels under spikes
Evidence: Transcript: one point and paste
Evidence: Transcript: no correct key
~/.no-mistakes/evidence/01M4R46D67RX0PXCDGXHSM9S6S/07-45s-narrow.txt)Evidence: Transcript: `sec` label column
Evidence: Transcript: snapshots read vs files on disk
~/.no-mistakes/evidence/01M4R46D67RX0PXCDGXHSM9S6S/driver)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (3) ✅
src/ui.rs:494- Four tests were converted frominsta::assert_snapshot!to plain assertions, but their.snapfiles were left in the tree, sosrc/snapshots/still holds 11 files for 7 snapshot tests. The commit message says the snapshots were trimmed to seven. Nothing reads these files any more,cargo testwill not flag them, and one of them still records the old brailleChartscreen (48│wpm,seconds,5.00), which now misdescribes the results screen. Delete all four:src/snapshots/thokr__ui__tests__pace_caret_ahead_of_the_typist.snap(test at src/ui.rs:489-495),src/snapshots/thokr__ui__tests__pace_caret_behind_the_typist.snap(src/ui.rs:498-504),src/snapshots/thokr__ui__tests__timed_test_shows_the_countdown.snap(src/ui.rs:507-513),src/snapshots/thokr__ui__tests__results_screen_after_the_timer_ran_out.snap(src/ui.rs:771-787). The other three removed snapshot tests (running_fresh,timed_test_in_a_narrow_terminal,results_chart_draws_a_peak_with_a_fraction_below_one_half) had their files deleted correctly.🔧 Fix applied.
3 issues (2 warnings, 1 info) still open:
src/ui.rs:494- Four tests were converted frominsta::assert_snapshot!to plain assertions, but their.snapfiles were left in the tree, sosrc/snapshots/still holds 11 files for 7 snapshot tests. The commit message says the snapshots were trimmed to seven. Nothing reads these files any more,cargo testwill not flag them, and one of them still records the old brailleChartscreen (48│wpm,seconds,5.00), which now misdescribes the results screen. Delete all four:src/snapshots/thokr__ui__tests__pace_caret_ahead_of_the_typist.snap(test at src/ui.rs:489-495),src/snapshots/thokr__ui__tests__pace_caret_behind_the_typist.snap(src/ui.rs:498-504),src/snapshots/thokr__ui__tests__timed_test_shows_the_countdown.snap(src/ui.rs:507-513),src/snapshots/thokr__ui__tests__results_screen_after_the_timer_ran_out.snap(src/ui.rs:771-787). The other three removed snapshot tests (running_fresh,timed_test_in_a_narrow_terminal,results_chart_draws_a_peak_with_a_fraction_below_one_half) had their files deleted correctly.src/ui.rs:279- The wpm rules are mislabelled when the plot is 2 rows tall (a 10-row terminal: chart_area height 3, minus the seconds row).wpm_scalefloorsintervalsat 2 (line 165), so the scale can carry 3 ticks, butrow()(line 235) maps them onto 2 rows. Traced with thefinished()data at 80x10: scale is 20/40/60,row(60)= 0,row(40)= round(0.5 * 1) = 1,row(20)= 1.axesdraws ticks in ascending order, so "40" overwrites "20" and the screen reads 60 on the top rule and 40 on the bottom rule while the canvas y-bounds are 20..60 - the final 36 wpm is plotted in a row labelled 40. Nothing fails, and no test covers it:the_results_chart_is_readable_at_every_size_and_lengthonly asserts no panic below 60x20. Fix inaxes: draw the extremes first (hi, then lo, then the middle ticks) and skip any tick whose row already holds a label, so the top and bottom rows always carryhiandlo. Same loop, sibling case: at 1 plot row (9-row terminal) every tick lands on row 0; the same fix leaveshithere. Collisions cannot happen at 3 rows or more (ticks - 1 <= rows - 1 holds from there), and the seconds axis already skips collisions throughnext_free(line 264), so only this axis is affected.src/ui.rs:297- Theseclabel is drawn one column outside the chart area whenever the top wpm tick is under 100. The gutter is sized from the tick labels alone (lines 196-202: 2 chars + 1 = 3), butsecis 3 chars and is placed atarea.x - 4, which ischart.x - 1. On screen it starts at column 4 while the wpm labels, the stats and the legend all keep the 5-column margin (visible as....bbbin the style rows of all three results snapshots). At 100+ wpm the gutter is 4 andsecstarts at column 5, so its position also shifts between results. It only works because the single caller leaves a left margin; for a chart at x = 0 thechecked_sub(4)silently drops the label. Asking because the remedy changes the layout rather than correcting a computation: size the gutter to at least the 3 chars ofsec(the plot moves one column right under 100 wpm), or keep it if the right-aligned overhang is intended.🔧 Fix applied.
5 issues (4 warnings, 1 info) still open:
src/ui.rs:494- Four tests were converted frominsta::assert_snapshot!to plain assertions, but their.snapfiles were left in the tree, sosrc/snapshots/still holds 11 files for 7 snapshot tests. The commit message says the snapshots were trimmed to seven. Nothing reads these files any more,cargo testwill not flag them, and one of them still records the old brailleChartscreen (48│wpm,seconds,5.00), which now misdescribes the results screen. Delete all four:src/snapshots/thokr__ui__tests__pace_caret_ahead_of_the_typist.snap(test at src/ui.rs:489-495),src/snapshots/thokr__ui__tests__pace_caret_behind_the_typist.snap(src/ui.rs:498-504),src/snapshots/thokr__ui__tests__timed_test_shows_the_countdown.snap(src/ui.rs:507-513),src/snapshots/thokr__ui__tests__results_screen_after_the_timer_ran_out.snap(src/ui.rs:771-787). The other three removed snapshot tests (running_fresh,timed_test_in_a_narrow_terminal,results_chart_draws_a_peak_with_a_fraction_below_one_half) had their files deleted correctly.src/ui.rs:279- The wpm rules are mislabelled when the plot is 2 rows tall (a 10-row terminal: chart_area height 3, minus the seconds row).wpm_scalefloorsintervalsat 2 (line 165), so the scale can carry 3 ticks, butrow()(line 235) maps them onto 2 rows. Traced with thefinished()data at 80x10: scale is 20/40/60,row(60)= 0,row(40)= round(0.5 * 1) = 1,row(20)= 1.axesdraws ticks in ascending order, so "40" overwrites "20" and the screen reads 60 on the top rule and 40 on the bottom rule while the canvas y-bounds are 20..60 - the final 36 wpm is plotted in a row labelled 40. Nothing fails, and no test covers it:the_results_chart_is_readable_at_every_size_and_lengthonly asserts no panic below 60x20. Fix inaxes: draw the extremes first (hi, then lo, then the middle ticks) and skip any tick whose row already holds a label, so the top and bottom rows always carryhiandlo. Same loop, sibling case: at 1 plot row (9-row terminal) every tick lands on row 0; the same fix leaveshithere. Collisions cannot happen at 3 rows or more (ticks - 1 <= rows - 1 holds from there), and the seconds axis already skips collisions throughnext_free(line 264), so only this axis is affected.src/ui.rs:297- Theseclabel is drawn one column outside the chart area whenever the top wpm tick is under 100. The gutter is sized from the tick labels alone (lines 196-202: 2 chars + 1 = 3), butsecis 3 chars and is placed atarea.x - 4, which ischart.x - 1. On screen it starts at column 4 while the wpm labels, the stats and the legend all keep the 5-column margin (visible as....bbbin the style rows of all three results snapshots). At 100+ wpm the gutter is 4 andsecstarts at column 5, so its position also shifts between results. It only works because the single caller leaves a left margin; for a chart at x = 0 thechecked_sub(4)silently drops the label. Asking because the remedy changes the layout rather than correcting a computation: size the gutter to at least the 3 chars ofsec(the plot moves one column right under 100 wpm), or keep it if the right-aligned overhang is intended.src/ui.rs:254- The seconds axis is drawn with no labels at all for some test lengths in a narrow terminal.seconds_labelspicksstep = nice_step(span, intervals)withintervals = (width / 9).clamp(2, 10)(line 249), and with 2 intervals the step can be larger than the last second, so thewhile secs <= self.x1loop (line 262) never runs. Traced: terminal 40 columns wide (chart 30, gutter 4, plot 26, so intervals = 2), a test whose last point is at 45 s: x0 = 1, x1 = 45, span = 44, raw = 22, step = 50, first multiple = ceil(1/50)*50 = 50 > 45, so the function returns an empty list and the row shows onlysec. A simulation of the function gives an empty axis for every plot width from 3 to 26 when the last point is between 41.1 and 49.9 s, and again between 401 and 499.9 s. Same cause, sibling path: one point alone (x0 = 0, x1 = 2 * last, line 216) with x1 between 4 and 5 or between 40 and 50, for example a single point at 2.2 s. Nothing fails and no test covers it:seconds_labels_sit_under_their_second_and_do_not_touchasserts a non-empty list at widths 24 and 12, but only for a 30 s test. Fix inseconds_labels: when no multiple ofsteplies in x0..=x1, fall back to the next smaller round step (50 to 20, 5 to 2), for example by raisingintervalsby one and callingnice_stepagain until the first label is at or before x1. With 3 intervals the 45 s case gets step 20 and labels 20 and 40. No committed snapshot changes, since each of them already has a label. Add the 45 s case at width 24 to the existing test. This code is from the original commit, not from a fix round.src/ui.rs:236- Seconds labels and wpm rules are placed with a different rounding than the one the canvas uses for the line, so at many sizes a label sits one cell away from the point it names.col(line 236) roundst * (width - 1)on the cell grid. The canvas (ratatui-widgets 0.3.2,Painter::get_point, with a 2x2 quadrant grid) roundst * (2 * width - 1)on the dot grid and then halves it. The two disagree by one cell at some sizes. Traced on a standard 80 column terminal (plot 66 wide) with an 8 second test, x from 1 to 8: second 3 has t = 2/7,colgives round(18.57) = 19, the canvas gives round(37.43) = 37, which is cell 18. The label3is drawn one column right of the point of second 3, and6one column left of its point (46 against 47). At 80 columns this happens for 18 of the 58 whole-second test lengths from 3 to 60, at 100 columns for 28. The intent asks for "seconds labels placed exactly under the second they name". Same invariant, other axis:row(line 242) roundst * (height - 1)while the canvas roundst * (2 * height - 1)and halves. With the issue 22 data (scale 20 to 100, step 20) in a terminal 27 rows high (plot 19 rows), the80rule is on plot row 5 while the canvas draws 80 wpm on row 4; the row that carries the80label holds 75.1 to 79.5 wpm. The same happens at heights 31, 35 and 39 for that data, and at 32 and 37 for thefinished()data (rules 55 and 40). Fix both in one place each by using the canvas's own mapping:self.area.x + (t * (2 * width - 1)).round() / 2incol, and the same with the height inrow(guard the zero size). Ticks stay on separate rows, since neighbouring ticks are more than 2 dots apart whenever the scale has at most rows - 1 intervals, which the round 2 change towpm_scalealready guarantees. None of the three committed results snapshots change: at their sizes both roundings agree.rowandcolare from the original commit; the round 2 fix changed which ticks are drawn but not where a value lands, and its testevery_wpm_label_is_on_the_rule_of_its_own_valuecompares a label againstplot.rowitself, so it cannot see this.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
thokr -w 6) at 80x24: the results graph is one unbroken line of quadrant blocks, with no braille dots80rule is on the row where the line passes 80 wpm at second 3 (the heights the review found one row off)3and6labels are in the column of each spike's tip (80x24: plot columns 18 and 47; also 100x30, 90x24, 64x20)thokr -p hi, 0.4 s): the one point is drawn as a three-cell dash at the moment the test ended, and the seconds row is labelled (0.5, 1)thokr -s 1, one wrong key): empty chart keeps its axes (0, 5, 10 and 0.5, 1), shows 0 wpm, survives resizes, exits 0secstarts at the chart's left edge (column 5) for a result under 100 wpm, at 100 and at six digits, and the plot starts in the same column under 100 and at 100cargo test --locked ui::(26 passed) and comparing the snapshots insta reported as read with the files in src/snapshots (transcript 09-sna…nix shell nixpkgs#cargo nixpkgs#rustc -c cargo build --release --lockedfor the change (ae434ed) and for base 61f8fad, both into a temp target directorythokr -w 6at 80x24 in tmux: typed the drawn prompt over 5.7 s with one wrong key, checked the results screen, then esc (exit 0); same keys replayed on the base binary for the before picturethokr -s 10 -p <long prompt>with 3,7,10,11,10,5,7,10,10,5 keys per second (peak 98.4 wpm at second 5, final 94 wpm), checked at 80x24, 100x30, 80x27, 80x31, 80x35, 80x39; same keys replayed on the base binaryResized that running results screen through 120x40, 200x60, 60x20, 40x14, 30x14, 30x12, 30x11, 30x10, 30x9, 30x8, 30x6, 20x9, 17x10, 12x5, 5x3, 2x2, 1x1 and back to 80x24 withtmux resize-window, then esc (exit 0)thokr -s 8 -p <long prompt>with 2,2,14,1,1,30,1,1 keys per second (spikes at seconds 3 and 6), checked at 80x24, 100x30, 90x24, 64x20thokr -p hityped in 0.4 s (one data point)thokr -p 'the quick brown fox jumps over the lazy dog'with all 43 keys sent at once (529390 wpm)thokr -s 1 -p 'hello world'with one wrong key, then the timer ran out; resized to 30x10, 12x5, 80x24; esc (exit 0)thokr -p <46 chars>in a 40x24 terminal, one key a second, last key at 45.3 s; results checked at widths 40, 38, 36, 34, 32, 30, 28, 26, 24, 22, 20, 19, 18, 17, 60, 80, 100ANSI colour check of the 80x24 results screens: rules andsecdim dark gray, line magenta, labels plainINSTA_UPDATE=no INSTA_SNAPSHOT_REFERENCES_FILE=refs.txt cargo test --locked ui::(26 passed), then compared insta's list of snapshots read with the files in src/snapshotsgit status --shortafter cleanup: cleanREADME.md:10- The README demo gif still shows the old results graph. I decoded its last frame: a braille dot line, a wpm axis from 0 to 132 with two labels, a seconds axis labelled1and9.00, and the titleswpmandseconds. This change replaces all of that with a solid line, a zoomed wpm axis with faint rules, and seconds labels under their second. Not fixable here: the gif lives in another repository (jrnxf/assets,thokr/demo.gif) and needs a new recording. The same frame also shows a(t)weetlegend entry the app no longer has, so the gif was already out of date before this change. Follow-up: record a new demo and replace the file in the assets repository; the README link can stay.cargois not on PATH in this environment (command -v cargo rustc rustup rustfmtfinds nothing;~/.cargo/binis on PATH but holds no cargo) and the repository supplies no local toolchain path. Blocked:cargo fmt --all -- --checkandcargo clippy --all-features -- -D warnings, the two commands.github/workflows/build.ymldefines. I did not search the machine or fetch a toolchain. Reading the changed code insrc/ui.rsby eye shows rustfmt-shaped layout and no obvious default clippy lint, but that is not a substitute for the run. The CIfmtandlintjobs run the same two commands.✅ **Push** - passed
✅ No issues found.