Skip to content

UI Spacing and Headers Refactor - #75

Merged
283375 merged 8 commits into
masterfrom
refactor/ui-spacing-20261005
Oct 7, 2026
Merged

283375 merged 8 commits into
masterfrom
refactor/ui-spacing-20261005

Conversation

@283375

@283375 283375 commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator
  • Add centralized Spacing object and MaterialTheme.spacing extension
  • Split ListGroupHeader and SettingsGroupHeader for proper usages
  • Rename *PreferencesWidget to *(Settings)Item

283375 added 6 commits October 6, 2026 00:23
The new `Spacing` object defines a 4dp-grid scale plus semantic
aliases, exposed as MaterialTheme.spacing, replacing all legacy
dimensionResource calls and raw spacing dps. dimens.xml is also
removed. Icon sizes, corner radii, and stroke widths are out of
scope and stay untouched.
@283375

283375 commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

/opencode Review this PR

@opencode-agent

opencode-agent Bot commented Oct 7, 2026

Copy link
Copy Markdown

I reviewed PR #75 (75 files, +937/−848) and verified it builds.

Verification

  • ./gradlew :app:compileStableDebugKotlin ✅
  • ./gradlew :desktopApp:compileKotlin ✅
  • ./gradlew :app:lintKotlin :shared:lintKotlin :desktopApp:lintKotlin ✅
  • No stray @dimen/, R.dimen., *PreferencesWidget, SelectPreferencesOption, or ListGroupHeaderDefaults references remain.

Overall this is a clean, mechanical migration to a token scale plus a rename/split of header components. No compile-blocking issues found. A few things worth addressing:

Findings

1. listTopInset undercounts the floating card offset — UtilitiesChartRecommendScreen.kt:254,365
ParametersCardTopPadding = MaterialTheme.spacing.xs (4dp) is used both as the card row's internal padding (line 283) and as the card's top offset in listTopInset. But the card's actual outer top offset is MaterialTheme.spacing.sm (8dp, line 466). So the gap between the card bottom and the list/scrim is 68 − (8 + 48) = 12dp, not the documented lg (16dp). Use spacing.sm for that term (or derive it from the same token as the card modifier) so the inset matches the comment.

2. dialogPadding token is defined but never used — Spacing.kt:22
BasicAlertDialogSurface.kt:32 hardcodes MaterialTheme.spacing.xl instead. Either switch it to spacing.dialogPadding or drop the token.

3. Corner radius uses a spacing token, contradicting the scale's contract — SettingsUnstableAlertCard.kt:52,54
The Spacing KDoc explicitly says "corner radii … are not part of this scale," yet CutCornerShape(topEnd = MaterialTheme.spacing.lg, bottomStart = MaterialTheme.spacing.lg). Values are unchanged (16dp), but this either needs a shape token or a doc adjustment.

4. val MaterialTheme.spacing is not actually themable — Spacing.kt:25
It ignores the composition and always returns the singleton Spacing, and it isn't @Composable (which is why it can be read from a top-level val). This reads like a theme-provided value but can't vary by theme/density. Consider exposing Spacing directly, or making it a real CompositionLocal/theme field if that flexibility is ever wanted.

Intentional visual changes to confirm

These are lossy relative to the old dimens/raw dps and change pixels (all arguably correct for a 4dp grid, but worth a design sign-off):

  • AppIconLabelButton.kt:37 icon↔text gap 10dp → 8dp
  • ImagePreviewDialog.kt:88 bottom padding 28dp → 24dp
  • OcrQueueStagingSheet.kt:91 picker vertical padding 36dp → 32dp
  • ListGroupHeader restyle (primary/FontWeight.Light → onSurfaceVariant/SemiBold) also changes the 3 remaining ListGroupHeader callers (UtilitiesChartRecommendScreen, DatabaseDeduplicatorGroupListItem).
  • DatabaseManageScreen drops all HorizontalDividers in favor of SettingsGroupHeader's new 24dp top padding.

Verdict: solid refactor; address #1–#3 before merge, #4 and the list above are design calls.

New%20session%20-%202026-10-07T00%3A34%3A34.845Z
opencode session  |  github run

@283375

283375 commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

/opencode Review the new 2 commits (after last review)

@opencode-agent

opencode-agent Bot commented Oct 7, 2026

Copy link
Copy Markdown

Reviewed the 2 new commits since the last review (41cc49c fixup, 298a3bc add comments). They directly target the previous findings and compile/lint cleanly.

Verification

  • ./gradlew :app:compileStableDebugKotlin :desktopApp:compileKotlin ✅
  • ./gradlew :app:lintKotlin :shared:lintKotlin :desktopApp:lintKotlin ✅

Prior findings addressed

  1. listTopInset offset (was 应用异常重绘界面 #1) — Fixed correctly. UtilitiesChartRecommendScreen.kt:255 introduces ParametersCardOuterVerticalPadding = MaterialTheme.spacing.sm and reuses it for both the card modifier (:469) and listTopInset (:366-369). The two can no longer drift; inset is now 48 + 8 + 16 = 72dp as documented.
  2. Unused dialogPadding (was ScoreEditor 复用问题 #2) — Fixed. BasicAlertDialogSurface.kt:31 now uses MaterialTheme.spacing.dialogPadding; the now-unused import ...unit.dp was removed.
  3. Corner radius via spacing token (was OCR 暂存 #3) — Addressed by reverting to literal dp (SettingsUnstableAlertCard.kt:52,54). Behavior is unchanged (both were 16dp) and it now respects the Spacing KDoc contract. See nit below.
  4. MaterialTheme.spacing not themable (was B30 列表 #4) — Acknowledged via KDoc (Spacing.kt:25-27), explicitly documenting it as a placeholder. Reasonable.

Nit (non-blocking)

SettingsUnstableAlertCard.kt:51-54 now uses a magic 16.dp (alongside 0.dp) for the cut corners. Since the fix intentionally moved radii out of the Spacing scale, consider a named constant or a Shapes-level token so this value isn't duplicated and undocumented. Purely cosmetic; no behavior change.

Verdict: the two commits resolve the actionable items from the previous review with no regressions. Only the magic-number nit remains, which is a design/style call.

New%20session%20-%202026-10-07T04%3A11%3A37.062Z
opencode session  |  github run

@283375
283375 marked this pull request as ready for review October 7, 2026 06:48
@283375
283375 merged commit f2919c4 into master Oct 7, 2026
4 checks passed
@283375
283375 deleted the refactor/ui-spacing-20261005 branch October 7, 2026 06:49
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