gui: Display warnings as rich text #18898

hebasto commented at 12:35 PM on May 6, 2020: member

On master (6621be53517d69ab855cee4a5978a44d6a133ba3), warnings that contain <hr /> HTML tag are not displayed correctly:

Screenshot from 2020-05-06 11-30-10

Fixed:

Screenshot from 2020-05-07 07-30-48

fanquake added the label GUI on May 6, 2020

in src/qt/bitcoin.cpp:138 in 1b87b55f08 outdated

 134 | @@ -135,7 +135,7 @@ BitcoinCore::BitcoinCore(interfaces::Node& node) :
 135 |  void BitcoinCore::handleRunawayException(const std::exception *e)
 136 |  {
 137 |      PrintExceptionContinue(e, "Runaway exception");
 138 | -    Q_EMIT runawayException(QString::fromStdString(m_node.getWarnings()));
 139 | +    Q_EMIT runawayException(QString::fromStdString(m_node.getWarnings()).replace("<hr />", "\n\n"));

vasild commented at 1:02 PM on May 6, 2020:

So we have

https://github.com/bitcoin/bitcoin/blob/88b2652fadf6e004e751d48884ae8d4cf5c452b8/src/warnings.h#L17

https://github.com/bitcoin/bitcoin/blob/88b2652fadf6e004e751d48884ae8d4cf5c452b8/src/warnings.cpp#L45

Would it be better to extract the warning_separator variable into a global constant and use it here instead of hardcoding "<hr />" in both places?

Even better would be to return a vector of strings instead of multiple strings concatenated to each other with "<hr />", but that would be a much bigger change, I am not suggesting to do it here in this PR.

hebasto commented at 1:30 PM on May 6, 2020:

... a much bigger change...

That was my concern too :)

promag commented at 11:03 PM on May 6, 2020:

How about drop warning_separator and use std::endl?

AFAICT this <hr /> was introduced in #7579.

hebasto commented at 4:51 AM on May 7, 2020:

Reworked.

vasild commented at 1:07 PM on May 6, 2020: member

Should the following be adjusted too?

https://github.com/bitcoin/bitcoin/blob/88b2652fadf6e004e751d48884ae8d4cf5c452b8/src/qt/clientmodel.cpp#L145

hebasto commented at 1:28 PM on May 6, 2020: member

@vasild

Should the following be adjusted too?

https://github.com/bitcoin/bitcoin/blob/88b2652fadf6e004e751d48884ae8d4cf5c452b8/src/qt/clientmodel.cpp#L145

No. It is for status bar: Screenshot from 2020-05-06 11-32-41

0xB10C commented at 3:59 PM on May 6, 2020: member

Is there a specific way to trigger a runaway exception?

hebasto commented at 4:02 PM on May 6, 2020: member

Is there a specific way to trigger a runaway exception?

By mutating the code :) E.g., #18643#issue-599952770

Empact commented at 9:00 PM on May 6, 2020: member

Concept ACK

Took a crack at an alternative and it seems reasonable to me: https://github.com/bitcoin/bitcoin/compare/master...Empact:200506-no-html

qt: Display warnings as rich text a9d28afe23

hebasto force-pushed on May 7, 2020

hebasto commented at 4:50 AM on May 7, 2020: member

Updated 1b87b55f08ed11e5aba63e49397cf294aaf3ff8f -> a9d28afe23a94efdccc53f9f10716f3a0c9337eb (pr18898.01 -> pr18898.02, diff):

reworked to the minimal diff

The OP has been updated.

hebasto renamed this:
~~gui: Drop HTML tags in exception messages~~
gui: Display warnings as rich text
on May 8, 2020

hebasto closed this on Jun 2, 2020

hebasto reopened this on Jun 2, 2020

hebasto commented at 10:45 AM on June 2, 2020: member

Reopened to update Travis CI build list.

hebasto closed this on Jun 2, 2020

hebasto reopened this on Jun 2, 2020

jonasschnelli commented at 12:38 PM on June 2, 2020: contributor

Does simply replacing the newlines at the end with a HTML   fix the <tr/> problem?

hebasto commented at 12:45 PM on June 2, 2020: member

@jonasschnelli

Does simply replacing the newlines at the end with a HTML   fix the <tr/> problem?

Sorry. Mind rewording your question?

jonasschnelli commented at 12:48 PM on June 2, 2020: contributor

Sorry. Mind rewording your question?

Sorry if it was not clear. You're only changing the \n to  . You write in the PR description that is solves the <hr /> issue (display a line instead of the plaintext html tag). I wonder why adding a   should make it rich text. Can you explain?

hebasto commented at 1:04 PM on June 2, 2020: member

@jonasschnelli

You're only changing the \n to  . You write in the PR description that is solves the <hr /> issue (display a line instead of the plaintext html tag). I wonder why adding a   should make it rich text. Can you explain?

From QMessageBox class docs:

The main text and informative text properties can be either plain text or rich text. These strings are interpreted according to the setting of the text format property. The default setting is auto-text.

As we do not have means to set Qt::RichText instead of Qt::AutoText directly, replacing s/\n/ / forces the text string to be interpreted as a rich text.

promag commented at 1:10 PM on June 2, 2020: member

Also https://doc.qt.io/qt-5/qt-sub-qtgui.html#mightBeRichText.

jonasschnelli commented at 1:17 PM on June 2, 2020: contributor

Now I understand. utACK a9d28afe23a94efdccc53f9f10716f3a0c9337eb

promag commented at 3:17 PM on June 2, 2020: member

Code review ACK a9d28afe23a94efdccc53f9f10716f3a0c9337eb.

MarcoFalke merged this on Jun 8, 2020

MarcoFalke closed this on Jun 8, 2020

hebasto deleted the branch on Jun 8, 2020

sidhujag referenced this in commit ebf9600adf on Jun 8, 2020

Fabcien referenced this in commit 24178a7b85 on Aug 25, 2021

DrahtBot locked this on Feb 15, 2022