fix(qt): handle pixel-sized fonts when scaling widgets - #7315
fix(qt): handle pixel-sized fonts when scaling widgets#7315thepastaclaw wants to merge 1 commit into
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
✅ Review complete (commit 9c94795) |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis change improves the robustness of font size detection in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, focused Qt fix replacing a fatal assert with a graceful pixel-to-point conversion path for stylesheet-driven pixel-sized fonts. Logic is sound; idempotency via mapWidgetDefaultFontSizes::emplace is preserved. No blocking issues — only two low-impact nitpicks worth surfacing.
Reviewed commit: 9c94795
💬 2 nitpick(s)
| if (font_size <= 0 && font.pixelSize() > 0) { | ||
| font_size = font.pixelSize() * 72.0 / w->logicalDpiY(); | ||
| font.setPointSizeF(font_size); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: setPointSizeF on line 585 is redundant
The local font is either fully replaced by getFont(...) on line 604 (when an entry exists in mapFontUpdates) or has its size overwritten by setPointSizeF(GetScaledFontSize(...)) on line 606. The explicit font.setPointSizeF(font_size) on line 585 has no observable effect on the font ultimately stored in mapWidgetFonts or compared at line 609. Removing it would tighten the code; keeping it is harmless as a defensive normalization.
source: ['claude']
| if (font_size <= 0 && font.pixelSize() > 0) { | ||
| font_size = font.pixelSize() * 72.0 / w->logicalDpiY(); | ||
| font.setPointSizeF(font_size); | ||
| } | ||
| if (font_size <= 0) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: logicalDpiY() result is cached for the lifetime of the widget
w->logicalDpiY() correctly reports the DPI of the widget's current screen, but the derived point-size is then cached in mapWidgetDefaultFontSizes as the widget's 'default font size'. If the widget later migrates to a screen with a different DPI, the cached value will no longer correspond to the original pixel size. The same caching limitation exists in the pre-PR code paths, so this is not a regression — just a corner case worth noting.
source: ['claude']
|
Closing during authored-PR cleanup. This small Qt font-scaling draft is not currently review-priority; can be reopened/recreated if the underlying issue becomes active. |
67d25da test: require fonts for pixel-sized widget regression (PastaClaw) 469abf8 test: cover pixel-sized font conversion (PastaClaw) 443e1d3 qt: convert pixel font sizes in class and QTextEdit paths (PastaClaw) 322f257 qt: fix abort in updateFonts() with pixel-sized fonts (PastaClaw) b6dd65a qt: add effectivePointSize() helper for pixel-sized fonts (PastaClaw) Pull request description: # Pixel-sized font scaling fix ## Issue being fixed or feature implemented `GUIUtil::updateFonts()` assumed every widget font had a positive point size. The recovery-phrase dialog stylesheet uses pixel-sized fonts, for which Qt reports an invalid point size, so an asynchronous global font refresh could abort while the dialog was open. This caused the original macOS report in #7281 and the cross-platform recurrence in #7464. Fixes #7281. Fixes #7464. Supersedes the earlier closed draft #7315, which GitHub would not reopen after the branch was rebased onto current `develop`. ## What was done? - Add a shared helper that preserves valid point sizes and converts valid pixel sizes using the relevant logical DPI. - Use the helper in the widget sweep, class-level font map, and `QTextEdit` styling path. - Skip individual widgets when Qt provides neither a valid point size nor a safely convertible pixel size. - Preserve fractional point sizes and the existing non-compounding per-widget cache behavior. - Add deterministic helper coverage and a focused `GUIUtil::updateFonts()` regression test to the existing `OptionTests` suite without changing test order or adding a translation unit. ## How Has This Been Tested? - A full local build completes successfully. - `QT_QPA_PLATFORM=minimal ./src/qt/test/test_dash-qt updateFontsWithPixelSizedWidget` passes with 7 `OptionTests` passing and the integration body explicitly skipped because the `minimal` plugin cannot load application fonts. The deterministic `effectivePointSize()` coverage runs under this configuration. - On a GUI-capable platform where `AppTests` successfully loads application fonts, `updateFontsWithPixelSizedWidget()` hard-checks that initialization occurred before exercising the production sweep and repeated-update cache behavior. - `git diff --check upstream/develop...HEAD`, lint, and clang-format checks pass. ## Breaking Changes None. ## Checklist - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation, where applicable - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes Top commit has no ACKs. Tree-SHA512: d928c92d4987c999324d87907670f23b4cac0569f45230da2e6a11de4e043a14a3a734334806a80e1921b8e1b067e1409018f4e74b375728568ad6c045c6958e
Pixel-sized font scaling fix
Summary
GUIUtil::updateFonts()runsfont-scaling path
Fixes #7281.
Validation
git diff --check upstream/develop...HEADshipforupstream/develop..tracker-1251to reproduce the original crash locally.