Enable users to configure their monospace font specifically #497
pull luke-jr wants to merge 5 commits into bitcoin-core:master from luke-jr:qt_fontsel changing 6 files +175 −132-
luke-jr commented at 0:18 am on December 3, 2021: memberThis 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: memberConcept 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 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: contributorConcept ACK.DrahtBot commented at 10:52 am on December 30, 2021: contributorThe 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 hebastoDrahtBot cross-referenced this on Dec 30, 2021 from issue Move third-party tx URL setting from Display to Wallet options tab by jarolrodDrahtBot cross-referenced this on Jan 7, 2022 from issue refactor, qt: Use std::chrono for parameters of QTimer methods by hebastoDrahtBot added the label Needs rebase on Jan 12, 2022hebasto added the label Waiting for author on Apr 13, 2022RandyMcMillan commented at 7:13 am on April 14, 2022: contributorhebasto removed the label Waiting for author on Apr 15, 2022luke-jr force-pushed on Apr 28, 2022DrahtBot removed the label Needs rebase on Apr 28, 2022DrahtBot cross-referenced this on May 20, 2022 from issue Unify bitcoin-qt and bitcoind persistent settings by ryanofskyDrahtBot cross-referenced this on May 20, 2022 from issue refactor: Pass interfaces::Node references to OptionsModel constructor by ryanofskyDrahtBot cross-referenced this on May 20, 2022 from issue refactor: Add OptionsModel getOption/setOption methods by ryanofskyDrahtBot cross-referenced this on May 21, 2022 from issue Add settings.json prune-prev, proxy-prev, onion-prev settings by ryanofskyDrahtBot added the label Needs rebase on May 22, 2022luke-jr force-pushed on Dec 6, 2022luke-jr commented at 10:10 am on December 6, 2022: memberRebasedDrahtBot removed the label Needs rebase on Dec 6, 2022hebasto commented at 4:17 pm on December 7, 2022: memberApproach 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: memberI’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 EmpactDrahtBot 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 hebastoDrahtBot cross-referenced this on Dec 17, 2022 from issue clang-tidy: Force checks for headers in `src/qt` by hebastoDrahtBot added the label Needs rebase on Jan 17, 2023hebasto commented at 4:14 pm on April 17, 2023: memberClosing 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 f2dfde80b8GUI: Add possibility for an explicit QFont for FontForMoney in OptionsModel 49eb97eff9GUI: Load custom FontForMoney from QSettings 3a6757eed9GUI: Use FontChoice type in OptionsModel settings abstraction 98e9ac5199GUI: OptionsDialog: Replace verbose two-option font selector with simple combobox with Custom... choice a17fd33eddachow101 reopened this on Jul 21, 2023
luke-jr force-pushed on Jul 21, 2023luke-jr commented at 5:51 pm on July 21, 2023: memberRebasedDrahtBot removed the label Needs rebase on Jul 21, 2023DrahtBot added the label CI failed on Aug 18, 2023DrahtBot removed the label CI failed on Aug 23, 2023DrahtBot added the label CI failed on Sep 3, 2023DrahtBot removed the label CI failed on Sep 4, 2023DrahtBot added the label CI failed on Sep 30, 2023DrahtBot removed the label CI failed on Oct 1, 2023pablomartin4btc commented at 4:26 am on October 6, 2023: contributortested 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, 2023DrahtBot 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 approvedhebasto commented at 7:19 pm on February 7, 2024: memberACK 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, 2024DrahtBot requested review from promag on Feb 7, 2024hebasto merged this on Feb 7, 2024hebasto closed this on Feb 7, 2024
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 21:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me