Repository navigation
Draw thin theme-coloured scroll bars and fit the shortcut list - #44
Merged
Merged
Conversation
The shell smoke now gives the rows 5,000 entries and checks that the list keeps a 10 px strip for its vertical bar, not the system bar's width, that a thin bar covers that strip, that a press below its thumb pages down once, that dragging the thumb to the end shows the last page, and that the wheel over it scrolls the rows. It then opens the settings panel and checks that the shortcut list has no horizontal bar, is no wider than its client area, and that every action fits whole in its row beside its keys. On main the first check fails with "the rows keep 17 px for their vertical scroll bar, not a 10 px strip", and the shortcut check, run on its own, with "the shortcut list has a horizontal scroll bar". scroll.rs holds the bar geometry (thumb size and place, the position a dragged thumb stands for, which way a track press pages) with unit tests. Nothing uses it yet.
The rows and the settings panel's shortcut list keep their own scroll bars and scroll state, so the wheel, the keyboard, touch panning and UI Automation work as before. A subclass on each list answers WM_NCCALCSIZE with a client area that leaves a 10 px strip for each bar it shows instead of the system bar's width. Windows still draws its bars from the client edge outwards, which is now past the window's edge. Two small sibling windows sit over the strips and draw the bars without arrow buttons: the track in the surface colour, a 6 px rounded thumb in the disabled text colour that grows to 8 px in the secondary text colour under the mouse or while dragged. A thumb drag scrolls the list with LVM_SCROLL, a press on the track pages with the list's own SB_PAGEUP and SB_PAGEDOWN and repeats while held, and the wheel over a bar goes to the list. Ctrl+wheel over the rows' bar zooms as it does over the rows. In high contrast the overlay is off and the system bars come back in system colours. The theme decides this each time it is applied. The shortcut list's rows are as wide as the list less the room a vertical bar takes, so it never needs a horizontal bar, and an action that does not fit beside its keys wraps onto a second line. Every row gets the height the tallest wrapped action needs.
The scroll bar smoke now checks that the rows clip their siblings, so they never paint over the bars lying on them, and that Ctrl+wheel over the rows' bar zooms one step, as it does over the rows.
The smoke now fails when the rows are taller than the tallest wrapped action needs, and runs the shortcut checks again after rebuilding the theme at 150% DPI in the order a WM_DPICHANGED refresh uses. Without the fit in the panel's layout the rows stay 150 px high where 74 px is needed.
During a 10M-row Page Down run the list got LVM_GETHEADER and the undocumented 0x10C1 about 35 and 32 times per page, and every one of them updated the bars. The new unit test fails on the branch because every message in the LVM_ range counts as a change.
List queries such as LVM_GETHEADER, and the undocumented 0x10C1 the list sends itself while it paints, no longer update the bars. On a 10M-row file Page Down went from 21.6 to 20.4 ms (main: 19.4 ms).
A list with an empty common controls subclass and nothing else took 20.6 ms per Page Down on a 10M-row file, against 19.6 ms on main and 19.7 ms with a plain window procedure swap. The smoke now fails while the bars use SetWindowSubclass on the rows.
SetWindowSubclass sends every list message through common controls' dispatcher, and the list gets dozens per paint. A plain GWLP_WNDPROC swap passes them on with CallWindowProcW. The list's procedure is put back on WM_NCDESTROY.
No message follows WM_NCDESTROY, so putting the list's procedure and user data back did nothing.
Merged
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 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.
What changes
The rows (both bars) and the settings shortcut list get thin scroll bars in the theme's colours, without arrow buttons, in LeanMark's look. A thumb is 6 px at 96 DPI and 8 px under the mouse, in a 10 px strip, with rounded ends and a minimum length of 32 px. Track, thumb and hot thumb use the theme's surface, disabled-text and secondary-text colours. The part of the vertical strip beside the column titles takes the titles' colour and border.
The shortcut list now fits its width. Each action wraps onto as many lines as it needs beside its keys, every row gets the height of the tallest wrapped action, and the list has no horizontal bar.
High contrast keeps the system scroll bars and their system colours.
How it works in Win32
The system scroll bar has no setting for its colour, width or arrows, and the list sets its own bars again whenever its rows or size change, so hiding them from outside does not last. A separate scroll bar control would need the list's scroll state copied both ways. So the list keeps its own bars and its own scroll state, and only the room it gives them changes:
native/scrollbar.rsthat passes every message on withCallWindowProcW. It answersWM_NCCALCSIZEby taking a 10 DIP strip from the client area for each bar the list shows, instead ofSM_CXVSCROLL(17 px here). Windows still lays the system bars out at their own width from the client edge, so they fall past the window's edge and are clipped away.LeanRowsScrollBar(WS_EX_NOACTIVATE) sit over the strips. The list getsWS_CLIPSIBLINGSso its paints do not cover them. They read the list'sGetScrollInfoand draw the track and thumb (geometry in the newscroll.rs, which has its own unit tests).WM_SETREDRAW, and the list messages that change its rows, columns or scroll position (LVM_SETITEMCOUNT,LVM_SCROLL,LVM_ENSUREVISIBLE, column changes,LVM_DELETEALLITEMS,LVM_SETIMAGELIST,LVM_SETEXTENDEDLISTVIEWSTYLE).LVM_SCROLLfor the rows between where the thumb is and where it should be; a press on the track sendsSB_PAGEUPorSB_PAGEDOWN, repeated while held (400 ms, then every 50 ms) until the thumb reaches the pointer; the wheel and Ctrl+wheel over a bar go to the list and to the zoom as before.set_enabled(false), which hides the bars and refits the frame at the system width.The listview decides whether it needs a horizontal bar as if its vertical bar were
SM_CXVSCROLLwide, so the shortcut rows are madewidth - max(SM_CXVSCROLL, strip)wide. Making them only the strip narrower still left a horizontal bar.Two costs found on the way
The first version of the branch made Page Down on the 10M-row file slower: 21.3 to 21.6 ms against 19.3 to 20.3 ms on main. Two causes, each with a failing check before its fix:
LVM_range updated the bars. A counting build showed the list getsLVM_GETHEADERand an undocumented0x10C1, which it sends itself while it paints, dozens of times per page. Only messages that change the list update the bars now (ada7a0b test, 3021d2b fix). Page Down went to 20.3 to 20.4 ms.SetWindowSubclassitself. A build with an empty common controls subclass on the list and nothing else took 20.55 to 20.71 ms per Page Down; main took 19.50 to 19.97 ms and a plainGWLP_WNDPROCswap 19.65 to 20.37 ms (3 alternating runs each). The bars now swap the window procedure (6ed0d05 check, 47f4909 fix).Tests
Commit 64b9f5c adds the checks before the change. On main the shell smoke fails with:
the rows keep 17 px for their vertical scroll bar, not a 10 px stripthe shortcut list has a horizontal scroll barThe smoke sets 5,000 rows and checks that the strip is 10 DIP, that a bar covers it, that a press near the bottom of the track pages the rows to the next page, that dragging the thumb to the end shows the last page, and that the wheel over the bar scrolls. With the overlay off it checks the system width is back and no bar shows. The shortcut check opens the settings panel and checks there is no horizontal bar, that the rows are no wider than the list, and that every action, wrapped, fits beside its keys in the row height.
Later commits add checks for
WS_CLIPSIBLINGS, Ctrl+wheel over the bar, the rows' height not being more than the tallest action needs, the shortcut checks again after the theme is rebuilt at 150% DPI in the order aWM_DPICHANGEDrefresh uses, list queries not updating the bars, and the list's window procedure being the bars' own. The last two failed on the commit before their fix:list_queries_do_not_move_the_barspanicked with0x101f moved the barsthe rows' messages go through the common controls subclass dispatcherMutation table
Each row is one change to the final branch, then
cargo test -p leanrows-win32 --liband the release build's--smoke-testand--document-smoke-test(both fixtures), as CI runs them. The first failing line is quoted. Each mutation was reverted afterwards and the suite is green again (273 workspace tests, clippy with-D warnings, smokes at saved zoom 50, 100 and 300%).narrow_frameinWM_NCCALCSIZEthe rows keep 17 px for their vertical scroll bar, not a 10 px stripSWP_HIDEWINDOW)no thin bar covers the strip beside the rowsdragging the thumb to the end showed row 0 first, not row 4973a press below the thumb showed row 0 first, not row 27the wheel over the thin bar did not scroll the rowswith the thin bars off the rows keep 10 px, not the system bar's 17 px, and a thin bar shows: falsehigh_contrast_keeps_the_system_scroll_bars(native.rs)width - strip)the shortcut list has a horizontal scroll barafter a change to 150% DPI, the shortcut rows are 150 px high, but the tallest action needs 74 pxDT_WORDBREAK)"Sort by it, again to reverse" needs 99 x 46 px beside "Click a column header" in a row with 110 x 32 pxWS_CLIPSIBLINGSto the listthe rows paint over their thin barsCtrl+wheel over the thin bar left the rows at 100%, not 110%ten_million_rows_keep_a_thumb_you_can_catch(scroll.rs:158); smoke:dragging the thumb to the end showed row 27 first, not row 4973LVM_message updates the bars againlist_queries_do_not_move_the_barsLVM_SETITEMCOUNTdoes not update the barslist_changes_move_the_barsS9 was green in the first mutation run. On opening, the presentation pass fits the rows and the width never changes afterwards at one DPI. On a DPI change the presentation pass runs before the new layout, so only the layout's fit sees the new width. e069c18 pins that.
The window procedure swap is pinned by the smoke check that failed on 6ed0d05, not by a mutation row: undoing it means writing the old subclass back.
Not pinned: the colours and the hot-thumb width are paint-only. Screenshots of the final build in Light, Dark, Dim and Dusk, at 100% and 300% and with the settings panel open, were checked by hand.
Measurements
Same machine and harness as #43. main is 95146aa built fresh, the branch is 47f4909 (the two later commits drop two lines that run on window destruction and add test cases). 1400 x 900 window, saved zoom set to 100% for the runs and put back.
10M-row file (
shuffled-10000000.csv, 2 columns), 300 Page Down presses then 300 wheel notches, each flushed, 3 alternating runs; median of the run medians with the range:Zoom on
six-2mb.csv(72 steps, 3 runs, the #43 harness):Start,
tools/Compare-LeanRowsBuilds.ps1with 7 starts per build, run three times (medians):Page Down is 0.4 ms (2%) slower and the ranges do not overlap, so this is a real cost of the overlay that is left. The wheel is 0.6 ms faster. Opening is 85 ms (2%) slower in the medians, with overlapping ranges. Start is 4 to 12 ms slower without a file and 6 to 42 ms slower with the large file; main's own range in those runs was 336 to 413 ms. The executable grows by 23,040 bytes, including the smoke checks, which ship in the executable because CI runs them against the release build.
The benchmark job on this PR (run 42272417168, one
windows-2022runner, main 95146aa against 5b64bc9) gave start to input idle 43 ms (42 to 98) against 44 ms (43 to 50) without a file and 65 ms (65 to 87) against 67 ms (67 to 70) with its large file; peak working set 14.9 and 16.3 MiB on both.A thumb drag was not timed. It sends one
LVM_SCROLLper mouse move.Back-compat and rollback
Not touched
header.rs:169titles do not zoom their text;restore_tabsets widths with redraw on; default widths are not DPI scaled (all named in Make a zoom step change only the rows and paint once #43).SetWindowSubclass. Their message rates are low; they were not measured.fit_rowsnever finds the height equal. It costs one image list per layout.Known gaps
set_enabled(false)and the theme rule, not by turning high contrast on: that needs a system setting changed.