Monospaced output in Console on macOS

RandyMcMillan commented at 4:20 am on November 20, 2021: contributor

This PR addresses issue #273 A monospace font is used on Linux and Windows for the console output - but not on MacOS. This change forces the MacOS GUI to use the embedded RobotoMono-Bold.ttf font, which is defined as the GUIUtil::fixedPitchFont()

RandyMcMillan commented at 4:22 am on November 20, 2021: contributor

REF: Issue: #273

Per @hebasto ’s suggestion

Before: Screen Shot 2021-11-19 at 11 15 18 PM

After: Screen Shot 2021-11-19 at 11 16 32 PM

RandyMcMillan commented at 5:30 am on November 20, 2021: contributor

A few more examples:

Screen Shot 2021-11-20 at 12 29 39 AM

RandyMcMillan commented at 5:45 am on November 20, 2021: contributor

RandyMcMillan renamed this:
~~rcpconsole.cpp: force monospaced formatted output~~
qt/rcpconsole.cpp: force monospaced formatted output
on Nov 20, 2021

shaavan commented at 7:10 am on November 20, 2021: contributor

Concept ACK

This is a definite improvement over the currently displayed font. However, I think the newly displayed font is too bold and looks very bulky when there is a lot of text on the screen. Like in this screenshot:

What do you think about experimenting with a lighter version of the monospace font?

For example, in #273, the screenshot of Linux GUI RPC Console displays a lighter version of the monospace font. That text looks very light while each word is also easily readable. Screenshot:

RandyMcMillan commented at 7:54 am on November 20, 2021: contributor

This issue has been posted since April (7 months) - if you want to focus on a bunch of minutia - feel free to on your own time. I am not burning a bunch of time on a 30 second edit that anybody else could have done.

shaavan commented at 8:47 am on November 20, 2021: contributor

Well, I was not expecting such an aggressive response. I was under the impression that the goal of this PR was to fix the problem entirely and not just to fix a tiny issue that anybody could have fixed.

RandyMcMillan closed this on Nov 20, 2021

RandyMcMillan reopened this on Nov 20, 2021

RandyMcMillan commented at 7:21 pm on November 20, 2021: contributor

@shaavan - may apologizes for the tone of my previous response…

The whole embedding fonts approach opens up a whole can of worms - which I prefer to avoid. This PR is a simple “fix” avoiding all that controversy.

There is clearly a more elegant fix which you may want to implement and I would be open to reviewing and contributing to.

With that said - this PR is a minimal fix which shouldn’t evoke the font issues. Feel free to review previous discussions on the topic.

RandyMcMillan force-pushed on Nov 21, 2021

shaavan approved

shaavan commented at 7:56 am on November 21, 2021: contributor

may apologizes for the tone of my previous response…

It’s okay. Sometimes all of us feel a little out of place and may lose our cool. The good thing is to accept our mistakes humbly. So I appreciate that.

With that said - this PR is a minimal fix which shouldn’t evoke the font issues.

I understand the context of this PR now. And IMO this PR accomplishes the goal it set out to achieve.

ACK 4516e39c0a9376385ad72689361992040defa2ff

katesalazar commented at 11:40 am on November 21, 2021: contributor

ACK 4516e39c0a9376385ad72689361992040defa2ff.

katesalazar commented at 11:42 am on November 21, 2021: contributor

I would be happier enclosing the new code under __APPLE__ ifdefs though.

hebasto added the label Feature on Nov 21, 2021

in src/qt/rpcconsole.cpp:869 in 4516e39c0a outdated

865@@ -866,7 +866,7 @@ void RPCConsole::clear(bool keep_prompt)
866     }
867 
868     // Set default style sheet
869-    QFontInfo fixedFontInfo(GUIUtil::fixedPitchFont());
870+    QFontInfo fixedFontInfo(GUIUtil::fixedPitchFont(true));

hebasto commented at 8:21 pm on November 21, 2021:

To make code more readable and easier to reason about:

0    QFontInfo fixedFontInfo(GUIUtil::fixedPitchFont(/*use_embedded_font=*/true));

See https://github.com/bitcoin/bitcoin/pull/23545

RandyMcMillan commented at 9:38 pm on November 21, 2021:

@katesalazar @hebasto - thanks for the feedback!

hebasto commented at 8:24 pm on November 21, 2021: member

Concept ACK.

I would be happier enclosing the new code under __APPLE__ ifdefs though.

Agree that there are no reasons to change look in systems that do not require any change.

Would you mind to provide the pr description? And refer to #273 in it?

Also please use qt: prefix in the commit message instead of rcpconsole.cpp:.

hebasto renamed this:
~~qt/rcpconsole.cpp: force monospaced formatted output~~
Force monospaced formatted output in RPC Console
on Nov 21, 2021

hebasto added the label UI on Nov 21, 2021

RandyMcMillan renamed this:
~~Force monospaced formatted output in RPC Console~~
qt: monospaced output in Console on macOS
on Nov 21, 2021

RandyMcMillan force-pushed on Nov 21, 2021

katesalazar commented at 6:05 am on November 22, 2021: contributor

ACK 9e6775d424ad931d3d94755dddf0563fe27b8ded.

I’d be happier with a commit message bringing a thorough description copying bits from here and the parent issue. Maybe in the future Apple fixes this and the two lines do the same, I wouldn’t want to check the issue tracker to know why this code, only VCS history. Lossless VCS history migration is far easier than lossless issue tracker migration. In other words, GitHub is temporary, VCS history could be forever.

in src/qt/rpcconsole.cpp:869 in 9e6775d424 outdated

865@@ -866,7 +866,11 @@ void RPCConsole::clear(bool keep_prompt)
866     }
867 
868     // Set default style sheet
869+#ifdef __APPLE__

hebasto commented at 6:40 am on November 22, 2021:

0#ifdef Q_OS_MAC

as we use it in our Qt code (sorry for not mentioning this earlier).

hebasto commented at 6:40 am on November 22, 2021: member

Approach ACK 9e6775d424ad931d3d94755dddf0563fe27b8ded.

hebasto added the label macOS on Nov 22, 2021

hebasto removed the label Feature on Nov 22, 2021

qt: monospaced output in Console on macOS b9f0aff6b4

RandyMcMillan force-pushed on Nov 22, 2021

hebasto commented at 12:00 pm on November 22, 2021: member

ACK b9f0aff6b4a64c75d8f937fb9a3546c96d6f5a01, Tested on macOS Big Sur 11.6.1 (20G224) + Homebrew’s Qt 5.15.2:

Screenshot from 2021-11-22 15-34-14

hebasto requested review from jarolrod on Nov 22, 2021

shaavan approved

shaavan commented at 1:35 pm on November 22, 2021: contributor

reACK b9f0aff6b4a64c75d8f937fb9a3546c96d6f5a01

Changes since my last review:

Applied ifdef, endif conditional statement to prevent this change from affecting other Operating Systems. This change prevents the risk of having unintentional behavior changes on some OS.
Added /*use_embedded_font=*/ in the argument of fixedPitchFont function. This is done to make the code more readable.

jarolrod commented at 1:54 am on November 24, 2021: member

tACK b9f0aff6b4a64c75d8f937fb9a3546c96d6f5a01

hebasto renamed this:
~~qt: monospaced output in Console on macOS~~
Monospaced output in Console on macOS
on Nov 25, 2021

hebasto merged this on Nov 25, 2021

hebasto closed this on Nov 25, 2021

sidhujag referenced this in commit 93b38bbffa on Nov 25, 2021

katesalazar commented at 5:12 pm on November 27, 2021: contributor

Well a couple of late points… I’m always swamped y’know.

Won’t ACK b9f0aff6b4a64c7 only because I’m not familiar enough with Q_OS_MAC.
I tested this, and (only after testing) I have to agree to the boldness point written by shaavan, I can’t avoid writing it, but
- in my system the boldness could be made away with this patch immediately on top of b9f0aff6b4a64c7:
```
diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp
index 4262866f32..847e73c7ae 100644
--- a/src/qt/guiutil.cpp
+++ b/src/qt/guiutil.cpp
@@ -88,7 +88,11 @@ QString dateTimeStr(qint64 nTime)
 QFont fixedPitchFont(bool use_embedded_font)
 {
     if (use_embedded_font) {
+#ifdef __APPLE__
+        return {"PT Mono"};
+#else
         return {"Roboto Mono"};
+#endif
     }
     return QFontDatabase::systemFont(QFontDatabase::FixedFont);
 }
```
  I think this is strongly more “UI-idiomatic” than the b9f0aff6b4a64c7 state of things, I’d recommend macOS users to try it.
  
  I’m afraid it’s not the actual end form the patch should have, if the effect were to be desirable and wanted in!
I’m not familiar enough with macOS development as to know what’s the more appropriate monospaced font on macOS (or if such thing depends on macOS versions). But this one would be definitely an improvement…

Cheers!

RandyMcMillan commented at 10:11 pm on November 27, 2021: contributor

I will take another look - to see how the boldness can be addressed. :)

luke-jr commented at 1:09 am on December 3, 2021: member

I will take another look - to see how the boldness can be addressed. :)

After #497, it should be simpler to make the console font user-configurable.

katesalazar commented at 8:47 pm on January 4, 2022: contributor

current Roboto Mono on Dark Appearance:

available PT Mono on Dark Appearance:

bitcoin-core locked this on Jan 4, 2023

Monospaced output in Console on macOS #477