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
  1. 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.
  2. luke-jr cross-referenced this on Dec 3, 2021 from issue Monospaced output in Console on macOS by RandyMcMillan
  3. jarolrod added the label Feature on Dec 3, 2021
  4. jarolrod added the label UX on Dec 3, 2021
  5. 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.
  6. 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 :)

  7. luke-jr cross-referenced this on Dec 13, 2021 from issue QRImageWidget: Sizing and font fixups by luke-jr
  8. 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.

  9. 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?
  10. 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.
  11. 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.
  12. promag commented at 4:43 pm on December 13, 2021: contributor
    Concept ACK.
  13. 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.

  14. DrahtBot cross-referenced this on Dec 30, 2021 from issue Do not require restart if overridden option is modified by hebasto
  15. DrahtBot cross-referenced this on Dec 30, 2021 from issue Move third-party tx URL setting from Display to Wallet options tab by jarolrod
  16. DrahtBot cross-referenced this on Jan 7, 2022 from issue refactor, qt: Use std::chrono for parameters of QTimer methods by hebasto
  17. DrahtBot added the label Needs rebase on Jan 12, 2022
  18. hebasto commented at 3:23 pm on April 13, 2022: member
    @luke-jr still working on this?
  19. hebasto added the label Waiting for author on Apr 13, 2022
  20. luke-jr commented at 5:42 pm on April 13, 2022: member
    @hebasto Yes, but I don’t understand the review comment.
  21. RandyMcMillan commented at 7:13 am on April 14, 2022: contributor
  22. hebasto removed the label Waiting for author on Apr 15, 2022
  23. luke-jr force-pushed on Apr 28, 2022
  24. DrahtBot removed the label Needs rebase on Apr 28, 2022
  25. DrahtBot cross-referenced this on May 20, 2022 from issue Unify bitcoin-qt and bitcoind persistent settings by ryanofsky
  26. DrahtBot cross-referenced this on May 20, 2022 from issue refactor: Pass interfaces::Node references to OptionsModel constructor by ryanofsky
  27. DrahtBot cross-referenced this on May 20, 2022 from issue refactor: Add OptionsModel getOption/setOption methods by ryanofsky
  28. DrahtBot cross-referenced this on May 21, 2022 from issue Add settings.json prune-prev, proxy-prev, onion-prev settings by ryanofsky
  29. DrahtBot added the label Needs rebase on May 22, 2022
  30. hebasto commented at 4:03 pm on October 26, 2022: member
    @luke-jr Rebase?
  31. luke-jr force-pushed on Dec 6, 2022
  32. luke-jr commented at 10:10 am on December 6, 2022: member
    Rebased
  33. DrahtBot removed the label Needs rebase on Dec 6, 2022
  34. 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?
  35. 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?
  36. DrahtBot cross-referenced this on Dec 9, 2022 from issue refactor: Extract MIB_BYTES constant for init.cpp by Empact
  37. 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
  38. DrahtBot cross-referenced this on Dec 17, 2022 from issue clang-tidy: Force checks for headers in `src/qt` by hebasto
  39. DrahtBot added the label Needs rebase on Jan 17, 2023
  40. hebasto commented at 4:14 pm on April 17, 2023: member
    Closing this due to lack of activity. Feel free to reopen.
  41. hebasto closed this on Apr 17, 2023

  42. GUI: Move "embedded font or not" decision into new OptionsModel::getFontForMoney method f2dfde80b8
  43. GUI: Add possibility for an explicit QFont for FontForMoney in OptionsModel 49eb97eff9
  44. GUI: Load custom FontForMoney from QSettings 3a6757eed9
  45. GUI: Use FontChoice type in OptionsModel settings abstraction 98e9ac5199
  46. GUI: OptionsDialog: Replace verbose two-option font selector with simple combobox with Custom... choice a17fd33edd
  47. achow101 reopened this on Jul 21, 2023

  48. luke-jr force-pushed on Jul 21, 2023
  49. luke-jr commented at 5:51 pm on July 21, 2023: member
    Rebased
  50. DrahtBot removed the label Needs rebase on Jul 21, 2023
  51. DrahtBot added the label CI failed on Aug 18, 2023
  52. DrahtBot removed the label CI failed on Aug 23, 2023
  53. DrahtBot added the label CI failed on Sep 3, 2023
  54. DrahtBot removed the label CI failed on Sep 4, 2023
  55. DrahtBot added the label CI failed on Sep 30, 2023
  56. DrahtBot removed the label CI failed on Oct 1, 2023
  57. pablomartin4btc commented at 4:26 am on October 6, 2023: contributor

    tested ACK a17fd33edd1374145fd6986fbe352295355fde4f

    Current UI without this PR:

    image

    With this PR:

    image

    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?

    image

    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?

  58. DrahtBot added the label CI failed on Oct 25, 2023
  59. 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.

  60. hebasto approved
  61. 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.
  62. DrahtBot requested review from jarolrod on Feb 7, 2024
  63. DrahtBot requested review from promag on Feb 7, 2024
  64. hebasto merged this on Feb 7, 2024
  65. hebasto 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