Choose monospaced font in C++ code rather in `*.ui` file #87

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:200910-mono changing 2 files +11 −56

hebasto commented at 4:16 PM on September 10, 2020: member

This change makes macOS choose the correct monospaced font.

fanquake noted (https://github.com/bitcoin/bitcoin/pull/16432#issuecomment-514486077):

the monospace font looks a bit weird on macOS

... because it is not monospaced.

This is an alternative to #79 as that PR has a Concept NACK from luke-jr.

I'm personally still lean to embedding font as it makes the GUI more stable.
hebasto commented at 4:16 PM on September 10, 2020: member

cc @fanquake @luke-jr
hebasto commented at 10:31 AM on October 23, 2020: member

@jonasschnelli Should this be considered a bugfix?
jonasschnelli commented at 10:34 AM on October 23, 2020: contributor

Unsure about this. IMO this is a MVC violation (which we have plenty but unsure to add more).
hebasto commented at 10:38 AM on October 23, 2020: member

IMO this is a MVC violation (which we have plenty but unsure to add more).

How is it so? I think both qt/overviewpage.cpp and qt/forms/overviewpage.ui belong to "View" in MVC, no?
jonasschnelli commented at 10:44 AM on October 23, 2020: contributor

How is it so? I think both qt/overviewpage.cpp and qt/forms/overviewpage.ui belong to "View" in MVC, no?

I'd say *.ui is view, *.cpp is controller (the non-automatic generated ones).
hebasto commented at 10:47 AM on October 23, 2020: member

How is it so? I think both qt/overviewpage.cpp and qt/forms/overviewpage.ui belong to "View" in MVC, no?

I'd say *.ui is view, *.cpp is controller (the non-automatic generated ones).

Right. But Qt programming usually assumes "the view and the controller objects are combined" (https://doc.qt.io/qt-5/model-view-programming.html).
in src/qt/overviewpage.cpp:147 in a44ba74cec outdated
```
 140 | @@ -141,6 +141,18 @@ OverviewPage::OverviewPage(const PlatformStyle *platformStyle, QWidget *parent)
 141 |      showOutOfSyncWarning(true);
 142 |      connect(ui->labelWalletStatus, &QPushButton::clicked, this, &OverviewPage::handleOutOfSyncWarningClicks);
 143 |      connect(ui->labelTransactionsStatus, &QPushButton::clicked, this, &OverviewPage::handleOutOfSyncWarningClicks);
 144 | +
 145 | +    QFont f = GUIUtil::fixedPitchFont();
 146 | +    f.setWeight(QFont::Bold);
 147 | +    f.setPointSize(10);
```
luke-jr commented at 7:58 PM on October 24, 2020:

Let's not override this for no reason?

hebasto commented at 9:01 AM on October 25, 2020:

The only reason is to make things explicitly rather than implicitly. Does setPointSize call has any downsides?

luke-jr commented at 1:53 PM on October 25, 2020:

Yes, the downside is that it is explicit and overrides the user's preference (which may not be 10pt).

hebasto commented at 3:09 PM on October 25, 2020:

Thanks! Updated.

Now I'm able to change the font size with the qt5ct utility on Linux Mint 20 :)
luke-jr changes_requested
luke-jr commented at 7:59 PM on October 24, 2020: member

Looks good to me, one change requested
qt: Choose monospaced font in C++ code rather in `*.ui` file
This change makes macOS choose the correct monospaced font.
2e386cd3dd
hebasto force-pushed on Oct 25, 2020
hebasto commented at 3:07 PM on October 25, 2020: member
Updated a44ba74ceccf6f61c65972e2ae37327de0db4c79 -> 2e386cd3dd3fea5a22dc8ae871b80a80f8cdb3f7 (pr87.01 -> pr87.02, diff):
- addressed @luke-jr's comment:
Yes, the downside is that it is explicit and overrides the user's preference (which may not be 10pt).
luke-jr approved
luke-jr commented at 3:24 PM on October 25, 2020: member

utACK
jarolrod approved
jarolrod commented at 9:56 PM on November 10, 2020: member

Tested ACK on macOS 10.15.7

Master <img width="427" alt="master" src="https://user-images.githubusercontent.com/23396902/98738345-8ede2100-2375-11eb-8bfc-ef4a78e3e15c.png">

PR <img width="427" alt="pr" src="https://user-images.githubusercontent.com/23396902/98738354-9271a800-2375-11eb-95c8-03027c1fde5c.png">
jonasschnelli commented at 9:27 AM on January 28, 2021: contributor

Master: <img width="1297" alt="Bildschirmfoto 2021-01-28 um 10 26 02" src="https://user-images.githubusercontent.com/178464/106117406-44d63180-6153-11eb-99df-c7ce56525e4d.png">

This PR: <img width="1285" alt="Bildschirmfoto 2021-01-28 um 10 23 57" src="https://user-images.githubusercontent.com/178464/106117429-4a337c00-6153-11eb-8b81-090d0251943b.png">

I think the correct monospace font should be bigger and eventually bold.
jonasschnelli commented at 10:03 AM on January 28, 2021: contributor

Adding f.setPointSize(12); makes it look much better (on macOS, unsure about windows/linux):
hebasto commented at 10:12 AM on January 28, 2021: member

Adding f.setPointSize(12); makes it look much better (on macOS, unsure about windows/linux):

Right. But @luke-jr's opinion is opposite.
RandyMcMillan commented at 3:10 AM on January 29, 2021: contributor

It would be great to add a font size selector - similar to the console interface.
hebasto commented at 12:17 PM on February 1, 2021: member

Closed in favor of #207 which should be less controversial.
hebasto closed this on Feb 1, 2021
hebasto deleted the branch on Feb 22, 2021
bitcoin-core locked this on Aug 16, 2022

Contributors

Linked (view graph)

#79 Embed monospaced font #207 scripted-diff: Use Courier New as tabular figures font in Overview page