qt: Remove QFont warnings with QT_QPA_PLATFORM=minimal #17908

hebasto · 2020-01-11T12:47:01Z

This PR removes massive warnings like:

QWARN  : ... QFont::setPointSizeF: Point size <= 0 (...), must be greater than 0

from test_bitcoin-qt output.

On master (e258ce7):

$ ./src/qt/test/test_bitcoin-qt | grep QFont | wc -l
~BitcoinApplication : Stopping thread
~BitcoinApplication : Stopped thread
57

With this PR:

$ ./src/qt/test/test_bitcoin-qt | grep QFont | wc -l
~BitcoinApplication : Stopping thread
~BitcoinApplication : Stopped thread
0

practicalswift · 2020-01-12T09:44:54Z

ACK 8ab99dd -- tests should be silent in the absence of errors: thanks for fixing! :)

promag

Not sure about this change. What if this is eventually fixed on Qt? Are these warnings worth the change?

Alternatively you could filter the warning in DebugMessageHandler or even call qInstallMessageHandler with a specific test message handler.

Either way a comment explaining the condition would be nice.

hebasto · 2020-01-14T17:41:14Z

Updated 8ab99dd -> d318a14 (pr17908.01 -> pr17908.02, compare).

@promag

Not sure about this change. What if this is eventually fixed on Qt? Are these warnings worth the change?

Tests should not be noisy, IMO. 57 warnings messages look spammy ;)

Either way a comment explaining the condition would be nice.

A comment has been added.

maflcko · 2020-01-14T17:58:49Z

src/qt/rpcconsole.cpp

@@ -496,7 +497,7 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
    ui->detailWidget->hide();
    ui->peerHeading->setText(tr("Select a peer to view detailed information."));

-    consoleFontSize = settings.value(fontSizeSettingsKey, QFontInfo(QFont()).pointSize()).toInt();
+    consoleFontSize = settings.value(fontSizeSettingsKey, QFont().pointSize()).toInt();


Why is this change needed? And what is the observable change?

https://doc.qt.io/qt-5/qfontinfo.html#details

If QT_QPA_PLATFORM=minimal (i.e., without window system integration), QFontInfo(QFont()).pointSize() returns -1.

I see, but when QT_QPA_PLATFORM is set to something else, we actually want the values that are used to draw the text, not the point size of QFont{}, no?

It seems there is no harm when RPCConsole::setFontSize() is called. With non-qminimal plugin Qt always could draw a correct font.

I am not an expert in qt, and it might work for you. But in the general case it might depend on the fonts that are selected in the system settings.

Qt Documentation -> Drawing and Filling:

Qt will use the font with the specified attributes, or if no matching font exists, Qt will use the closest matching installed font.

DrahtBot · 2020-03-02T23:30:58Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

qt: Handle exceptions instead of crash #18897 (qt: Handle exceptions instead of crash by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag

Alternatively you could filter the warning in DebugMessageHandler or even call qInstallMessageHandler with a specific test message handler.

@hebasto have you considered this approach?

src/qt/transactionview.cpp

hebasto · 2020-03-15T19:40:38Z

@promag

Alternatively you could filter the warning in DebugMessageHandler or even call qInstallMessageHandler with a specific test message handler.

@hebasto have you considered this approach?

it seems DebugMessageHandler is not used in test_bitcoin-qt
qInstallMessageHandler with a specific test message handler could suppress warnings for possible new cases in the future

hebasto · 2020-03-15T20:32:33Z

Updated d318a14 -> 50c2791 (pr17908.02 -> pr17908.03, diff):

implemented @promag's suggestion

promag

Code review ACK 50c2791.

src/qt/guiutil.cpp

promag · 2020-04-26T22:58:05Z

src/qt/guiutil.cpp

+void QMenuPopup(QMenu* menu, const QPoint& point, QAction* atAction)
+{
+    // The qminimal plugin does not provide window system integration.
+    if (QGuiApplication::platformName() == "minimal") return;


nit, QApplication just to make it clear what's the required include.

Actually, QApplication inherits platformName() from QGuiApplication 🐅

hebasto · 2020-04-27T03:05:07Z

Updated 50c2791 -> 2a388bd (pr17908.03 -> pr17908.04, diff):

addressed @promag's comments

promag · 2020-04-27T08:08:22Z

Code review ACK 2a388bd.

hebasto · 2020-05-13T13:06:22Z

Rebased 2a388bd -> 1122817 (pr17908.04 -> pr17908.05) due to the conflict with #16710.

promag · 2020-05-18T22:49:23Z

Code review ACK 1122817.

Only Qt issue I found that could be related is https://bugreports.qt.io/browse/QTBUG-72335.

jonasschnelli · 2020-05-29T08:17:19Z

utACK 1122817

…inimal 1122817 qt: Remove QFont warnings with QPA=minimal (Hennadii Stepanov) Pull request description: This PR removes massive warnings like: ``` QWARN : ... QFont::setPointSizeF: Point size <= 0 (...), must be greater than 0 ``` from `test_bitcoin-qt` output. On master (e258ce7): ``` $ ./src/qt/test/test_bitcoin-qt | grep QFont | wc -l ~BitcoinApplication : Stopping thread ~BitcoinApplication : Stopped thread 57 ``` With this PR: ``` $ ./src/qt/test/test_bitcoin-qt | grep QFont | wc -l ~BitcoinApplication : Stopping thread ~BitcoinApplication : Stopped thread 0 ``` ACKs for top commit: promag: Code review ACK 1122817. jonasschnelli: utACK 1122817 Tree-SHA512: 32fa72a5d3db1d4c73a2a324aa9cad807ee46f23fc5319f7d71202987dc73ea7c90082904489b323a432e1afaebd9976b7dd0374236a16153162aa75fe368723

Summary: This is a backport of [[bitcoin/bitcoin#17908 | core#17908]] Test Plan: Before this patch: ``` $ ./src/qt/test/test_bitcoin-qt | grep QFont | wc -l ~BitcoinApplication : Stopping thread ~BitcoinApplication : Stopped thread 17 ``` After: ``` $ ./src/qt/test/test_bitcoin-qt | grep QFont | wc -l ~BitcoinApplication : Stopping thread ~BitcoinApplication : Stopped thread 0 ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9924

fanquake added the GUI label Jan 11, 2020

fanquake changed the title ~~qa, qt: Remove QFont warnings with QT_QPA_PLATFORM=minimal~~ qt: Remove QFont warnings with QT_QPA_PLATFORM=minimal Jan 11, 2020

promag reviewed Jan 12, 2020

View reviewed changes

hebasto force-pushed the 20200111-qttest-minimal-warn branch from 8ab99dd to d318a14 Compare January 14, 2020 17:33

maflcko reviewed Jan 14, 2020

View reviewed changes

DrahtBot mentioned this pull request Mar 3, 2020

build: Enable -Wsuggest-override if available #16710

Merged

promag reviewed Mar 15, 2020

View reviewed changes

src/qt/transactionview.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 20200111-qttest-minimal-warn branch from d318a14 to 50c2791 Compare March 15, 2020 20:28

promag reviewed Apr 26, 2020

View reviewed changes

hebasto force-pushed the 20200111-qttest-minimal-warn branch from 50c2791 to 2a388bd Compare April 27, 2020 03:04

DrahtBot mentioned this pull request May 8, 2020

qt: Handle exceptions instead of crash #18897

Closed

DrahtBot added the Needs rebase label May 13, 2020

qt: Remove QFont warnings with QPA=minimal

1122817

hebasto force-pushed the 20200111-qttest-minimal-warn branch from 2a388bd to 1122817 Compare May 13, 2020 13:04

DrahtBot removed the Needs rebase label May 13, 2020

jonasschnelli merged commit c2c15ea into bitcoin:master May 29, 2020

hebasto deleted the 20200111-qttest-minimal-warn branch May 29, 2020 09:00

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request May 29, 2020

[qt] Remove QFont warnings with QT_QPA_PLATFORM=minimal (bitcoin#17908)

bf9f502

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

qt: Remove QFont warnings with QT_QPA_PLATFORM=minimal #17908

qt: Remove QFont warnings with QT_QPA_PLATFORM=minimal #17908

Uh oh!

Conversation

hebasto commented Jan 11, 2020

Uh oh!

practicalswift commented Jan 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jan 14, 2020

Uh oh!

maflcko Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Mar 15, 2020

Uh oh!

hebasto commented Mar 15, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag Apr 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 27, 2020

Uh oh!

promag commented Apr 27, 2020

Uh oh!

hebasto commented May 13, 2020

Uh oh!

promag commented May 18, 2020

Uh oh!

jonasschnelli commented May 29, 2020

Uh oh!

Uh oh!

practicalswift commented Jan 12, 2020 •

edited

Loading

hebasto Jan 14, 2020 •

edited

Loading

DrahtBot commented Mar 2, 2020 •

edited

Loading

promag Apr 26, 2020 •

edited

Loading