Skip to content

Conversation

@jonasschnelli
Copy link
Contributor

Before:
bildschirmfoto 2015-10-08 um 10 55 51

bildschirmfoto 2015-10-08 um 10 55 55

After:
bildschirmfoto 2015-10-08 um 10 54 44

bildschirmfoto 2015-10-08 um 10 56 32

If the window is to small it will result in line break without indent (as in console):
bildschirmfoto 2015-10-08 um 10 57 04

bildschirmfoto 2015-10-08 um 11 02 07

@maflcko
Copy link
Member

maflcko commented Oct 8, 2015

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

@dexX7
Copy link
Contributor

dexX7 commented Oct 8, 2015

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.

@maflcko
Copy link
Member

maflcko commented Oct 8, 2015

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

@dexX7
Copy link
Contributor

dexX7 commented Oct 8, 2015

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
Copy link
Contributor

btcdrak commented Oct 8, 2015

ACK, nice work.

@jonasschnelli
Copy link
Contributor Author

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

@maflcko
Copy link
Member

maflcko commented Oct 8, 2015

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; padding-top: 3px; } "
+                "td.message { font-family: monospace; }"
                 "td.cmd-request { color: #006060; } "
                 "td.cmd-error { color: red; } "
                 "b { color: #006060; } "

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

@dexX7
Copy link
Contributor

dexX7 commented Oct 8, 2015

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

08

To compare without adjusted size, but Monospace:

10

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
Copy link
Contributor Author

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
Copy link
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 laanwj added the GUI label Oct 8, 2015
@laanwj
Copy link
Member

laanwj commented Oct 19, 2015

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

@fanquake
Copy link
Member

utACK

@maflcko maflcko mentioned this pull request Oct 24, 2015
@laanwj
Copy link
Member

laanwj commented Oct 26, 2015

Does #6864 make this unnecessary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laanwj Yes, see #6864

@maflcko
Copy link
Member

maflcko commented Oct 26, 2015

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

@laanwj #6864 replaces this PR

@laanwj
Copy link
Member

laanwj commented Oct 29, 2015

Closing this one then

@laanwj laanwj closed this Oct 29, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants