The “Error” prefix/title is set already in the next functions:
noui_ThreadSafeMessageBox()https://github.com/bitcoin/bitcoin/blob/2068f089c8b7b90eb4557d3f67ea0f0ed2059a23/src/noui.cpp#L17ThreadSafeMessageBox()https://github.com/bitcoin/bitcoin/blob/a720a983015c9ef8cc814c16a5b9ef6379695817/src/qt/bitcoingui.cpp#L1351Currently on master:
With this PR:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
@promag https://doc.qt.io/qt-5/qmessagebox.html#setWindowTitle
On macOS, the window title is ignored (as required by the macOS Guidelines).
I can see two possible ways:
QMessageBox text:
Which one is more preferable?
Thanks for working on this. I agree with @ryanofsky that the message should be the same.
Maybe the wording should be more “final” since “low” is eventually something that is non-critical were “too low” would be understandable (together with red error sign).
What about prepending the prefix based on the error type?
0diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
1index abf9136eee..851bfe2b71 100644
2--- a/src/qt/bitcoingui.cpp
3+++ b/src/qt/bitcoingui.cpp
4@@ -1022,9 +1022,9 @@ void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, double nVer
5 progressBar->setToolTip(tooltip);
6 }
7
8-void BitcoinGUI::message(const QString &title, const QString &message, unsigned int style, bool *ret)
9+void BitcoinGUI::message(const QString& title, QString message, unsigned int style, bool* ret)
10 {
11- QString strTitle = tr("Bitcoin"); // default title
12+ QString strTitle{PACKAGE_NAME}; // default title
13 // Default to information icon
14 int nMBoxIcon = QMessageBox::Information;
15 int nNotifyIcon = Notificator::Information;
16@@ -1039,20 +1039,24 @@ void BitcoinGUI::message(const QString &title, const QString &message, unsigned
17 switch (style) {
18 case CClientUIInterface::MSG_ERROR:
19 msgType = tr("Error");
20+ message = tr("Error") + ": " + message;
21 break;
22 case CClientUIInterface::MSG_WARNING:
23 msgType = tr("Warning");
24+ message = tr("Warning") + ": " + message;
25 break;
26 case CClientUIInterface::MSG_INFORMATION:
27 msgType = tr("Information");
28+ message = tr("Information") + ": " + message;
29 break;
30 default:
31 break;
32 }
33 }
34- // Append title to "Bitcoin - "
35- if (!msgType.isEmpty())
36+ // Append type to title
37+ if (!msgType.isEmpty()) {
38 strTitle += " - " + msgType;
39+ }
40
41 // Check for error/warning icon
42 if (style & CClientUIInterface::ICON_ERROR) {
43diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h
44index b58ccbb455..54a4b3903c 100644
45--- a/src/qt/bitcoingui.h
46+++ b/src/qt/bitcoingui.h
47@@ -219,7 +219,7 @@ public Q_SLOTS: [@see](/bitcoin-bitcoin/contributor/see/) CClientUIInterface::MessageBoxFlags [@param](/bitcoin-bitcoin/contributor/param/)[in] ret pointer to a bool that will be modified to whether Ok was clicked (modal only)
48 */
49- void message(const QString &title, const QString &message, unsigned int style, bool *ret = nullptr);
50+ void message(const QString& title, QString message, unsigned int style, bool* ret = nullptr);
51
52 #ifdef ENABLE_WALLET
53 void setCurrentWallet(WalletModel* wallet_model);
re: #15894 (comment)
message = tr("Error") + ": " + message;
Formatting could be part of the translation:
0message = tr("Error: %s", message)
Currently, Bitcoin Core code has the following features.
The ThreadSafeMessageBox() messages are rendered in two ways:
noui_ThreadSafeMessageBox() using LogPrintf() and fprintf(stderr, ...) with
adding translated type prefix (“Error” / “Warning” / “Information”)BitcoinGUI::message() using QMessageBox with adding translated type suffix (“Error” / “Warning” / “Information”) to the window title. Note: On macOS, the window title is ignored (as required by the macOS Guidelines)The ThreadSafeMessageBox() is called by some functions:
AbortNode() and InitRPCAuthentication() pass translated messages with “Error: " prefixFatalError() passes non translated messages with “Error: " prefixInitError(); some passed messages are translated, some are notInitWarning() passes translated messagesInitHTTPAllowList() passes non-translated messagesCCoinsViewErrorCatcher::GetCoin() pass translated message started with “Error”CConnman::BindListenPort() via CConnman::Bind() passes:
CConnman::Start() passes translated messagesAddTimeData() passes translated messageUnfortunately, I cannot see the simple way to refactor the code to achieve both uniformity and usability.
Any thoughts?
re: #15894 (comment) from hebasto
Unfortunately, I cannot see the simple way to refactor the code to achieve both uniformity and usability.
I think it’d make sense to just focus on implementing desired behavior this PR, and not do a broad refactoring at the same time. It sounds like the goal is just to get identical translated “Error: Disk space is low!” messages to show up in the log and dialogs. Refactoring is something that can come later, and usually refactoring changes are more reviewable if they don’t change behavior.
I think the easiest way to keep the PR limited would be to define a new CClientUIInterface::MSG_NOPREFIX flag, and have AbortNode pass it to noui_ThreadSafeMessageBox. AbortNode could also pass strprintf(_("Error: %s"), userMessage) instead of userMessage, and you could keep the existing changes in this PR. Maybe a few other AbortNode calls might need to be updated, too.
As for a bigger refactoring, I think Marco’s suggested change #15894 (comment) could be a good idea since it makes BitcoinGUI::message formatting more similar to noui_ThreadSafeMessageBox message formatting.
An alternative would go the opposite direction and make noui_ThreadSafeMessageBox more similar to BitcoinGUI::message, giving callers more control over formatting. If we wanted to do this, it might help to add some utility functions to util/error.h and util/error.cpp that callers could use:
0std::string FormatError(const std::string& message)
1{
2 return strprintf(_("Error: %s"), message);
3}
4
5std::string FormatWarning(const std::string& message)
6{
7 return strprintf(_("Warning: %s"), message);
8}
Logging translated messages to debug.log is a bug (it makes the log useless as universally understood debugging medium)
Steps to reproduce:
0$ ./src/qt/bitcoin-qt -whitelist=asdf -lang=fr -printtoconsole
1
2...
32019-04-30T13:11:46Z nBestHeight = 3045
42019-04-30T13:11:46Z torcontrol thread start
52019-04-30T13:11:46Z Erreur: Masque réseau invalide indiqué dans -whitelist : « asdf »
Logging translated messages to debug.log is a bug (it makes the log useless as universally understood debugging medium)
Does this impact my suggestion above? It seems like currently we log both translated and untranslated messages, and after my suggestion we’d continue to do that. My suggestion is just about making prefixes in translated messages consistent.
More generally, I don’t see any harm in logging translated messages next to untranslated messages. Seems fine if you want to drop translated messages, though.
Just a reminder:
Do not translate internal errors, log messages, or messages that appear on the RPC interface. If an error is to be shown to the user, use a translatable generic message, then log the detailed message to the log. E.g., “A fatal internal error occurred, see debug.log for details”. This helps troubleshooting; if the error is the same for everyone, the likelihood is increased that it can be found using a search engine.
The following comments have been addressed:
… I think ideally the line would appear as “Error: Disk space is low!” in both the GUI and in the log.
Maybe the wording should be more “final” since “low” is eventually something that is non-critical were “too low” would be understandable (together with red error sign).
Maybe add a comment in source to remind us of the macOS situation.
What about prepending the prefix based on the error type?
I think the easiest way to keep the PR limited would be to define a new
CClientUIInterface::MSG_NOPREFIXflag, and haveAbortNodepass it tonoui_ThreadSafeMessageBox.
The result:
1039- else {
1040+ } else {
1041 switch (style) {
1042 case CClientUIInterface::MSG_ERROR:
1043- msgType = tr("Error");
1044+ msgType = tr("ERROR");
1040+ } else {
1041 switch (style) {
1042 case CClientUIInterface::MSG_ERROR:
1043- msgType = tr("Error");
1044+ msgType = tr("ERROR");
1045+ message = tr("ERROR: %1").arg(message);
AbortNode() so it wouldn’t need to have a new argument added. I also would have preferred not to change BitcoinGUI::message so the scope of this PR would be limited to AbortNode calls and not affect every other error dialog. But either way this is ok.
It forces do not prepend error/warning prefix.
1037@@ -1038,49 +1038,51 @@ void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, double nVer
1038 progressBar->setToolTip(tooltip);
1039 }
1040
1041-void BitcoinGUI::message(const QString &title, const QString &message, unsigned int style, bool *ret)
1042+void BitcoinGUI::message(const QString& title, QString message, unsigned int style, bool* ret)
1043 {
1044- QString strTitle = tr("Bitcoin"); // default title
1045+ // Default title. On macOS, the window title is ignored (as required by the macOS Guidelines).
1046+ QString strTitle{PACKAGE_NAME};
PACKAGE_NAMEs?
I’m agree that in the src/qt/ directory the translated PACKAGE_NAME is always used.
How should I fix it when this PR has been merged already? A new PR?
Why is this no longer translated..?
The PACKAGE_NAME is a brand/product name which should not be translated, in my strong opinion.
I’ve read rebranding discussion (#3203, #3400, #3408, …).
Could you point me to a similar discussion regarding translation of the PACKAGE_NAME?
Also, please note the next lines of code
0 <message>
1 <source>Bitcoin Core</source>
2 <translation>Bitcoin Core</translation>
3 </message>
through almost(?) all files in the src/qt/locale/ directory.
Maybe some national traditions force to translate foreign brand names? I don’t know about such traditions.
We should at least be self-consistent.
Right.
1060- else {
1061+ } else {
1062 switch (style) {
1063 case CClientUIInterface::MSG_ERROR:
1064 msgType = tr("Error");
1065+ if (prefix) message = tr("Error: %1").arg(message);
tr("Error")? Seems overkill to burden the translator with a format string.
2924@@ -2923,7 +2925,7 @@ static bool FindUndoPos(CValidationState &state, int nFile, FlatFilePos &pos, un
2925 bool out_of_space;
2926 size_t bytes_allocated = UndoFileSeq().Allocate(pos, nAddSize, out_of_space);
2927 if (out_of_space) {
2928- return AbortNode(state, "Disk space is low!", _("Error: Disk space is low!"));
2929+ return AbortNode(state, "Disk space is too low!", _("Error: Disk space is too low!"), CClientUIInterface::MSG_NOPREFIX);
Please see:
Yeah, I’d say it is shorter to write
0return AbortNode(state, "Disk space is too low!", _("Disk space is too low!"));
which should do the same?
which should do the same?
Not quite.
The difference is whether {“Error: “|“Warning: “:} prefix is translated or not in the debug.log and stderr.
Right. I guess it shouldn’t, but at the current state of this pull request, it is translated.
I presume you are fixing this in #16224 (or similar)?
1393
1394-static bool AbortNode(CValidationState& state, const std::string& strMessage, const std::string& userMessage="")
1395+static bool AbortNode(CValidationState& state, const std::string& strMessage, const std::string& userMessage = "", unsigned int prefix = 0)
1396 {
1397- AbortNode(strMessage, userMessage);
1398+ AbortNode(strMessage, userMessage, prefix);
msg_prefix or something more specific, no?
1380 SetMiscWarning(strMessage);
1381 LogPrintf("*** %s\n", strMessage);
1382- uiInterface.ThreadSafeMessageBox(
1383- userMessage.empty() ? _("Error: A fatal internal error occurred, see debug.log for details") : userMessage,
1384- "", CClientUIInterface::MSG_ERROR);
1385+ if (!userMessage.empty()) {
36- default:
37- strCaption += caption; // Use supplied caption (can be empty)
38+ if (prefix) {
39+ switch (style) {
40+ case CClientUIInterface::MSG_ERROR:
41+ strCaption = "Error: ";