-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(qt): handle pixel-sized fonts when scaling widgets #7465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b6dd65a
322f257
443e1d3
469abf8
67d25da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,8 +9,12 @@ | |
| #include <test/util/setup_common.h> | ||
| #include <util/system.h> | ||
|
|
||
| #include <QApplication> | ||
| #include <QFont> | ||
| #include <QLabel> | ||
| #include <QSettings> | ||
| #include <QTest> | ||
| #include <QWidget> | ||
|
|
||
| #include <univalue.h> | ||
|
|
||
|
|
@@ -132,3 +136,67 @@ void OptionTests::extractFilter() | |
| filter = QString("Image (*.png *.jpg)"); | ||
| QCOMPARE(GUIUtil::ExtractFirstSuffixFromFilter(filter), "png"); | ||
| } | ||
|
|
||
| void OptionTests::effectivePointSize() | ||
| { | ||
| using GUIUtil::internal::effectivePointSize; | ||
|
|
||
| // A point-sized font reports its size directly, fractions included, whatever the DPI. | ||
| QFont point_font; | ||
| point_font.setPointSizeF(12.5); | ||
| QCOMPARE(effectivePointSize(point_font, 96).value_or(0), 12.5); | ||
| QCOMPARE(effectivePointSize(point_font, 72).value_or(0), 12.5); | ||
|
|
||
| // A pixel-sized font carries no point size and must be converted using the target DPI. | ||
| QFont pixel_font; | ||
| pixel_font.setPixelSize(17); | ||
| QVERIFY(pixel_font.pointSizeF() <= 0); | ||
| QCOMPARE(effectivePointSize(pixel_font, 96).value_or(0), 17 * 72.0 / 96); | ||
| QCOMPARE(effectivePointSize(pixel_font, 144).value_or(0), 8.5); | ||
| // At 72 DPI points and pixels coincide; pinned so the identity is deliberate rather | ||
| // than an accident of whichever DPI the host happens to report. | ||
| QCOMPARE(effectivePointSize(pixel_font, 72).value_or(0), 17.0); | ||
|
|
||
| // A non-positive DPI cannot yield a conversion factor, so the pixel size is unusable | ||
| // even though it is valid. QWidget::logicalDpiY() is not guaranteed to be positive. | ||
| QVERIFY(!effectivePointSize(pixel_font, 0).has_value()); | ||
| QVERIFY(!effectivePointSize(pixel_font, -1).has_value()); | ||
|
|
||
| // The remaining branch -- neither size usable -- is guarded but not asserted here: Qt | ||
| // rejects non-positive sizes in the setters, and once a QGuiApplication exists (as it | ||
| // does in this binary) every QFont is handed a valid default point size. The state is | ||
| // still reachable in production, e.g. a font engine that populates no size at all, so | ||
| // the helper compares against 0 rather than trusting any particular sentinel. | ||
| } | ||
|
|
||
| void OptionTests::updateFontsWithPixelSizedWidget() | ||
| { | ||
| if (QApplication::platformName() == "minimal") { | ||
| QSKIP("AppTests cannot initialize fonts with the 'minimal' platform plugin."); | ||
| } | ||
|
|
||
| // updateFonts() is a no-op until loadFonts() has run, and loadFonts() is process-global, | ||
| // non-idempotent state owned by AppTests. Treat missing initialization as a failure on | ||
| // supported platforms so the regression test cannot pass without exercising updateFonts(). | ||
| QVERIFY2(GUIUtil::fontsLoaded(), | ||
| "GUIUtil::loadFonts() must succeed in AppTests::appTests() before OptionTests run."); | ||
|
Comment on lines
+181
to
+182
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On macOS the Qt test binary forces Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in the final merged head ( |
||
|
|
||
| QWidget host; | ||
| QLabel* label{new QLabel(&host)}; | ||
| QFont pixel_font{label->font()}; | ||
| pixel_font.setPixelSize(17); | ||
| label->setFont(pixel_font); | ||
| QVERIFY(label->font().pointSizeF() <= 0); | ||
|
|
||
| // The pre-fix code asserted pointSize() > 0 here and aborted the process. The widget must | ||
| // now be swept normally and end up with a usable point size. The exact value depends on | ||
| // the host DPI, so the arithmetic is pinned in effectivePointSize() above instead. | ||
| GUIUtil::updateFonts(); | ||
| const double scaled_size{label->font().pointSizeF()}; | ||
| QVERIFY(scaled_size > 0); | ||
|
|
||
| // The size is cached per widget on the first sweep, so repeated passes must not compound | ||
| // it -- the defect that makes the cache load-bearing rather than an optimisation. | ||
| GUIUtil::updateFonts(); | ||
| QCOMPARE(label->font().pointSizeF(), scaled_size); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: dashpay/dash
Length of output: 1910
🏁 Script executed:
Repository: dashpay/dash
Length of output: 1910
🏁 Script executed:
Repository: 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:
🌐 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:
🌐 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:
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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