Enable users to configure their monospace font specifically

luke-jr commented at 0:18 am on December 3, 2021: member

This replaces the overly-verbose radio-button font setting (which only allows embedded or autodetected from system) with a simple combobox providing both existing options as well as a custom option to allow the user to select any font of their choice/style.

luke-jr cross-referenced this on Dec 3, 2021 from issue Monospaced output in Console on macOS by RandyMcMillan

jarolrod added the label Feature on Dec 3, 2021

jarolrod added the label UX on Dec 3, 2021

jarolrod commented at 4:39 am on December 3, 2021: member

Concept ACK, a follow-up can be to use the selected font wherever monospace fonts are used such as the console.

luke-jr commented at 4:53 am on December 3, 2021: member

a follow-up can be to use the selected font wherever monospace fonts are used such as the console.

A separate configuration for that may make sense , but yes, this makes it much simpler to do that. Figured it would be a bit too much for this PR though :)

luke-jr cross-referenced this on Dec 13, 2021 from issue QRImageWidget: Sizing and font fixups by luke-jr

RandyMcMillan commented at 2:42 pm on December 13, 2021: contributor

Concept ACK

This should allow users to select a dyslexic font as well (if they have it installed) - which can be helpful when dealing with numbers.

in src/qt/optionsdialog.cpp:36 in 7740196117 outdated

28 #include <QMessageBox>
29 #include <QSettings>
30 #include <QSystemTrayIcon>
31 #include <QTimer>
32 
33+int setFontChoice(QComboBox* cb, const OptionsModel::FontChoice& fc)

promag commented at 4:37 pm on December 13, 2021:

move under anonymous namespace?

in src/qt/optionsdialog.cpp:38 in 7740196117 outdated

30 #include <QSystemTrayIcon>
31 #include <QTimer>
32 
33+int setFontChoice(QComboBox* cb, const OptionsModel::FontChoice& fc)
34+{
35+    int i;

promag commented at 4:38 pm on December 13, 2021:

0int i = cb->count();
1while (i--) {

luke-jr commented at 5:09 pm on December 13, 2021:

The current code is better/clearer.

in src/qt/optionsdialog.cpp:74 in 7740196117 outdated

66+        QFont f;
67+        if (item_data.canConvert<OptionsModel::FontChoice>()) {
68+            f = OptionsModel::getFontForChoice(item_data.value<OptionsModel::FontChoice>());
69+        } else {
70+            bool ok;
71+            f = QFontDialog::getFont(&ok, GUIUtil::fixedPitchFont(false), cb->parentWidget());

promag commented at 4:42 pm on December 13, 2021:

This creates a nested event loop which we are trying to avoid. At least there should be a follow-up refactor to avoid this helper.

luke-jr commented at 5:30 pm on December 13, 2021:

What do you mean?

hebasto commented at 7:06 pm on April 13, 2022:

@luke-jr

What do you mean?

I think, @promag means an event loop created by a dialog.

luke-jr commented at 4:02 am on April 28, 2022:

I don’t see a reason to avoid it. The caution in the link is not applicable.

promag commented at 4:43 pm on December 13, 2021: contributor

Concept ACK.

DrahtBot commented at 10:52 am on December 30, 2021: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, hebasto
Concept ACK	jarolrod, promag, RandyMcMillan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot cross-referenced this on Dec 30, 2021 from issue Do not require restart if overridden option is modified by hebasto

DrahtBot cross-referenced this on Dec 30, 2021 from issue Move third-party tx URL setting from Display to Wallet options tab by jarolrod

DrahtBot cross-referenced this on Jan 7, 2022 from issue refactor, qt: Use std::chrono for parameters of QTimer methods by hebasto

DrahtBot added the label Needs rebase on Jan 12, 2022

hebasto commented at 3:23 pm on April 13, 2022: member

@luke-jr still working on this?

hebasto added the label Waiting for author on Apr 13, 2022

luke-jr commented at 5:42 pm on April 13, 2022: member

@hebasto Yes, but I don’t understand the review comment.

RandyMcMillan commented at 7:13 am on April 14, 2022: contributor

Concept ACK

https://github.com/RandyMcMillan/gui/runs/6019248786

Screen Shot 2022-04-14 at 2 58 41 AM Screen Shot 2022-04-14 at 2 59 07 AM

hebasto removed the label Waiting for author on Apr 15, 2022

luke-jr force-pushed on Apr 28, 2022

DrahtBot removed the label Needs rebase on Apr 28, 2022

DrahtBot cross-referenced this on May 20, 2022 from issue Unify bitcoin-qt and bitcoind persistent settings by ryanofsky

DrahtBot cross-referenced this on May 20, 2022 from issue refactor: Pass interfaces::Node references to OptionsModel constructor by ryanofsky

DrahtBot cross-referenced this on May 20, 2022 from issue refactor: Add OptionsModel getOption/setOption methods by ryanofsky

DrahtBot cross-referenced this on May 21, 2022 from issue Add settings.json prune-prev, proxy-prev, onion-prev settings by ryanofsky

DrahtBot added the label Needs rebase on May 22, 2022

hebasto commented at 4:03 pm on October 26, 2022: member

@luke-jr Rebase?

luke-jr force-pushed on Dec 6, 2022

luke-jr commented at 10:10 am on December 6, 2022: member

Rebased

DrahtBot removed the label Needs rebase on Dec 6, 2022

hebasto commented at 4:17 pm on December 7, 2022: member

Approach ACK. Tested on Windows 11. Observing kind of jittering of the vertical position of the “Font in the Overview tab:” label when choosing different fonts. Could it be avoided?

luke-jr commented at 1:07 am on December 8, 2022: member

I’m guessing it’s the preview font changing? How would you suggest avoiding it?

DrahtBot cross-referenced this on Dec 9, 2022 from issue refactor: Extract MIB_BYTES constant for init.cpp by Empact

DrahtBot cross-referenced this on Dec 16, 2022 from issue clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers by hebasto

DrahtBot cross-referenced this on Dec 17, 2022 from issue clang-tidy: Force checks for headers in `src/qt` by hebasto

DrahtBot added the label Needs rebase on Jan 17, 2023

hebasto commented at 4:14 pm on April 17, 2023: member

Closing this due to lack of activity. Feel free to reopen.

hebasto closed this on Apr 17, 2023

GUI: Move "embedded font or not" decision into new OptionsModel::getFontForMoney method f2dfde80b8

GUI: Add possibility for an explicit QFont for FontForMoney in OptionsModel 49eb97eff9

GUI: Load custom FontForMoney from QSettings 3a6757eed9

GUI: Use FontChoice type in OptionsModel settings abstraction 98e9ac5199

GUI: OptionsDialog: Replace verbose two-option font selector with simple combobox with Custom... choice a17fd33edd

achow101 reopened this on Jul 21, 2023

luke-jr force-pushed on Jul 21, 2023

luke-jr commented at 5:51 pm on July 21, 2023: member

Rebased

DrahtBot removed the label Needs rebase on Jul 21, 2023

DrahtBot added the label CI failed on Aug 18, 2023

DrahtBot removed the label CI failed on Aug 23, 2023

DrahtBot added the label CI failed on Sep 3, 2023

DrahtBot removed the label CI failed on Sep 4, 2023

DrahtBot added the label CI failed on Sep 30, 2023

DrahtBot removed the label CI failed on Oct 1, 2023

pablomartin4btc commented at 4:26 am on October 6, 2023: contributor

tested ACK a17fd33edd1374145fd6986fbe352295355fde4f

Current UI without this PR:

With this PR:

The label is moving vertically with some font selection as @hebasto pointed it out and even the window gets resized sometimes.

Perhaps to avoid it, we could just leave the view as we have it today but instead of a radio button we fit the combo box in there so there’s enough space for the text preview.

In the dialog that’s shown once we click on “custom…”, shouldn’t we filter by “monospace” font types only? @RandyMcMillan, I’m not sure if this could be an issue, however I checked that some dyslexic fonts that are non-monospaced (eg OpenDyslexic) have monospaced versions as well.

And perhaps limit the size?

When reviewing the commit messages, except for the 3rd one (3a6757eed9a24e91e7d800d8026cc3a5c4840141), please take into account the title’s length compared to the recommendation in our guidelines (eg GUI: Move "embedded font or not" decision into new OptionsModel::getFontForMoney method) perhaps when the title doesn’t fit you can shorten them with the intention and add both technical and code-specific details in the commit body?

DrahtBot added the label CI failed on Oct 25, 2023

DrahtBot commented at 2:34 pm on February 5, 2024: contributor

🤔 There hasn’t been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

hebasto approved

hebasto commented at 7:19 pm on February 7, 2024: member

ACK a17fd33edd1374145fd6986fbe352295355fde4f, I have reviewed the code and tested it on Ubuntu 22.04. This is a UX improvement. #497 (comment) might be addressed in a follow-up.

DrahtBot requested review from jarolrod on Feb 7, 2024

DrahtBot requested review from promag on Feb 7, 2024

hebasto merged this on Feb 7, 2024

hebasto closed this on Feb 7, 2024

Enable users to configure their monospace font specifically #497

Reviews

Conflicts