Enable users to configure their monospace font specifically #497
Conversation
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 :) |
|
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. |
| #include <QIntValidator> | ||
| #include <QLocale> | ||
| #include <QMessageBox> | ||
| #include <QSettings> | ||
| #include <QSystemTrayIcon> | ||
| #include <QTimer> | ||
|
|
||
| int setFontChoice(QComboBox* cb, const OptionsModel::FontChoice& fc) |
There was a problem hiding this comment.
move under anonymous namespace?
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
@luke-jr still working on this? |
|
@hebasto Yes, but I don't understand the review comment. |
|
@luke-jr Rebase? |
|
Rebased |
|
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? |
|
I'm guessing it's the preview font changing? How would you suggest avoiding it? |
|
Closing this due to lack of activity. Feel free to reopen. |
…ontForMoney method
…ple combobox with Custom... choice
|
Rebased |
There was a problem hiding this comment.
tested ACK a17fd33
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 (3a6757e), 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?
|
🤔 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. |
There was a problem hiding this comment.
ACK a17fd33, 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.
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.