utACK ad2b70aa26cecbcc42c305401855212de9b3c26c

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #16224 (gui: Bilingual GUI error messages by hebasto)
• #15921 (Tidy up ValidationState interface by jnewbery)
• #13674 (Qt: Fix for bitcoin-qt becoming unresponsive during shutdown (issue [GUI] QT shutdown is blocked if called before gettxoutsetinfo is finished #13217) by LeandroRocha84)

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.

utACK ad2b70a, that was annoying me too. @ryanofsky I don't think the UI dialog text needs a "Error: " prefix. The dialog title and giant red icon make it clear enough that this is an error.

On macos the message box title is missing, should fix that at least?

@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:

• platform-dependent QMessageBox text:
  • "Error: Message" on macOS
  • "Message" on other platforms
• do not touch GUI code at all

Which one is more preferable?

I agree with @ryanofsky, having equal messages isn't that terrible, especially when it's about to quit.

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).

Maybe add a comment in source to remind us of the macOS situation.

What about prepending the prefix based on the error type?

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index abf9136eee..851bfe2b71 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -1022,9 +1022,9 @@ void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, double nVer
     progressBar->setToolTip(tooltip);
 }
 
-void BitcoinGUI::message(const QString &title, const QString &message, unsigned int style, bool *ret)
+void BitcoinGUI::message(const QString& title, QString message, unsigned int style, bool* ret)
 {
-    QString strTitle = tr("Bitcoin"); // default title
+    QString strTitle{PACKAGE_NAME}; // default title
     // Default to information icon
     int nMBoxIcon = QMessageBox::Information;
     int nNotifyIcon = Notificator::Information;
@@ -1039,20 +1039,24 @@ void BitcoinGUI::message(const QString &title, const QString &message, unsigned
         switch (style) {
         case CClientUIInterface::MSG_ERROR:
             msgType = tr("Error");
+            message = tr("Error") + ": " + message;
             break;
         case CClientUIInterface::MSG_WARNING:
             msgType = tr("Warning");
+            message = tr("Warning") + ": " + message;
             break;
         case CClientUIInterface::MSG_INFORMATION:
             msgType = tr("Information");
+            message = tr("Information") + ": " + message;
             break;
         default:
             break;
         }
     }
-    // Append title to "Bitcoin - "
-    if (!msgType.isEmpty())
+    // Append type to title
+    if (!msgType.isEmpty()) {
         strTitle += " - " + msgType;
+    }
 
     // Check for error/warning icon
     if (style & CClientUIInterface::ICON_ERROR) {
diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h
index b58ccbb455..54a4b3903c 100644
--- a/src/qt/bitcoingui.h
+++ b/src/qt/bitcoingui.h
@@ -219,7 +219,7 @@ public Q_SLOTS:
                             @see CClientUIInterface::MessageBoxFlags
        @param[in] ret       pointer to a bool that will be modified to whether Ok was clicked (modal only)
     */
-    void message(const QString &title, const QString &message, unsigned int style, bool *ret = nullptr);
+    void message(const QString& title, QString message, unsigned int style, bool* ret = nullptr);
 
 #ifdef ENABLE_WALLET
     void setCurrentWallet(WalletModel* wallet_model);

re: #15894 (comment)

message = tr("Error") + ": " + message;

Formatting could be part of the translation:

message = tr("Error: %s", message)

Currently, Bitcoin Core code has the following features.

The ThreadSafeMessageBox() messages are rendered in two ways:

• by noui_ThreadSafeMessageBox() using LogPrintf() and fprintf(stderr, ...) with
  adding translated type prefix ("Error" / "Warning" / "Information")
• by 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: " prefix
• FatalError() passes non translated messages with "Error: " prefix
• InitError(); some passed messages are translated, some are not
• InitWarning() passes translated messages
• InitHTTPAllowList() passes non-translated messages
• CCoinsViewErrorCatcher::GetCoin() pass translated message started with "Error"
• CConnman::BindListenPort() via CConnman::Bind() passes:
  • non translated messages with "Error: " prefix
  • translated messages without any prefix
  • translated messages with "Error: " prefix
• CConnman::Start() passes translated messages
• AddTimeData() passes translated message

Unfortunately, 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:

std::string FormatError(const std::string& message)
{
    return strprintf(_("Error: %s"), message);
}

std::string FormatWarning(const std::string& message)
{
    return strprintf(_("Warning: %s"), message);
}

Logging translated messages to debug.log is a bug (it makes the log useless as universally understood debugging medium)

Steps to reproduce:

$ ./src/qt/bitcoin-qt -whitelist=asdf -lang=fr -printtoconsole

...
2019-04-30T13:11:46Z nBestHeight = 3045
2019-04-30T13:11:46Z torcontrol thread start
2019-04-30T13:11:46Z Erreur: Masque réseau invalide indiqué dans -whitelist : « asdf »

@MarcoFalke
Agree. There was a dedicated PR #15340.

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:

Translation Strings Policy:

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:

• Remove duplicated "Error: " prefix in logs #15894 (review) by @ryanofsky

... I think ideally the line would appear as "Error: Disk space is low!" in both the GUI and in the log.

• Remove duplicated "Error: " prefix in logs #15894 (comment) by @jonasschnelli

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).

• Remove duplicated "Error: " prefix in logs #15894 (comment) by @Sjors

Maybe add a comment in source to remind us of the macOS situation.

• Remove duplicated "Error: " prefix in logs #15894 (comment) by @MarcoFalke

What about prepending the prefix based on the error type?

• Remove duplicated "Error: " prefix in logs #15894 (comment) by @ryanofsky

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.

The result:

Rebased.

concept and code-review ACK f724f31

@MarcoFalke Thank you for your review. I could see my code from another point of view.