This PR removes massive warnings like:
QWARN : ... QFont::setPointSizeF: Point size <= 0 (...), must be greater than 0
from test_bitcoin-qt output.
On master (e258ce792a4849927a6db51786732d71cbbb65fc):
$ ./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
ACK 8ab99dd9544e6fa61da1cd77922c85e2970c0416 -- tests should be silent in the absence of errors: thanks for fixing! :)
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.
Updated 8ab99dd9544e6fa61da1cd77922c85e2970c0416 -> d318a14cc87f672198aea7490d76950919357f7d (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.
496 | @@ -496,7 +497,7 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty 497 | ui->detailWidget->hide(); 498 | ui->peerHeading->setText(tr("Select a peer to view detailed information.")); 499 | 500 | - consoleFontSize = settings.value(fontSizeSettingsKey, QFontInfo(QFont()).pointSize()).toInt(); 501 | + consoleFontSize = settings.value(fontSizeSettingsKey, QFont().pointSize()).toInt();
Why is this change needed? And what is the observable change?
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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
396 | @@ -396,8 +397,9 @@ void TransactionView::contextualMenu(const QPoint &point) 397 | abandonAction->setEnabled(model->wallet().transactionCanBeAbandoned(hash)); 398 | bumpFeeAction->setEnabled(model->wallet().transactionCanBeBumped(hash)); 399 | 400 | - if(index.isValid()) 401 | - { 402 | + if (index.isValid() && QGuiApplication::platformName() != "minimal") { 403 | + // The qminimal plugin does not provide window system integration. 404 | + // Skip this call to suppress warnings during the tests with QT_QPA_PLATFORM=minimal. 405 | contextMenu->popup(transactionView->viewport()->mapToGlobal(point));
If you keep this approach maybe refactor this to something like GUIUtil::popup(contextMenu, ...) and remove the new include which is already in guiutil.cpp.
Alternatively you could filter the warning in
DebugMessageHandleror even callqInstallMessageHandlerwith 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
Updated d318a14cc87f672198aea7490d76950919357f7d -> 50c2791e35929c53c351e82a6188e2b58f5151e8 (pr17908.02 -> pr17908.03, diff):
903 | @@ -904,4 +904,11 @@ void LogQtInfo() 904 | } 905 | } 906 | 907 | +void QMenuPopup(QMenu* menu, const QPoint& point, QAction* atAction)
nit, I'd name popupMenu, not sure why the Q prefix.
903 | @@ -904,4 +904,11 @@ void LogQtInfo() 904 | } 905 | } 906 | 907 | +void QMenuPopup(QMenu* menu, const QPoint& point, QAction* atAction) 908 | +{ 909 | + // The qminimal plugin does not provide window system integration. 910 | + if (QGuiApplication::platformName() == "minimal") return;
nit, QApplication just to make it clear what's the required include.
Actually, QApplication inherits platformName() from QGuiApplication :tiger2:
Code review ACK 50c2791e35929c53c351e82a6188e2b58f5151e8.
Updated 50c2791e35929c53c351e82a6188e2b58f5151e8 -> 2a388bdf8f6685d639713c903443458afc2458e2 (pr17908.03 -> pr17908.04, diff):
Code review ACK 2a388bdf8f6685d639713c903443458afc2458e2.
Rebased 2a388bdf8f6685d639713c903443458afc2458e2 -> 1122817c194ed49abf896e68604e725c3b5c8569 (pr17908.04 -> pr17908.05) due to the conflict with #16710.
Code review ACK 1122817c194ed49abf896e68604e725c3b5c8569.
Only Qt issue I found that could be related is https://bugreports.qt.io/browse/QTBUG-72335.
utACK 1122817c194ed49abf896e68604e725c3b5c8569