MarcoFalke commented at 10:58 AM on October 21, 2015: member

This uses #4228 to find a monospace font for the qt rpcconsole.

(This replaces, thus closes #6781.)

in src/qt/rpcconsole.cpp:None in 780aa5c434 outdated

 462 | @@ -463,13 +463,15 @@ void RPCConsole::clear()
 463 |  
 464 |      // Set default style sheet
 465 |      ui->messagesWidget->document()->setDefaultStyleSheet(
 466 | +        QString::fromStdString(strprintf(

laanwj commented at 11:12 AM on October 21, 2015:

Please use Qt's formatting, instead of strprintf. e.g.

QString("....\1...." ).arg(...)

laanwj added the label GUI on Oct 21, 2015

MarcoFalke force-pushed on Oct 21, 2015

jonasschnelli commented at 1:19 PM on October 21, 2015: contributor

Binaries are here: https://bitcoin.jonasschnelli.ch/pulls/6864/

Would be good if we get some screenshot from HiDPI Ubuntu and retina OSX.

in src/qt/rpcconsole.cpp:None in 98c66cd797 outdated

 470 |                  "td.cmd-request { color: #006060; } "
 471 |                  "td.cmd-error { color: red; } "
 472 |                  "b { color: #006060; } "
 473 | -                );
 474 | -
 475 | +            ).arg(QFont("Monospace").pointSize() * 4 / 5) // 0.8 em

Diapolo commented at 7:56 PM on October 21, 2015:

QFont("Monospace").pointSize() * 4 / 5 is a float, so should this also include a call to QString & QString::setNum(float n, char format = 'g', int precision = 6)?

Also not sure if relevant, but as per the docs .pointSize(): Returns the point size of the font. Returns -1 if the font size was specified in pixels.

Perhaps you also could exchange Monospace with QFont::Monospace... I didn't try the code btw.

MarcoFalke commented at 8:32 PM on October 21, 2015:

Thanks, I have alrady replaced the code with a version that should work on all platforms (and maybe even HiDPI).

in src/qt/rpcconsole.cpp:None in 98c66cd797 outdated

 850 | @@ -849,4 +851,4 @@ void RPCConsole::showOrHideBanTableIfRequired()
 851 |      bool visible = clientModel->getBanTableModel()->shouldShow();
 852 |      ui->banlistWidget->setVisible(visible);
 853 |      ui->banHeading->setVisible(visible);
 854 | -}
 855 | \ No newline at end of file
 856 | +}

Diapolo commented at 7:57 PM on October 21, 2015:

Nit: Unwanted change

in src/qt/rpcconsole.cpp:None in d0e439936a outdated

 460 | @@ -461,15 +461,30 @@ void RPCConsole::clear()
 461 |                      platformStyle->SingleColorImage(ICON_MAPPING[i].source).scaled(ICON_SIZE, Qt::IgnoreAspectRatio, Qt::SmoothTransformation));
 462 |      }
 463 |  
 464 | +    QFont fontMatch("Monospace");

Diapolo commented at 8:38 PM on October 21, 2015:

What about the QFont::Monospace hint from a few minutes ago, seems clearer?

MarcoFalke commented at 8:46 PM on October 21, 2015:

May not work according to https://stackoverflow.com/questions/1468022/how-to-specify-monospace-fonts-for-cross-platform-qt-applications#comment28709540_1835938

in src/qt/rpcconsole.cpp:None in d0e439936a outdated

 483 |                  "td.cmd-request { color: #006060; } "
 484 |                  "td.cmd-error { color: red; } "
 485 |                  "b { color: #006060; } "
 486 | -                );
 487 | -
 488 | +            ).arg(fi.family(), "10pt")

dexX7 commented at 11:04 AM on October 22, 2015:

Doesn't a fixed font size bring back the issue addressed via #5898?

jonasschnelli commented at 11:06 AM on October 22, 2015:

IIRC the issue from #5898 was mainly the px based font size. Switching back to "Monospace" could be possible, but needs broad testing and also acceptance.

MarcoFalke commented at 11:38 AM on October 22, 2015:

@jonasschnelli Agree on your comment. Would you mind shooting up some new builds for this PR? =)

in src/qt/rpcconsole.cpp:None in d0e439936a outdated

 460 | @@ -461,15 +461,30 @@ void RPCConsole::clear()
 461 |                      platformStyle->SingleColorImage(ICON_MAPPING[i].source).scaled(ICON_SIZE, Qt::IgnoreAspectRatio, Qt::SmoothTransformation));
 462 |      }
 463 |  
 464 | +    QFont fontMatch("Monospace");
 465 | +    fontMatch.setStyleHint(QFont::TypeWriter);
 466 | +    QFontInfo fi(fontMatch);
 467 | +    assert(fi.fixedPitch());

dexX7 commented at 11:05 AM on October 22, 2015:

I personally think using an assertion in non-consensus code is too "strong".

MarcoFalke commented at 11:39 AM on October 22, 2015:

Just ignore everything i put in the {//Debug} block for code review. ;)

MarcoFalke force-pushed on Oct 22, 2015

laanwj commented at 3:49 PM on October 22, 2015: member

Looks good here. There used to be problems with (absurdly large) font size in Ubuntu with previous monospace patches, but I'm not seeing that now. schermafdruk van 2015-10-22 17 45 27 Can't test hidpi though...

jonasschnelli commented at 6:32 PM on October 22, 2015: contributor

Kicked the build server,... new builds should be available within the next 30mins: https://bitcoin.jonasschnelli.ch/pulls/6864/ (reminds me to finally complete my IRC-gitian-build-bot and collect some devs pub keys to allow dev-build-sig-checks)

jonasschnelli commented at 7:31 PM on October 22, 2015: contributor

Ubuntu/Linux HiDPI looks ugly,.. but not related to this PR. Fine for me: <img width="1302" alt="bildschirmfoto 2015-10-22 um 21 27 29" src="https://cloud.githubusercontent.com/assets/178464/10676095/44df62e2-7904-11e5-9976-5b8dad3c4bc3.png">

Crash on OSX:

jonasschnelli$ ~/Desktop/bitcoin-qt --regtest
Assertion failed: (fixedFontInfo.fixedPitch()), function clear, file qt/rpcconsole.cpp, line 470.

MarcoFalke commented at 8:35 PM on October 22, 2015: member

To properly support HiDPI, I think we need to do the switch to qt5.

Windows works now, as well. (Courier New 10pt)

win-7e7d299

MarcoFalke force-pushed on Oct 23, 2015

laanwj commented at 8:34 AM on October 23, 2015: member

To properly support HiDPI, I think we need to do the switch to qt5.

Supporting HiDPI is not a requirement for Qt4. All the distribution binaries are built with Qt5. We still support Qt4 in the same way as websites support IE6, e.g. bare-bones: functionality must work but it is allowed to be ugly.

luke-jr commented at 8:40 AM on October 23, 2015: member

Uh, Qt4 should remain a fully-supported platform until KDE has finished switching away from it... (this case would be an exception IMO since Qt4 itself lacks HiDPI support.)

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

@jonasschnelli Your OS X binaries are crashing for me, are you having any issue with them?

jonasschnelli commented at 8:41 AM on October 24, 2015: contributor

@fanquake: the crash is related to the PR. Monospace is not available on Mac (or something similar).

jonasschnelli commented at 9:12 AM on October 24, 2015: contributor

@fanquake: you probably picked an old build. Always check the git tip. Just re-started the build of the PR.

MarcoFalke force-pushed on Oct 24, 2015

MarcoFalke commented at 5:47 PM on October 24, 2015: member

This is ready for review now. @fanquake and reviewers: Please use jonasschnelli's binaries for debug (send me stdout when no monospace font is found).

The code in this PR has the debug to stdout removed in 1d15e0e762cdfa83d4f98d96e532e35c0f8f6cf4 , was squashed and force pushed.

dexX7 commented at 8:58 PM on October 24, 2015: contributor

On the first glimpse (general UI, address manager, debug console) it looks good on Win10:

mono

jonasschnelli commented at 4:17 PM on October 26, 2015: contributor

I don't have a fixed space font on OSX. Also the font is relatively small now.

This PR: <img width="1190" alt="bildschirmfoto 2015-10-26 um 17 16 46" src="https://cloud.githubusercontent.com/assets/178464/10734817/6c815072-7c05-11e5-8855-8fe3f0681257.png">

Current ~Master: <img width="1190" alt="bildschirmfoto 2015-10-26 um 09 26 24" src="https://cloud.githubusercontent.com/assets/178464/10734830/76316bc0-7c05-11e5-8124-bd25c9cc3dec.png">

dexX7 commented at 9:39 PM on October 26, 2015: contributor

Also the font is relatively small now.

In this case I wouldn't further increase the size, because then it probably gets too big on the other OS, but I agree, it looks too small.

While this doesn't seem to be an issue with fixed space font (especially since it's not available on OS X, as it seems), it may be worth to consider using a relative size based on a common, well sized font as reference, i.e. instead of using 10pt:

retrieve actual font size of "reference font"
use the reference size as size for the console font (whatever font it is)

MarcoFalke force-pushed on Oct 27, 2015

MarcoFalke commented at 8:09 AM on October 27, 2015: member

@dexX7 If there was a "reference font", you could as well just not set the pt size at all, I guess. Let's try this?

MarcoFalke force-pushed on Oct 27, 2015

laanwj commented at 9:16 AM on October 27, 2015: member

Latest changes have brought back my original issue with monospace console: childishly large letters on Ubuntu screenshot0 (just compare with above screenshot, or the default terminal font in the background)

dexX7 commented at 10:37 AM on October 27, 2015: contributor

@MarcoFalke: If there was a "reference font", you could as well just not set the pt size at all, I guess. Let's try this?

I may be completely wrong, but font size and font type seems to be coupled, and a mono space font appears to be bigger than a "standard" font without explicit size, so we'd need to set a size nevertheless.

But there are several UI elements, which already look good on all OS, whether it's the timestamp in the RPC console, the balances on the overview tab, or other text elements.

So what if the actual size of the "reference font" is used explicitly?

I haven't tested the following, but just to give you an idea of what I was thinking about::

     // Set default style sheet
+    QFontInfo fixedFontInfo(GUIUtil::fixedPitchFont());
     ui->messagesWidget->document()->setDefaultStyleSheet(
+        QString(
                 "table { }"
                 "td.time { color: [#808080](/bitcoin-bitcoin/808080/); padding-top: 3px; } "
+                "td.message { font-family: %1; font-size: %2pt, white-space:pre-wrap; } "
                 "td.cmd-request { color: [#006060](/bitcoin-bitcoin/006060/); } "
                 "td.cmd-error { color: red; } "
                 "b { color: [#006060](/bitcoin-bitcoin/006060/); } "
-                );
+            ).arg(fixedFontInfo.family()).arg(QFont().pointSize()) // size of "reference font"
+        );

QFont() may be replaced with something else, in case another font is used as "reference".

dexX7 commented at 1:17 PM on October 27, 2015: contributor

Hmm.. I'm just testing it on Ubuntu now, and it seems to work. However, there is quite a difference in the appearance depending on the actual font, even if the font size is similar.

When using QFont().family() (basically what was used before this PR):

ubuntu

When using GUIUtil::fixedPitchFont().family():

fixed

Here is another very interesting idea, which I stumbled upon while looking for a solution for OS X (via http://stackoverflow.com/a/1471899):

Instead of manually finding a monospaced font and size, what if the <pre> HTML tag is used instead? It should definitely work cross-plattform, and it should have the properties we want: a fixed-width font and preservation of white-spaces. As per default, line breaks are also preserved, which may, or may not be desired:

Without line breaks (http://pastebin.com/nmWbwYpT):

pre

With line breaks (http://pastebin.com/4c29AdNL):

pre_wrap

While this probably needs some tuning, I believe going the <pre></pre> route is more reliable and straight forward.

laanwj commented at 10:41 AM on October 28, 2015: member

Unfortunately, pre seems to have a different font, but also very large font size on Ubuntu (looks even larger than what was tried before..).

jonasschnelli commented at 10:55 AM on October 28, 2015: contributor

Hmm... I question myself: is this worth the effort? Why not just using "Courier" which should be available on any OS and would be similar to browsers showing code.

jonasschnelli commented at 11:09 AM on October 28, 2015: contributor

Or, if more control is desirable: add a monospace otf font (one with Apache or MIT license) to the qrc file and add it to the QFontManager. Fonts are small, I'll guess ~30kb.

dexX7 commented at 11:47 AM on October 28, 2015: contributor

The default setting on Ubuntu for fixed-width fonts is Ubuntu Mono with a size of 13 (which is larger than the others with 11), if I'm not mistaken.

I guess it comes down to where this should go, and whether the font (and size) should be defined by the application or externally.

Why not just using "Courier" which should be available on any OS

This is pretty straight forward, and I tend to believe Bitcoin Core should look shiny out of the box, without messing with the underlying system, so +1, this is probably worth a try.

MarcoFalke force-pushed on Oct 28, 2015

MarcoFalke commented at 6:51 PM on October 28, 2015: member

@jonasschnelli I agree this is getting too messy. But I'd rather not hard code a font or include one right now. If there is a problem with fixed font, qt and OSX this is a greater problem and should be addressed in a follow up PR.

Maybe you retrigger the builds once more and check if the font has the correct size. I will check the other OSs. If the size is ok, let's merge this. Otherwise I better close this and open a different PR with a new approach.

jonasschnelli commented at 9:40 AM on October 29, 2015: contributor

Rebuit: https://bitcoin.jonasschnelli.ch/pulls/6864/

dexX7 commented at 12:31 PM on October 29, 2015: contributor

Looks good to me. Hopefully it works on OS X.

Windows 10:

win10

Ubuntu:

ubuntu

jonasschnelli commented at 12:54 PM on October 29, 2015: contributor

Seems not to work on OSX: Console prints:

Error: No monospace font found! Please report!
family: Lucida Grande
pixelS: 12
pointS: 12
pointS: (QFont()) 13
2015-10-29 12:53:49

MarcoFalke commented at 1:34 PM on October 29, 2015: member

Ok, then. Can we try to tackle the OSX error: no monospace found in another PR?

At least it seems for all tested OS the font size is correct and the white space is preserved. Only OSX can't find a fixed pitch font but this is already the behavior bitcoin/master. So nothing gets worse if I squash the commits and this gets merged?

jonasschnelli commented at 1:38 PM on October 29, 2015: contributor

As soon as the std::out debug infos are gone (maybe remove entirely or LogPrintf) i would ACK this PR (improvement for Windows/Linux).

[qt] Use fixed pitch font for the rpc console

Also:
* Preserve white space
* Make fixed font as large as default font

28313b83fc

MarcoFalke force-pushed on Oct 29, 2015

MarcoFalke commented at 5:29 PM on October 29, 2015: member

@jonasschnelli Lucida Grande can be monospace. According to your screenshot it also looks monospace but I am not sure why QT does not detect it as such...

For reference the binaries were built with eb475c0.

I removed the logging again and force pushed a squash.

MarcoFalke commented at 5:35 PM on October 29, 2015: member

If no one complains, this should be merged.

More screenshots:

Windows

nightly
this PR

Fedora

nightly
this PR

Fedora

8daffe2
28313b8

laanwj commented at 3:41 AM on November 2, 2015: member

Still has a crazily big font on Ubuntu. Tending towards closing this, it's not worth the hassle.

MarcoFalke commented at 1:09 PM on November 2, 2015: member

@laanwj Unfortunately I can not reproduce the issue you mentioned. I did a fresh install of Ubuntu 15.10 and got the results below. Also, @dexX7 could not find an issue for Ubuntu. Maybe you changed the default settings for the system fonts accidentally?

$ gsettings list-recursively  org.gnome.desktop.interface|egrep "(font|text-scaling)"
org.gnome.desktop.interface monospace-font-name 'Ubuntu Mono 13'
org.gnome.desktop.interface text-scaling-factor 1.0
org.gnome.desktop.interface font-name 'Ubuntu 11'
org.gnome.desktop.interface document-font-name 'Sans 11'

Ubuntu

Nightly

nigh-ubu

This PR

mono-ubu

OSX

(screenshots from @jonasschnelli)

Nightly

<img width="1190" alt="76316bc0-7c05-11e5-8124-bd25c9cc3dec" src="https://cloud.githubusercontent.com/assets/6399679/10882080/1eb2e6d6-8168-11e5-8a7f-dd7c6de3211d.png"> - This PR

laanwj commented at 1:24 PM on November 2, 2015: member

Looks like it's the same:

$ gsettings list-recursively  org.gnome.desktop.interface|egrep "(font|text-scaling)"
org.gnome.desktop.interface monospace-font-name 'Ubuntu Mono 13'
org.gnome.desktop.interface text-scaling-factor 1.0
org.gnome.desktop.interface font-name 'Ubuntu 11'
org.gnome.desktop.interface document-font-name 'Sans 11'

Have tried it on two systems. I'm using Ubuntu 14.04 and Qt5. I compile using the system's Qt5 library and am not using depends. It is curious that 15.10 does display it in a normal font so maybe it's an Ubuntu issue that was fixed but not in LTS.

Edit: tried with Qt4, same problem. I'll try with a 15.10 VM.

laanwj commented at 3:48 PM on November 2, 2015: member

15.10 - fresh install. What am I doing wrong?

Before: untitled

After: screenshot from 2015-11-02 16 46 33

MarcoFalke commented at 3:52 PM on November 2, 2015: member

@laanwj Thank you for trying this. Would you mind to edit your posts to include the screenshots for the build without this PR?

laanwj commented at 4:12 PM on November 2, 2015: member

Added for the 15.10 VM above: for the others the effect is exactly the same, except for the code changes as they were at the beginning of this pull request, there the fixed and proportional font were almost the same size.

MarcoFalke commented at 5:23 PM on November 2, 2015: member

[...] beginning of this pull request, there the fixed and proportional font were almost the same size.

Yes, it was "hard-coded" at 10pt but @jonasschnelli complained 10pt won't work in OS X, IIRC.

laanwj commented at 6:34 AM on November 3, 2015: member

So it looks like Ubuntu consistently specifies a huge monospace default font. I guess that's simply the Ubuntu Mono 13 from qsettings? That's kind of annoying. I just never noticed on other programs - there are only few programs that use monospace besides the terminal, which seems to choose its own.

The only thing I don't understand is that you don't get the same.

jonasschnelli commented at 9:12 AM on November 3, 2015: contributor

Tested ACK.

Ubuntu: <img width="492" alt="bildschirmfoto 2015-11-03 um 10 05 54" src="https://cloud.githubusercontent.com/assets/178464/10904301/8580eee4-8212-11e5-816d-e392deec1920.png"> <img width="479" alt="bildschirmfoto 2015-11-03 um 10 05 44" src="https://cloud.githubusercontent.com/assets/178464/10904302/85be809c-8212-11e5-8c6e-fbfd6d46684b.png">

Ubuntu HiDPI: <img width="1178" alt="bildschirmfoto 2015-11-03 um 10 03 39" src="https://cloud.githubusercontent.com/assets/178464/10904310/94e04362-8212-11e5-9cb3-def7d9e9ca23.png">

OSX: <img width="1190" alt="bildschirmfoto 2015-11-03 um 10 04 40" src="https://cloud.githubusercontent.com/assets/178464/10904315/9bb652da-8212-11e5-87d9-1f550404b344.png">

jonasschnelli commented at 9:13 AM on November 3, 2015: contributor

Maybe someone can test this over a Qt4 build? (@luke-jr)?

[qt] rpcconsole: Scale monospace font to 95% 268b79ef0c

laanwj commented at 11:37 AM on November 3, 2015: member

I've already tested Qt4. Works OK.

MarcoFalke commented at 11:44 AM on November 3, 2015: member

I guess that's simply the Ubuntu Mono 13 from qsettings?

No, qt selects "DejaVu Sans Mono 12", and I set the size to QFontInfo(QFont()).pointSize(), which is less than 12pt (e.g. 10pt or 9pt) but equal to the font height we had before for the rpc console.

the terminal, which seems to choose its own.

No, terminal chooses the default "Ubuntu Mono 13".

The only thing I don't understand is that you don't get the same.

Maybe, there is a difference when you use the gitian builds by jonasschnelli ( 9pt) and your own build (10pt). Probably, because gitian uses qt5 and you are using qt4? @laanwj The monospace font appears larger than the non monospace font because it consumes way more horizontal space. The only thing I can do here is to scale the font's height by a hard coded factor (like ~95%).

I have added a commit to do just that. If a constant factor can't fix your concern (exact value up for bike shedding), you are welcome to close this PR.

laanwj commented at 11:47 AM on November 3, 2015: member

Maybe, there is a difference when you use the gitian builds by jonasschnelli ( 9pt)

Right - the gitian build by jonasschelli uses static qt from the depends system, I build against the system's Qt5 library (this is what the Ubuntu PPA does). Fine with a fixed factor, seems an Ubuntu problem, don't want to bikeshed this further either.

MarcoFalke commented at 12:44 PM on November 3, 2015: member

As OS X was the only one getting it right, I expect the constant factor makes the font look smaller now on OS X. Hopefully such that it is still acceptable.

jonasschnelli commented at 1:22 PM on November 3, 2015: contributor

I think this is a improvement over the current version and the PR is pretty clean. Minor font size differences are an upstream issue IMO and still result in a adequate quality. Also the indent formatting increase readability.

MarcoFalke commented at 1:30 PM on November 3, 2015: member

@jonasschnelli So you are against 268b79e?

jonasschnelli commented at 1:38 PM on November 3, 2015: contributor

ACK https://github.com/bitcoin/bitcoin/commit/268b79ef0c54a3bff0b50e093b98b943e49a4939

in src/qt/rpcconsole.cpp:None in 268b79ef0c

 461 | @@ -462,13 +462,19 @@ void RPCConsole::clear()
 462 |      }
 463 |  
 464 |      // Set default style sheet
 465 | +    QFontInfo fixedFontInfo(GUIUtil::fixedPitchFont());
 466 | +    // Try to make fixed font adequately large on different OS
 467 | +    QString ptSize = QString("%1pt").arg(QFontInfo(QFont()).pointSize() * 8.5 / 9);

luke-jr commented at 6:33 PM on November 3, 2015:

This seems weird. Why is this not simply using fixedFontInfo.pointSize()?

jonasschnelli commented at 7:12 PM on November 3, 2015:

I also preferred to non-* 8.5 / 9 solution. But i have ACKed this solution because it might slightly reduce the size of mostly-oversized monospace fonts. But a *0.95 instead a * 8.5/9 would be cleaner, right.

MarcoFalke commented at 7:16 PM on November 3, 2015:

@luke-jr fixedFontInfo.pointSize() is 12, so way too large. Also, people were requesting that the fixed pitch font is exactly the same size as the previous default font. @jonasschnelli QFontInfo(QFont()).pointSize() is 9, often. So we get a 8.5pt in this case instead of a longer float.

luke-jr commented at 8:09 PM on November 3, 2015:

@MarcoFalke If fixedFontInfo.pointSize() is wrong, then that should be fixed instead. If people merely don't have their systems configured correctly, that isn't a problem we should try to workaround in the application.

MarcoFalke commented at 12:01 PM on November 4, 2015:

@luke-jr Maybe you could do something like http://stackoverflow.com/a/12179548 to read the system default font. But I am not planning to do this within this PR, as this will likely change the text for the whole GUI.

I see only three options for this PR:

Close
Merge 28313b83fc1a16b5e6da2b868b5f297050ce8b05 (Monospace with "old" text-height)
Merge 268b79ef0c54a3bff0b50e093b98b943e49a4939 (Same as above with constant scale factor)

luke-jr commented at 6:35 PM on November 3, 2015: member

268b79ef0c54a3bff0b50e093b98b943e49a4939 looks fine to me, but it can be improved by using the fixed font for the input as well. This nit should at least get a comment explaining the rationale. Also, next time, please do the rename (bitcoinAddressFont -> fixedPitchFont) in a dedicated commit.

laanwj commented at 12:05 PM on November 4, 2015: member

This is already bikeshedded to death, please let's not start another round. I personally couldn't care less if the console font is monospace or proportional but some people want this and it seems to work acceptably, so I'm going to merge it now.

laanwj merged this on Nov 4, 2015

laanwj closed this on Nov 4, 2015

laanwj referenced this in commit c702521a85 on Nov 4, 2015

MarcoFalke deleted the branch on Nov 4, 2015

DrahtBot locked this on Sep 8, 2021

[qt] Use monospace font #6864

Windows

Fedora

Fedora

Ubuntu

OSX