Repository navigation
components: size menu rows by the theme's control height - #15
Open
xiaoyumuxi wants to merge 1 commit into
Open
xiaoyumuxi wants to merge 1 commit into
xiaoyumuxi wants to merge 1 commit into
Conversation
Menu item rows were always 32px, so a compact theme (control height 26) packed its buttons and inputs densely but left its context, dropdown and menubar menus as loose as the default one. A row now takes the theme's control height: 32 at the default density, as before, 26 at the compact one and 40 at the large one. A RowHeight option still wins, and a theme without a control height keeps 32. Labels and separators keep their size. Select and Combobox lists keep their own 32px rows.
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 this changes
Menu item rows take the theme's control height instead of a fixed 32px:
components/dropdown_menu.go:menuThemeRowHeight(theme)is the row height of a menu surface. It returnstheme.Metrics.ControlHeightand falls back to the old 32 (menuRowHeight) for a theme without one.menuSurface.itemHeight()andmenuItemHeightboth use it when no row height was set, so the rows a menu paints and the heightui.MenuItemsreserves for them (ContextMenuItemHeight,DropdownMenuItemHeight) stay the same.menuSurfacefollows the theme:ContextMenu,DropdownMenuandMenubarContent, and soui.ContextMenuandui.Menutoo.DensityCompact, 40 atDensityLarge.ContextMenuOptions.RowHeightstill wins when it is set.RowHeightdoc already says.NextItemBounds,RowHeightandContextMenuItemHeightnow say what zero means.context_menuanddropdown_menuinstaller templates are regenerated withgo run ./tools/componentgen.SelectandComboboxkeep their own 32px option rows. They are separate list components, not menu surfaces, and are out of scope here.Why
An application that sets
Metrics.WithDensity(DensityCompact)gets 26px buttons, inputs and tabs, but its context menus kept 32px rows. A right-click menu was then the loosest surface in an otherwise compact window, noticeably sparser than the platform menus it sits next to. The only way around it was passingRowHeighton every menu, andui.ContextMenuhas no way to pass it.How it was verified
test -z "$(gofmt -l $(git ls-files '*.go'))"go vet ./...go test ./... -count=1:componentsanduipass. Two kinds of failures incmd/vivaalso happen on the base commit without this change:TestGeneratedRecipesAreCurrent/TestInstalledRecipesMatchReferenceSourcereport a stalecolor_swatchtemplate from c1b073c. That template is left alone here.TestPackage*tests fail on this machine because the toolchain builds for macOS 13.0 while the fixture saysmin_system12.0.New test
TestMenuRowsFollowTheThemeControlHeight:ContextMenuItemHeightreserves 26.RowHeight: 24: 24px rows.ControlHeight: 0: 32px rows.DropdownMenu: 26px rows.The compact case fails without the change (
rows = {... Height:32} ..., want 26 high).Benchmarks, run interleaved (before, after, before, after, ...) 8 times each with
-benchmemon an Apple M-series laptop. The machine was not idle, so treat differences of a few percent as noise:BenchmarkMenuOpen(ui)BenchmarkDropdownMenuWithBackdrop(components)BenchmarkContextMenuDescription(components)The ranges overlap and nothing allocates. The change only swaps a constant for one field read.
Visual evidence
ui.ContextMenuon a page whose theme usesgfx.LightTheme()withMetrics.WithDensity(gfx.DensityCompact).TestMenuRowsFollowTheThemeControlHeight.Where the boundary sits
Framework capability. The theme already carries the density, and the menus are the one surface that ignored it.
Cost
A program that doesn't change the density sees the same 32px rows. Each row reads one more metrics field.