Skip to content

components: size menu rows by the theme's control height - #15

Open
xiaoyumuxi wants to merge 1 commit into
mainfrom
fix/menu-row-height-density
Open

xiaoyumuxi wants to merge 1 commit into
mainfrom
fix/menu-row-height-density

Conversation

@xiaoyumuxi

Copy link
Copy Markdown

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 returns theme.Metrics.ControlHeight and falls back to the old 32 (menuRowHeight) for a theme without one. menuSurface.itemHeight() and menuItemHeight both use it when no row height was set, so the rows a menu paints and the height ui.MenuItems reserves for them (ContextMenuItemHeight, DropdownMenuItemHeight) stay the same.
  • Every menu built on menuSurface follows the theme: ContextMenu, DropdownMenu and MenubarContent, and so ui.ContextMenu and ui.Menu too.
  • Heights by density: 32 at the default density (unchanged), 26 at DensityCompact, 40 at DensityLarge.
  • ContextMenuOptions.RowHeight still wins when it is set.
  • Labels (28) and separators (9) keep their size, as the RowHeight doc already says.
  • Doc comments on NextItemBounds, RowHeight and ContextMenuItemHeight now say what zero means.
  • The context_menu and dropdown_menu installer templates are regenerated with go run ./tools/componentgen.

Select and Combobox keep 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 passing RowHeight on every menu, and ui.ContextMenu has no way to pass it.

How it was verified

  • test -z "$(gofmt -l $(git ls-files '*.go'))"
  • go vet ./...
  • go test ./... -count=1: components and ui pass. Two kinds of failures in cmd/viva also happen on the base commit without this change:
    • TestGeneratedRecipesAreCurrent / TestInstalledRecipesMatchReferenceSource report a stale color_swatch template from c1b073c. That template is left alone here.
    • The TestPackage* tests fail on this machine because the toolchain builds for macOS 13.0 while the fixture says min_system 12.0.
  • [N/A] platform vet: no backend is touched
  • benchmarks before and after (below)
  • [N/A] stripped binary size: nothing new links
  • a real window: verified in tests only, see below

New test TestMenuRowsFollowTheThemeControlHeight:

  • Default theme: 32px rows.
  • Compact theme: 26px rows, and ContextMenuItemHeight reserves 26.
  • Compact theme with RowHeight: 24: 24px rows.
  • A theme with ControlHeight: 0: 32px rows.
  • A compact DropdownMenu: 26px rows.
  • Labels stay 28px whatever the rows are.

The compact case fails without the change (rows = {... Height:32} ..., want 26 high).

Benchmarks, run interleaved (before, after, before, after, ...) 8 times each with -benchmem on an Apple M-series laptop. The machine was not idle, so treat differences of a few percent as noise:

Benchmark Before mean (min–max) After mean (min–max) Allocs
BenchmarkMenuOpen (ui) 2545 ns (2439–2686) 2611 ns (2524–2833) 0 → 0
BenchmarkDropdownMenuWithBackdrop (components) 535 ns (523–547) 552 ns (532–596) 0 → 0
BenchmarkContextMenuDescription (components) 416 ns (394–436) 423 ns (396–439) 0 → 0

The ranges overlap and nothing allocates. The change only swaps a constant for one field read.

Visual evidence

  • Changed: the rows of a context or dropdown menu in a compact or large theme. The default density does not change.
  • Reach it by: any ui.ContextMenu on a page whose theme uses gfx.LightTheme() with Metrics.WithDensity(gfx.DensityCompact).
  • Evidence: the test above measures the rows; no screenshot attached.
  • Protected by: 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant