[QT] pretty print (indent) multiline html output #6781

jonasschnelli commented at 9:02 AM on October 8, 2015: contributor

Before: <img width="952" alt="bildschirmfoto 2015-10-08 um 10 55 51" src="https://cloud.githubusercontent.com/assets/178464/10361981/da9b92c4-6dab-11e5-85b5-2cb7199fcd2b.png">

After: <img width="1144" alt="bildschirmfoto 2015-10-08 um 10 54 44" src="https://cloud.githubusercontent.com/assets/178464/10361984/df08dc90-6dab-11e5-8079-3003c19950f5.png">

If the window is to small it will result in line break without indent (as in console): <img width="696" alt="bildschirmfoto 2015-10-08 um 10 57 04" src="https://cloud.githubusercontent.com/assets/178464/10362011/fe877248-6dab-11e5-8938-562c6f9524fe.png">

[QT] pretty print (indent) multiline html output 0daa74b3b5

MarcoFalke commented at 11:54 AM on October 8, 2015: member

Looks good. Tested ACK 0daa74b3b5bcbd22ec17160956dcbe90d80a6ec3. Maybe the font should be set to monospace as well in this PR.

dexX7 commented at 12:52 PM on October 8, 2015: contributor

Thanks @jonasschnelli!

Looks good to me, now it has a similar behavior as the console. @MarcoFalke: the font was changed earlier this year, see #5898.

Personally I find it pretty readable how it is.

MarcoFalke commented at 12:58 PM on October 8, 2015: member

@dexX7 Pretty print without monospace is very limited, though.

dexX7 commented at 1:14 PM on October 8, 2015: contributor

Sorry, my bad! Now I see where you were going. On the first glimpse I assumed the help message was just badly formatted, but the messed up indentation is caused by the font.

Currently (with this PR):

nofont

When reverting #5898:

monospace

btcdrak commented at 1:25 PM on October 8, 2015: contributor

ACK, nice work.

jonasschnelli commented at 1:37 PM on October 8, 2015: contributor

Hmm... we could set the monospace font for json responses only? Or would this look ugly?

MarcoFalke commented at 2:32 PM on October 8, 2015: member

No for json only.

What about something like

diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
index f387a3e..396fe54 100644
--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -465,6 +465,7 @@ void RPCConsole::clear()
     ui->messagesWidget->document()->setDefaultStyleSheet(
                 "table { }"
                 "td.time { color: [#808080](/bitcoin-bitcoin/808080/); padding-top: 3px; } "
+                "td.message { font-family: monospace; }"
                 "td.cmd-request { color: [#006060](/bitcoin-bitcoin/006060/); } "
                 "td.cmd-error { color: red; } "
                 "b { color: [#006060](/bitcoin-bitcoin/006060/); } "

This will cause the font to be too large in Ubuntu?

dexX7 commented at 3:06 PM on October 8, 2015: contributor

Hm.. so as far as I can see a monospaced font is desired, but the issue was that the default on Ubuntu was too big, and < 1em doesn't work, right?

Maybe it would be possible to retrieve the default font size and use this, or an adjusted size based on this size, for the stylesheet, assuming the default size equals 1em?

E.g.:

// 1.0 em of default font:
QFont().pointSize()

// 0.8 em of the Monospace font:
QFont("Monospace").pointSize() * 4 / 5

(pixelSize() is -1 for me for some reason)

Monospace with 0.8em looks not bad on Ubuntu imho (same as 12px fixed):

To compare without adjusted size, but Monospace:

I'm not sure how this plays out for other OS though, and slightly related: Monospace isn't necessarily cross-plattform compatible, see here on stackoverflow.com.

Edit: this is what I tested: https://gist.github.com/dexX7/218beb74246a1b5f0a6f

jonasschnelli commented at 3:40 PM on October 8, 2015: contributor

I also like the monospace look in Ubuntu.

But I think this is controversial (IIRC there are also HiDPI issues) and would recommend to address it in a different PR.

paveljanik commented at 4:04 PM on October 8, 2015: contributor

It is difficult, because this PR migrates the look from bad to less bad. But once finally solved (using proper font), it is required anyway, so ACK.

laanwj added the label GUI on Oct 8, 2015

laanwj commented at 3:05 PM on October 19, 2015: member

I think this looks fine. I don't think monospace/fixed is necessary, and I remember some problems with those in the past.

fanquake commented at 2:16 AM on October 24, 2015: member

utACK

laanwj commented at 8:06 AM on October 26, 2015: member

Does #6864 make this unnecessary?

in src/qt/guiutil.cpp:None in 0daa74b3b5

 242 | @@ -243,6 +243,7 @@ QString HtmlEscape(const QString& str, bool fMultiLine)
 243 |      if(fMultiLine)
 244 |      {
 245 |          escaped = escaped.replace("\n", "<br>\n");
 246 | +        escaped = escaped.replace(" ", "&nbsp;");

laanwj commented at 8:08 AM on October 26, 2015:

I think it's slightly unexpected for this to be part of the responsibility of a function called HtmlEscape. Especially to do this only on fMultiLine - there's nothing inherently multiline about replacing spaces. If this is still necessary with monospace - maybe add a separate flag for this step?

laanwj commented at 11:47 AM on October 26, 2015:

Related: I suppose that changing the spaces to non-breaking ones cause problems when pasting the output into a json file and trying to parse it with a json parser? E.g. according to RFC4627, %xa0 is not a valid whitespace character:

      ws = *(
                %x20 /              ; Space
                %x09 /              ; Horizontal tab
                %x0A /              ; Line feed or New line
                %x0D                ; Carriage return
            )

(not saying this is a huge issue, not many people will likely do this, but it's something to keep in mind when considering presentation versus representation)

laanwj commented at 11:55 AM on October 26, 2015:

Can we achieve the same, non-collapsing spaces, using css' whitespace property? http://www.w3schools.com/cssref/pr_text_white-space.asp . Or is this not supported by Qt?

MarcoFalke commented at 8:20 AM on October 27, 2015:

@laanwj Yes, see #6864

MarcoFalke commented at 8:19 AM on October 26, 2015: member

~~@laanwj #6864 is based on this one for convenience.~~ @laanwj #6864 replaces this PR

laanwj commented at 11:46 AM on October 29, 2015: member

Closing this one then

laanwj closed this on Oct 29, 2015

MarcoFalke locked this on Sep 8, 2021