Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Monospaced output in Console on macOS #477

Merged
merged 1 commit into from
Nov 25, 2021

Conversation

RandyMcMillan
Copy link
Contributor

@RandyMcMillan RandyMcMillan commented Nov 20, 2021

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

RandyMcMillan commented Nov 20, 2021

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

A few more examples:

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

@RandyMcMillan
Copy link
Contributor Author

Screen Shot 2021-11-20 at 12 45 06 AM

@RandyMcMillan RandyMcMillan changed the title rcpconsole.cpp: force monospaced formatted output qt/rcpconsole.cpp: force monospaced formatted output Nov 20, 2021
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

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:

image

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

@RandyMcMillan
Copy link
Contributor Author

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

shaavan commented Nov 20, 2021

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

RandyMcMillan commented Nov 20, 2021

@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 RandyMcMillan force-pushed the issue-275 branch 2 times, most recently from 15eb511 to 4516e39 Compare November 21, 2021 04:18
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

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 4516e39

@katesalazar
Copy link
Contributor

ACK 4516e39.

@katesalazar
Copy link
Contributor

katesalazar commented Nov 21, 2021

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

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

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

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@hebasto hebasto changed the title qt/rcpconsole.cpp: force monospaced formatted output Force monospaced formatted output in RPC Console Nov 21, 2021
@hebasto hebasto added the UI All about "look and feel" label Nov 21, 2021
@RandyMcMillan RandyMcMillan changed the title Force monospaced formatted output in RPC Console qt: monospaced output in Console on macOS Nov 21, 2021
@katesalazar
Copy link
Contributor

katesalazar commented Nov 22, 2021

ACK 9e6775d.

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.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK 9e6775d.

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@hebasto hebasto added macOS and removed Feature labels Nov 22, 2021
@hebasto
Copy link
Member

hebasto commented Nov 22, 2021

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

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

@hebasto hebasto requested a review from jarolrod November 22, 2021 12:21
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

reACK b9f0aff

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.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

tACK b9f0aff

@hebasto hebasto changed the title qt: monospaced output in Console on macOS Monospaced output in Console on macOS Nov 25, 2021
@hebasto hebasto merged commit 9facad0 into bitcoin-core:master Nov 25, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 25, 2021
b9f0aff qt: monospaced output in Console on macOS (randymcmillan)

Pull request description:

  This PR addresses issue bitcoin-core/gui#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()

ACKs for top commit:
  hebasto:
    ACK b9f0aff, Tested on macOS Big Sur 11.6.1 (20G224) + Homebrew's Qt 5.15.2:
  shaavan:
    reACK b9f0aff
  jarolrod:
    tACK b9f0aff

Tree-SHA512: 53e6635a0189e133681c85d442c6c9c4a10438151e4bf7da5bbd62abca7ab55685caf2c9a75ff200aadea771c1602902e6ab14afdc4f411e1b3013dd49625dbc
@katesalazar
Copy link
Contributor

katesalazar commented Nov 27, 2021

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

  • Won't ACK b9f0aff 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 b9f0aff:

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

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

@luke-jr
Copy link
Member

luke-jr commented Dec 3, 2021

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

current Roboto Mono on Dark Appearance:

Screen Shot 2022-01-04 at 21 41 26

available PT Mono on Dark Appearance:

Screen Shot 2022-01-04 at 21 42 53

@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jan 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
macOS UI All about "look and feel"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants