Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Sep 10, 2020

This change makes macOS choose the correct monospaced font.

fanquake noted (bitcoin/bitcoin#16432 (comment)):

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

hebasto commented Sep 10, 2020

cc @fanquake @luke-jr

@hebasto
Copy link
Member Author

hebasto commented Oct 23, 2020

@jonasschnelli Should this be considered a bugfix?

@jonasschnelli
Copy link
Contributor

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

@hebasto
Copy link
Member Author

hebasto commented Oct 23, 2020

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

hebasto commented Oct 23, 2020

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

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

Looks good to me, one change requested


QFont f = GUIUtil::fixedPitchFont();
f.setWeight(QFont::Bold);
f.setPointSize(10);
Copy link
Member

Choose a reason for hiding this comment

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

Let's not override this for no reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Updated.

Now I'm able to change the font size with the qt5ct utility on Linux Mint 20 :)

This change makes macOS choose the correct monospaced font.
@hebasto
Copy link
Member Author

hebasto commented Oct 25, 2020

Updated a44ba74 -> 2e386cd (pr87.01 -> pr87.02, diff):

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

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK

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.

Tested ACK on macOS 10.15.7

Master
master

PR
pr

@jonasschnelli
Copy link
Contributor

Master:
Bildschirmfoto 2021-01-28 um 10 26 02

This PR:
Bildschirmfoto 2021-01-28 um 10 23 57

I think the correct monospace font should be bigger and eventually bold.

@jonasschnelli
Copy link
Contributor

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

@hebasto
Copy link
Member Author

hebasto commented Jan 28, 2021

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

Right. But @luke-jr's opinion is opposite.

@RandyMcMillan
Copy link
Contributor

It would be great to add a font size selector - similar to the console interface.

Screen Shot 2021-01-28 at 10 07 51 PM

@hebasto
Copy link
Member Author

hebasto commented Feb 1, 2021

Closed in favor of #207 which should be less controversial.

@hebasto hebasto closed this Feb 1, 2021
@hebasto hebasto deleted the 200910-mono branch February 22, 2021 12:34
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants