fix(qt): handle pixel-sized fonts when scaling widgets - #7465
Conversation
|
The The failing test is Filed #7466 with the failure logs and reproduction evidence. No commit, push, rebase, or CI retrigger is appropriate for this PR. |
|
🕓 Ready for review — 3 ahead in queue (commit 67d25da) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The pixel-sized font fix correctly converts pixels to points using the widget's logical DPI, handles invalid sizes safely, and has focused Qt regression coverage. The new Dash-specific test files are not yet registered for Dash cppcheck analysis, and the conversion branch contains one harmless redundant assignment.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:38: Register the new font tests for Dash-specific cppcheck
The newly added `src/qt/test/fonttests.cpp` and `src/qt/test/fonttests.h` files have no upstream Bitcoin Core counterpart, but no pattern in this registry matches them. `test/lint/lint-cppcheck-dash.py` constructs its complete input set from these patterns, so both files are currently omitted from Dash-specific static analysis. Add the test files alongside the existing `guiutil_font.*` entry.
aa1fcec to
aa68ce3
Compare
|
Addressed the current review findings in the existing clean two-commit stack:
Validation: |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Both carried-forward findings are resolved at the exact current head: the Dash-specific font tests are registered for cppcheck, and the redundant point-size assignment has been removed. Independent review of the rewritten PR stack and the incremental delta found no new in-scope issues; the pixel-to-point conversion handles invalid sizes safely and has focused Qt regression coverage.
GitHub does not allow PastaClaw to approve their own PR, so the canonical APPROVE verdict is transported as a COMMENT review.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
WalkthroughAdds Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GUIUtil_updateFonts
participant QWidget
participant QFont
participant QScreen
GUIUtil_updateFonts->>QWidget: Read font and logicalDpiY
GUIUtil_updateFonts->>QFont: Read pointSizeF or pixelSize
GUIUtil_updateFonts->>QScreen: Read primary screen DPI
GUIUtil_updateFonts->>QWidget: Apply scaled point size
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/qt/test/fonttests.cpp`:
- Around line 16-23: Update the macOS "minimal" platform guard in FontTests to
call QSKIP(...) instead of returning normally, preserving the existing
explanatory message so Qt Test records the unsupported configuration as skipped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fd0dca7-2575-4bc4-bbce-8883da3fc646
📒 Files selected for processing (6)
src/Makefile.qttest.includesrc/qt/guiutil_font.cppsrc/qt/test/fonttests.cppsrc/qt/test/fonttests.hsrc/qt/test/test_main.cpptest/util/data/non-backported.txt
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/qt/test/uritests.cpp`:
- Around line 127-145: In the test covering GUIUtil::setApplicationFont() and
GUIUtil::updateFonts(), save the existing QApplication font before mutating
global font state and restore it with an RAII guard that remains active through
the test. Ensure restoration occurs on all exit paths, including QSKIP and
assertion failures, so later AppTests and OptionTests see the original
application font.
- Around line 141-148: Update the font assertions in the URI test around
GUIUtil::updateFonts() to preserve label.font().pixelSize() before the update,
then verify pointSizeF() against pixelSize * 72.0 / label.font().
화면?logicalDpiY() with a small floating-point tolerance. Replace the broad
positive-and-less-than-100 checks while retaining the existing pre-update
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e1fcbee-ea80-46c7-9438-614769ec5bac
📒 Files selected for processing (3)
src/qt/test/test_main.cppsrc/qt/test/uritests.cppsrc/qt/test/uritests.h
✅ Action performedReviews resumed. |
✅ Action performedReview finished.
|
✅ Action performedReviews resumed. |
QFont stores either a point size or a pixel size, never both, and reports -1 for whichever was not set. A font sized in pixels -- which is what a stylesheet rule such as `font-size: 17px` produces, since Qt's CSS parser calls setPixelSize() with no unit conversion -- therefore yields -1 from both pointSize() and pointSizeF(). Code that reads a point size off an arbitrary widget has to convert rather than assume, and several call sites in this file currently do not. Qt offers no usable public API for this: - QFont(font, paintDevice) only stamps the target device's DPI onto the font; it leaves the requested size untouched, so a pixel font stays a pixel font. - QFontInfo does resolve the size, and is Qt's documented answer to this problem (QTBUG-3555), but it forces a font-engine load per query -- unacceptable in a loop over every widget in the application -- and degrades to the same useless sentinel under the headless "minimal" platform plugin, where the box-engine fallback reports a point size of -0.72. That -0.72 also shows why the guards here compare against 0 instead of -1: the sentinel's exact value is not contractual. The conversion mirrors Qt's own, in QFontDatabase::load(), including its guard against a non-positive DPI. Taking the DPI as an explicit int rather than reading it from a QWidget keeps the helper usable for the application-wide class fonts, which have no associated widget, and testable without constructing one. No behavior change; the helper is unused until the following commits. Co-Authored-By: Claude <noreply@anthropic.com>
updateFonts() sweeps every widget in the application and asserted that each one reported a positive point size. Widgets sized in pixels report -1, so the assert fired and aborted the process. assert() is live in release builds here -- src/util/check.h refuses to compile with NDEBUG -- so this was a crash in shipped binaries, not a debug-only check. The trigger is in the shipped stylesheet: general.css sizes MnemonicVerificationDialog's warningLabel and instructionLabel with `font-size: 17px` / `14px`. QDialog is in vecIgnoreClasses but QLabel is not, so those labels are swept. Qt writes the stylesheet's font into the widget's own font, which is what the sweep reads. (QGroupBox's `font-size: 16px` rule looks like a second trigger but is not: QGroupBox is ignored, and a pixel size on it does not propagate to its children.) The assert was never a designed invariant. 634ccc8 added it while a working defaultFontSize fallback still handled this case, and c6bf0d3 removed that fallback the same day, leaving the assert as the sole handler -- a regression from dashpay#3772. Skip the widget when no size can be derived rather than substituting a default, which would resize a widget the stylesheet deliberately sized. The counter is moved below the check so it keeps reflecting the widgets actually considered updatable. Seeding the per-widget cache from pointSizeF() rather than pointSize() also completes cdd7b37 ("refactor: consolidate font attributes to struct"), which widened this map and its consumer from int to double but left the producer truncating, silently discarding the fractional part on every pass. Fixes dashpay#7281 Fixes dashpay#7464 Co-Authored-By: Claude <noreply@anthropic.com>
Two more paths read a point size straight off a font that may not have one. mapClassFontUpdates used -1 as its "not captured yet" marker, which collides with the value a pixel-sized font reports. If an application-wide class font were ever set in pixels, the capture branch would store -1, re-fire on every subsequent pass, and feed a negative size into GetScaledFontSize(). Storing std::optional removes the collision, and the map's value type becomes double so the captured size is no longer truncated -- the same producer/consumer mismatch the previous commit fixed for widgets. These fonts belong to no widget, so the primary screen's DPI drives the conversion; if it is unavailable the entry is left uncaptured and retried next pass rather than poisoned. This one is defensive, not a live fix: class-scoped stylesheet rules do not reach qApp->font(class), so nothing in the shipped CSS can currently reach it. It takes an explicit qApp->setFont(pixel_font, class) call. setStyledHtml() was already guarded and so never crashed, but it silently fell back to DEFAULT_FONT_SIZE for a pixel-sized QTextEdit instead of honouring the size the widget was given. It now converts, keeping the existing fallback for the genuinely-unusable case. Co-Authored-By: Claude <noreply@anthropic.com>
effectivePointSize() takes the DPI as a parameter, so its arithmetic can be pinned exactly without a widget, a screen, or a particular host: point sizes pass through unchanged, pixel sizes convert at 72, 96 and 144 DPI, and a non-positive DPI reports failure rather than producing a negative size. Pinning the 72 DPI case matters because points and pixels coincide there -- a test that only ran at whatever DPI the host reports could assert 17 == 17 and look like it was checking the conversion when it was not. The integration test covers the smallest meaningful slice of the production path: a pixel-sized widget survives an updateFonts() sweep, which aborted the process before this series, and a second sweep does not compound the size, since the per-widget cache is what keeps repeated passes stable. updateFonts() is a no-op until loadFonts() has run, and loadFonts() is process-global, non-idempotent state owned by AppTests. Rather than calling it here -- which would make this test's behavior depend on suite execution order and could corrupt AppTests -- it is skipped when fonts are not loaded. It therefore skips under the "minimal" platform plugin the test runner defaults to, whose plugin cannot load application fonts at all. The helper tests carry the coverage that always runs. These live in OptionTests rather than a new translation unit, and the existing test registration order in test_main.cpp is left untouched. Co-Authored-By: Claude <noreply@anthropic.com>
ced4737 to
469abf8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/qt/guiutil_font.cpp`:
- Around line 772-776: Update both font-cache sites in src/qt/guiutil_font.cpp:
lines 772-776 near mapWidgetDefaultFontSizes and lines 807-820 in the companion
cache path. Store the original pixel-sized value for Qt pixel-font entries,
rather than the first DPI-converted point size, and on each refresh recompute
the point size using the current relevant DPI; preserve existing behavior for
non-pixel font entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cfb2d44-6937-4c78-96ed-503cf90be4e2
📒 Files selected for processing (4)
src/qt/guiutil.hsrc/qt/guiutil_font.cppsrc/qt/test/optiontests.cppsrc/qt/test/optiontests.h
| // Insert/Get the default font size of the widget. Seeded once per widget, so a | ||
| // later stylesheet re-apply cannot compound the scaling. Note this freezes a | ||
| // pixel-derived size at the DPI first seen; moving the window to a screen with a | ||
| // different DPI will not re-honour the stylesheet's pixel intent. | ||
| auto itDefault = mapWidgetDefaultFontSizes.emplace(w, *base_size); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file and relevant symbols =="
git ls-files | grep -F 'src/qt/guiutil_font.cpp' || true
wc -l src/qt/guiutil_font.cpp
fd -i 'guiutil_font.cpp|guiutil_font.h' .
echo "== outline around relevant functions =="
ast-grep outline src/qt/guiutil_font.cpp --match ApplyStyleSheet --view expanded || true
ast-grep outline src/qt/guiutil_font.cpp --match internal --view expanded || true
echo "== relevant slices =="
sed -n '720,860p' src/qt/guiutil_font.cpp
echo "== definitions map variables =="
rg -n "mapWidgetDefaultFontSizes|mapClassFontUpdates|effectivePointSize|GetScaledFontSize|logicalDpiY|screenDpi|logicalDotsPerInch" src/qt/guiutil_font.cpp src/qt -g '*.cpp' -g '*.h' | head -200Repository: dashpay/dash
Length of output: 1910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect surrounding code in full relevant section =="
sed -n '1,260p' src/qt/guiutil_font.cpp
echo "== next sections =="
sed -n '560,720p' src/qt/guiutil_font.cpp
sed -n '860,970p' src/qt/guiutil_font.cpp
sed -n '970,1080p' src/qt/guiutil_font.cppRepository: dashpay/dash
Length of output: 1910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo status =="
git rev-parse --show-toplevel 2>/dev/null || true
git status --short 2>/dev/null | head -50 || true
echo "== locate relevant files =="
git ls-files 2>/dev/null | grep -- 'src/qt/guiutil_font\.\(cpp|h\)$' || true
echo "== inspect relevant file sizes =="
for f in $(git ls-files 2>/dev/null | grep -- 'src/qt/guiutil_font\.cpp$' || true); do
wc -l "$f"
done
echo "== relevant definitions and slices =="
fs="$(git ls-files 2>/dev/null | grep -- 'src/qt/guiutil_font\.cpp$' | head -1 || true)"
if [ -n "${fs:-}" ]; then
sed -n '1,260p' "$fs"
echo "///// 560-720"
sed -n '560,720p' "$fs"
echo "///// 720-860"
sed -n '720,860p' "$fs"
echo "///// 860-1080"
sed -n '860,1080p' "$fs"
fi
echo "== searches =="
rg -n "mapWidgetDefaultFontSizes|mapClassFontUpdates|effectivePointSize|GetScaledFontSize|logicalDpiY|logicalDotsPerInch|setPointSize|PixelMetric" src/qt/guiutil_font.cpp src/qt -g '*.cpp' -g '*.h' 2>/dev/null || trueRepository: dashpay/dash
Length of output: 1910
🌐 Web query:
Qt documentation QFont isPixel logicalDotsPerInch logicalDotsPerInchY
💡 Result:
In Qt, logical DPI (dots per inch) is a metric used to scale user interface elements and convert font point sizes into pixel sizes, ensuring consistent physical size across different displays [1][2][3]. The key properties and functions related to this concept are: QScreen Properties The QScreen class provides three read-only properties for logical DPI [1][4]: - logicalDotsPerInchX: Returns the horizontal logical DPI [1][3]. - logicalDotsPerInchY: Returns the vertical logical DPI [1][3]. - logicalDotsPerInch: A convenience property that returns the average of the horizontal and vertical logical DPI values [1][3]. QPaintDevice Functions The QPaintDevice class (the base class for objects like QWidget and QPixmap) provides equivalent methods [5][6][2]: - logicalDpiX: Returns the horizontal resolution in dots per inch [5][6]. - logicalDpiY: Returns the vertical resolution in dots per inch [5][6]. Key Distinctions and Usage - Purpose: Logical DPI is used by Qt to calculate UI scaling and font sizes [1][2][7]. It may differ from physical DPI, which represents the screen's actual hardware pixel density [1][8][2]. - Customization: Users or desktop environments can sometimes adjust logical DPI settings to globally control the size of UI elements and text [1][8]. - High DPI Scaling: When high-DPI scaling is active, Qt calculates the appropriate scale factor (device pixel ratio) using the logical DPI relative to a base DPI (typically 96) [9][7]. - QFont Interaction: While QFont itself does not directly hold DPI properties, it relies on the system's logical DPI to interpret point sizes correctly during font matching and rendering [10][11][3]. In modern Qt development, relying on the QScreen methods is the preferred way to query these values for display-specific information, as QPaintDevice methods are often used for general device resolution queries [2].
Citations:
- 1: https://doc.qt.io/qt-6/qscreen.html
- 2: https://runebook.dev/en/docs/qt/qpaintdevice/logicalDpiY
- 3: https://codebrowser.dev/qt6/qtbase/src/gui/kernel/qscreen.cpp.html
- 4: https://doc.qt.io/qt-6.11/qscreen.html
- 5: https://doc.qt.io/qt-6.11/qpaintdevice.html
- 6: https://doc.qt.io/QT-6/qpaintdevice.html
- 7: https://github.com/qtproject/qtbase/blob/dev/src/gui/kernel/qplatformscreen.cpp
- 8: https://stackoverflow.com/questions/16561879/what-is-the-difference-between-logicaldpix-and-physicaldpix-in-qt
- 9: https://doc.qt.io/qt-6.5/highdpi.html
- 10: https://doc.qt.io/QT-6/qfont.html
- 11: https://codebrowser.dev/qt6/qtbase/src/gui/text/qfont.cpp.html
🌐 Web query:
Qt QFont documentation logicalDpiY QtQPA logicalDotsPerInchY documentation source code
💡 Result:
In Qt, logicalDpiY (and its associated property logicalDotsPerInchY) represents the vertical resolution of a device in dots per inch, which is specifically used for calculating font sizes [1][2][3][4]. Key Technical Details: 1. QPaintDevice::logicalDpiY: This function is a member of the QPaintDevice class and returns the device's vertical DPI [1][2]. It is used to ensure that fonts are rendered at the correct physical size regardless of the display's actual pixel density [1][2]. If the logical DPI differs from the physical DPI, the QPaintEngine is responsible for handling the mapping [1][2]. 2. QScreen::logicalDotsPerInchY: This property (accessed via the method of the same name) provides the vertical logical DPI for a specific screen [3][4]. It is primarily used to convert font point sizes into pixel sizes [3][4]. 3. Implementation and Usage: - When a QFont is constructed for a specific QPaintDevice, Qt uses the device's logicalDpiY to initialize the font's internal DPI settings, ensuring the font metrics are appropriate for that device [5][6][7]. - If no specific device is provided, Qt falls back to a default DPI (often retrieved via internal helper functions like qt_defaultDpiY) [5][6][7]. - In the Qt source code (e.g., in qfont.cpp), these values are frequently accessed to determine the scaling factor for text rendering [5][6][7]. In summary, logicalDpiY is a fundamental metric in Qt's font rendering pipeline that bridges the gap between abstract point sizes and concrete pixel dimensions [1][3][4].
Citations:
- 1: https://doc.qt.io/qt-6.11/qpaintdevice.html
- 2: https://doc.qt.io/qt-6.10/qpaintdevice.html
- 3: https://doc.qt.io/QT-6/qscreen.html
- 4: https://doc.qt.io/qt-6.10/qscreen.html
- 5: https://github.com/qt/qtbase/blob/85949d70/src/gui/text/qfont.cpp
- 6: https://codebrowser.dev/qt6/qtbase/src/gui/text/qfont.cpp.html
- 7: https://codebrowser.dev/qt5/qtbase/src/gui/text/qfont.cpp.html
🌐 Web query:
"QFont" "logicalDpiY" "dpiY" "Qt" "isPixel" documentation
💡 Result:
In Qt, logicalDpiY and QFont size settings serve different purposes regarding UI scaling and device independence [1][2]. logicalDpiY (from QPaintDevice) The logicalDpiY function returns the vertical resolution of a paint device in dots per inch (DPI), which is used by Qt to calculate font sizes and scale UI elements [3][1]. It represents the resolution used by the operating system or Qt framework for display scaling, intended to ensure that elements maintain a consistent physical appearance across different monitors [1][2]. QFont and Font Sizes QFont supports two primary ways of defining font size: point size and pixel size [4][5]. 1. Point Size (Recommended): When you use setPointSize or setPointSizeF, the font size is defined in points (1/72 of an inch) [1]. This is a device-independent measurement [5]. Qt converts this point size into a pixel size based on the device's logical DPI [1][2]. Because this conversion accounts for the current logical DPI, point-sized fonts scale automatically when the system DPI or user scaling settings change [1][6]. 2. Pixel Size (Device Dependent): When you use setPixelSize, the font size is defined explicitly in pixels [7][5]. This makes the font device-dependent because it bypasses the logical DPI scaling calculation [5][6]. A pixel-sized font will generally remain the same number of pixels tall regardless of the DPI, which can cause it to appear too small on high-resolution (high-DPI) displays if not handled manually [1][6]. Key Takeaways - Use setPointSize for most applications to ensure font scaling behaves correctly with the system's logical DPI [5][1]. - logicalDpiY is a metric used for scaling, not a property you typically modify for individual font objects [1]. - If you use setPixelSize, you are responsible for manually adjusting the font size if you want it to scale with the display resolution [5][1]. For further details, refer to the QPaintDevice::logicalDpiY and QFont documentation [3][4].
Citations:
- 1: https://runebook.dev/en/docs/qt/qpaintdevice/logicalDpiY
- 2: https://stackoverflow.com/questions/16561879/what-is-the-difference-between-logicaldpix-and-physicaldpix-in-qt
- 3: https://doc.qt.io/qt-6/qpaintdevice.html
- 4: https://doc.qt.io/qt-6/qfont.html
- 5: https://doc.qt.io/qt-6.10/qfont.html
- 6: https://stackoverflow.com/questions/73562193/what-is-the-difference-between-point-and-pixel-and-why-is-pixel-unaffected-by-dp
- 7: https://github.com/qt/qtbase/blob/85949d70/src/gui/text/qfont.cpp
Preserve pixel-sized cache entries in their original unit.
Both caches store the first converted point size instead of the original pixel size. A pixel font captured at one DPI is then reused on a different DPI as the same point value, so stylesheet text can shrink or grow unintentionally after a screen/DPI transition. Keep the original pixel size for these Qt pixel-font entries and rederive the scaled point size from the relevant current DPI for each refresh.
📍 Affects 1 file
src/qt/guiutil_font.cpp#L772-L776(this comment)src/qt/guiutil_font.cpp#L807-L820
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/qt/guiutil_font.cpp` around lines 772 - 776, Update both font-cache sites
in src/qt/guiutil_font.cpp: lines 772-776 near mapWidgetDefaultFontSizes and
lines 807-820 in the companion cache path. Store the original pixel-sized value
for Qt pixel-font entries, rather than the first DPI-converted point size, and
on each refresh recompute the point size using the current relevant DPI;
preserve existing behavior for non-pixel font entries.
There was a problem hiding this comment.
Good catch. This identifies a real but explicitly documented limitation of the merged fix, rather than the original abort regression.
The per-widget cache intentionally freezes the first usable base size so repeated updateFonts() passes cannot compound scaling; the final code calls out that a pixel-derived size will therefore not be re-derived after a later per-screen DPI transition. Preserving pixel intent across such transitions would require a unit-aware cache (pixel vs. point), current-screen DPI conversion on each refresh, and focused screen/DPI-change coverage.
That broader behavior was not folded into the already-merged crash fix. No further change can be made on this PR; this comment remains useful follow-up guidance.
There was a problem hiding this comment.
@thepastaclaw, agreed. The finding is valid follow-up guidance, but the documented first-DPI cache behavior is an intentional trade-off in the merged crash-regression fix—not a reason to reopen #7465.
Would you like me to open a GitHub issue for unit-aware pixel/point font caching, DPI-transition re-conversion, and focused multi-screen/DPI test coverage?
🐇🔍
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/dash PR: 7465
File: src/qt/guiutil_font.cpp:772-776
Timestamp: 2026-07-29T20:39:54.924Z
Learning: In `src/qt/guiutil_font.cpp`, `GUIUtil::updateFonts()` intentionally caches the first derived widget base font size to prevent scaling from compounding on repeated refreshes. Consequently, a pixel-sized stylesheet font converted at its first observed DPI is not re-derived when the widget later moves to a screen with a different DPI. Preserving pixel intent across DPI transitions requires a unit-aware cache, conversion using the current relevant DPI on each refresh, and dedicated screen/DPI-change tests; this behavior was deliberately out of scope for the crash fix merged in Dash pull request `#7465`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07a676fbd4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| QVERIFY2(GUIUtil::fontsLoaded(), | ||
| "GUIUtil::loadFonts() must succeed in AppTests::appTests() before OptionTests run."); |
There was a problem hiding this comment.
On macOS the Qt test binary forces QT_QPA_PLATFORM=minimal before running AppTests and then OptionTests, but AppTests::appTests() returns before GUIUtil::loadFonts() on that platform. In that default macOS test context this new assertion fails before exercising the regression body, so test_dash-qt is broken unless this test also skips under the same condition or initializes the font state itself.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the final merged head (67d25da616a). OptionTests::updateFontsWithPixelSizedWidget() now calls QSKIP(...) when the platform plugin is minimal, and on supported platforms it requires GUIUtil::fontsLoaded() before exercising the regression path. The final CI matrix, including the macOS build, passed.
07a676f to
1cb9e2c
Compare
1cb9e2c to
67d25da
Compare
…dgets 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) Cherry-picked from upstream dashpay#7465; the backported code is unchanged. Two adaptations were needed for v23.1.x: - Conflicts in guiutil_font.cpp were limited to surrounding context that does not exist on this branch (develop-only helpers and a differing include block). - optiontests.cpp additionally includes qt/guiutil_font.h: fontsLoaded() and updateFonts() are declared there on v23.1.x, whereas develop declares them in qt/guiutil.h, which is all the upstream test includes.
…dgets 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) Cherry-picked from upstream dashpay#7465; the backported code is unchanged. Two adaptations were needed for v23.1.x: - Conflicts in guiutil_font.cpp were limited to surrounding context that does not exist on this branch (develop-only helpers and a differing include block). - optiontests.cpp additionally includes qt/guiutil_font.h: fontsLoaded() and updateFonts() are declared there on v23.1.x, whereas develop declares them in qt/guiutil.h, which is all the upstream test includes.
24920a0 chore: prepare v23.1.8 release (pasta) 2194248 Merge #7348: fix: penalize oversized notfound messages (pasta) f5c72c3 Merge #7347: fix: punish invalid dstx messages (pasta) 550caf7 Merge #7465: fix(qt): handle pixel-sized fonts when scaling widgets (pasta) e203710 Merge #7419: fix(net): bound CoinJoin message vector intake (pasta) 5f5b960 Merge #7418: fix(net): bound signing message vector intake (Pasta) 7cc2cca Merge #7450: test: make governance vote fixtures wire-valid (Pasta) f011c80 Merge #7440: fix(net): bound governance vote signature deserialization (Pasta) 4b4d96a Merge #7442: fix(net): authorize governance inv responses via the net-layer per-peer request tracker (Pasta) f855b13 Merge #7444: fix(net): bound bloom message vectors before allocation (Pasta) 5b5c6fb Merge #7415: fix: bound pending sig share queue (Pasta) 9bbe808 Merge #7416: fix(net): bound quorum data response vectors (Pasta) da42f50 Merge #7424: fix: bound ChainLock seen cache (Pasta) 5b310df Merge #7438: fix: bound SPORK signature deserialization (Pasta) e118d0c Merge #7259: fix: dangling point to cj client (Pasta) 9921621 Merge #7439: refactor: add bounded vector deserialization (Pasta) 89bdf7c Merge #7414: fix(net): throttle per-object governance vote sync requests (Pasta) 44c396d Merge #7402: fix: bound pending recovered sig queue to prevent remote OOM (Pasta) 05cfe27 Merge #7351: fix: limit signing share sessions per peer (pasta) 3ef3a5b Merge #7408: fix: bound DKG contribution blob intake (pasta) 0ea6532 Merge #7387: test: migrate governance inv cache coverage to unit tests (Pasta) 8ffdf7f Merge #7398: backport: compact block relay hardening (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) (Pasta) 2915142 backport: bitcoin#27608 - p2p: Avoid prematurely clearing download state for other peers (PastaClaw) 90b5473 Merge #7396: fix: run of circular-dependencies with python3.15 (Pasta) b003cdc Merge #7395: ci: update GitHub Actions pins for Node 24 (pasta) 97c3dd1 Merge #7394: fix: stabilize par help text in manpages (pasta) 8f8616b Merge #7372: backport: bitcoin#32693: depends: fix cmake compatibility error for freetype (pasta) 48f72be Merge #7360: fix: empty platformP2PPort deprecated field in protx listdiff results (pasta) a8cccff Merge #7298: fix(qt): keep PoSe score visible when hiding banned masternodes (pasta) Pull request description: Release PR for Dash Core v23.1.8, a patch release on top of v23.1.7. Fast-forwards from `v23.1.x` (currently at `chore: prepare v23.1.7 release`), 29 commits, no merge commits, no conflicts. ## Contents Backports of PRs already reviewed and merged on `develop`: `#7259` `#7347` `#7348` `#7351` `#7298` `#7360` `#7372` `#7387` `#7394` `#7395` `#7396` `#7398` `#7402` `#7408` `#7414` `#7415` `#7416` `#7418` `#7419` `#7424` `#7438` `#7439` `#7440` `#7442` `#7444` `#7450` `#7465` Plus `backport: bitcoin#27608`, a single commit taken from Dash #7237 because #7398's compact-block hardening depends on it. The rest of that v0.26 batch is intentionally not included on v23.1.x. The commit is byte-identical to its reviewed counterpart inside #7237. And release preparation: version bump, regenerated man pages, release notes, archived 23.1.7 notes. ## Note for reviewers: this branch was rebuilt An earlier revision of this PR was discarded and the branch rebuilt from scratch. Review comments on the previous revision point at commits that no longer exist, though the feedback itself was carried over (see below). The reason: several commits titled `Merge #NNNN` in the earlier revision contained substantial code that exists nowhere upstream — apparently written from a description of each PR rather than ported from its diff. For example, `feature_llmq_simplepose.py` is byte-identical between v23.1.7 and `develop`, yet the earlier `Merge #7408` rewrote 66 lines of it; `test/functional/p2p_governance_invs.py` does not exist on `develop` at all, yet had grown from 62 to 148 lines. That mislabeling matters because a commit titled `Merge #NNNN` invites less scrutiny, not more. It also had consequences: the earlier revision was **missing #7440 entirely**, and contained eleven consecutive commits that did not compile (code written against newer upstream APIs this branch does not have — `Misbehaving(Peer&)`, and `PeerIsBanned` used five commits before it was declared). Every commit on this branch has now been diffed against its upstream merge commit. Where a backport differs, it is because v23.1.x predates an upstream refactor and the change had to be applied to the pre-refactor file — for example #7418 and #7438 patch `signing_shares.cpp` / `spork.cpp` where upstream patches `net_signing.cpp` / `net_processing.cpp`. ## Dropped from this branch - **#7350** (`net: don't lock cs_main while reading blocks`) — dropped on review feedback. It is a 110-line lock-structure refactor of `ProcessGetBlockData` with no measured benefit, and it would add avoidable churn to the eventual master→develop merge-back. Nothing on this branch depends on it: #7398's compact-block work precedes it, and the remaining 14 commits replay with zero conflicts once it is removed. Thanks @knst. ## Added after the initial review pass - **#7351** (`fix: limit signing share sessions per peer`) — cherry-picked as a single commit and placed before #7402, matching upstream's merge order. The include block additionally carries `<ranges>`: upstream's diff adds only `<algorithm>` because develop already had it, whereas v23.1.x did not and the backported `GetSessionCount()` / `GetAnnouncementSessionCount()` use `std::ranges::count_if`. - **#7465** (`fix(qt): handle pixel-sized fonts when scaling widgets`) — cherry-picked from the five upstream commits. `optiontests.cpp` additionally includes `qt/guiutil_font.h`, because `fontsLoaded()` and `updateFonts()` are declared there on v23.1.x while develop declares them in `qt/guiutil.h`, which is all the upstream test includes. Two further backports were added later and applied without any adaptation -- their diffs are byte-for-byte identical to upstream: - **#7347** (`fix: punish invalid dstx messages`) - **#7348** (`fix: penalize oversized notfound messages`) ## Adaptations worth flagging - **#7360** — upstream gates `platformP2PPort` / `platformHTTPPort` in `protx listdiff` behind `IsServiceDeprecatedRPCEnabled()`. On 23.x those deprecated fields are deliberately not enforced through gating (see `bbcd9d543e6`), so shipping the gate as-is would silently drop two fields that v23.1.7 always returned. Changed to `if (true)` with a comment, per review feedback, keeping the block aligned with `develop`. The substantive fix from #7360 — reading the live port from `netInfo` instead of the always-zero scalar — is retained. - **#7415** — the pending-map caps (`MAX_PENDING_SIG_SHARES_PER_NODE`, `MAX_PENDING_SIG_SHARES_TOTAL`) are backported. The additional bound upstream places on batches awaiting verification is not, because it guards a condition that does not exist here: upstream's dispatcher pushes one task per batch inside an inner loop, whereas v23.1.x pushes a single looping worker per 10 ms tick. There is no unbounded task queue to bound. - **Man pages** — regenerated without the `lock` debug category, which only exists under `DEBUG_LOCKCONTENTION` and so is absent from release binaries. Thanks @UdjinM6 for catching this. ## Known CI failure macOS jobs are expected to fail. `actions/upload-artifact@v6` rejects filenames containing `:`, and the Xcode SDK ships Perl man pages with `::` in the name. A release-branch-only workaround existed on the earlier revision but was dropped as it corresponds to no upstream PR. This is accepted for this release. ## Testing - Every commit through #7465 compiles individually (verified for 27 of the 29; the three additions below were verified at the tip) — verified individually, not just at the tip. - Full build clean; no new warnings. - Unit tests pass. - Functional tests pass: `feature_llmq_signing` (both variants), `feature_llmq_chainlocks`, `feature_llmq_dkgerrors`, `feature_llmq_is_cl_conflicts`, `p2p_instantsend`, `feature_dip3_deterministicmns` (both wallet types), `rpc_coinjoin`. - Qt unit tests pass (32 cases, run under the `cocoa` platform plugin so the pixel-sized font regression from #7465 actually executes rather than self-skipping). - Lint: one pre-existing `lint-cppcheck-dash` failure, identical on v23.1.7, in files this branch does not touch. Top commit has no ACKs. Tree-SHA512: 0fa469c9a33820aa85fbb8b90c5877409d09490f746f1300b05ceda470a600765f900bec42e5aad5d90a2d46b44e28c20e44dc4a8fe719553a072298069eaec4
Upstream landed its own "release: prepare v23.1.8" (dashpay#7493) plus parallel merges of several PRs this branch had already backported, so most conflicts are two adaptations of the same change rather than divergent intent. Resolutions: - Release artifacts (manpages, flatpak metainfo, release notes, configure.ac): took upstream's published v23.1.8 text as the base, then re-added the CoinJoin/wallet, GUI, and credits content that upstream's copy lacked. - LLMQ/net/test files where upstream carried a refined superset (dashpay#7351, dashpay#7347, dashpay#7348, dashpay#7465): took upstream. - CoinJoin client lifetime, overviewpage mixing-state, dmnstate platform ports, and coinjoin_tests: kept this branch's versions. - test/functional/p2p_governance_invs.py: accepted upstream's removal in favor of the src/test/governance_inv_tests.cpp migration (dashpay#7387). Auto-merge produced two duplicate definitions that would not compile (IsSyncableObject in governance.cpp, UnserializeBatchedSigShares in signing_shares.cpp); both were reduced to a single definition. Co-Authored-By: Claude <noreply@anthropic.com>
4 participants
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?
QTextEditstyling path.GUIUtil::updateFonts()regression test to the existingOptionTestssuite without changing test order or adding a translation unit.How Has This Been Tested?
QT_QPA_PLATFORM=minimal ./src/qt/test/test_dash-qt updateFontsWithPixelSizedWidgetpasses with 7OptionTestspassing and the integration body explicitly skipped because theminimalplugin cannot load application fonts. The deterministiceffectivePointSize()coverage runs under this configuration.AppTestssuccessfully 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