refactor(qt): simplify GUIUtill[fonts] API - #7336
Conversation
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98ac82eb41
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ui->fontWeightNormalSlider->setMinimum(nMin); | ||
| ui->fontWeightNormalSlider->setMaximum(nMax); | ||
|
|
||
| ui->fontWeightNormalSlider->setMinimum(0); | ||
| ui->fontWeightNormalSlider->setMaximum(nMaximum); | ||
| ui->fontWeightBoldSlider->setMinimum(nMin); | ||
| ui->fontWeightBoldSlider->setMaximum(nMax); |
There was a problem hiding this comment.
updateWeightSlider() now sets each slider to a continuous [min,max] range from supportedWeightArgs(), but supported weights are often sparse (for example, a font may support only a subset like 3 and 6). That lets users pick intermediate values that are unsupported; updateFontWeightNormal/Bold() then calls setWeightFromArg(...) and silently ignores failure, so the UI value can diverge from the applied font and an invalid weight can be persisted. Previously the slider operated on dense indices over only supported weights, so this regression is new.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 77beedd49d8002b4155b00e7c16c1fa87e8b3332 and d4badd1. 📒 Files selected for processing (41)
💤 Files with no reviewable changes (33)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughCentralize Qt font handling by removing the public guiutil_font.h surface, internalizing the font registry in guiutil_font.cpp, and expanding GUIUtil (guiutil.h/cpp) with platform, monospace, weight-arg, scale, active-font, setFont overloads, and setStyledHtml. Migrate AppearanceWidget, OptionsModel, bitcoin startup, and all UI files to the new API and update build/test headers and circular-dependency expectations. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/qt/bitcoin.cppsrc/qt/bitcoin.cpp:10:10: fatal error: 'qt/bitcoin.h' file not found ... [truncated 1046 characters] ... ns/clang/install/lib/clang/18/include" src/qt/guiutil.cppsrc/qt/guiutil.cpp:6:10: fatal error: 'qt/guiutil.h' file not found ... [truncated 2200 characters] ... _CTrans.CTrans_funct.instruction_scope in file "src/clang/cTrans.ml", line 4850, characters 22-79 src/qt/guiutil_font.cppsrc/qt/guiutil_font.cpp:5:10: fatal error: 'qt/guiutil.h' file not found ... [truncated 2200 characters] ... cTrans.ml" (inlined), line 104, characters 20-38
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/qt/appearancewidget.cpp`:
- Around line 286-299: The sliders currently use a continuous min/max derived
from supportedWeightArgs(), which allows selecting unsupported intermediate
values; change the slider logic to only expose valid supported args by using the
supported list indices as the slider range (e.g., setMinimum(0),
setMaximum(supported.size()-1)) and map slider positions to supported[i] when
reading/writing weights; update initial setting code to set the slider value to
the index of GUIUtil::defaultWeightArg(...) and modify
updateFontWeightNormal/updateFontWeightBold handlers to convert the slider index
into the corresponding supported value (or snap to nearest supported arg) before
calling GUIUtil::setWeightFromArg()/the existing setter.
- Around line 330-340: The heading font changes are only queued by
GUIUtil::setFont for lblHeading and lblSubHeading and won't apply immediately;
after calling GUIUtil::setFont(...) for those labels, call
GUIUtil::updateFonts() before showing the dialog (i.e., before dlg.exec()) so
the queued font updates are applied; ensure you keep the existing
QObject::connect and dlg.exec() calls and only insert the GUIUtil::updateFonts()
call after the setFont calls and before dlg.exec().
In `@src/qt/guiutil.cpp`:
- Around line 166-175: The file still references BitcoinGUI::DEFAULT_UIPLATFORM
in loadStyleSheet(), so remove that dependency by replacing the fallback there
with a call to the local helper defaultUIPlatform() (or mirror its
platform-detection logic) instead of BitcoinGUI::DEFAULT_UIPLATFORM; update the
loadStyleSheet() code paths that set the "-uiplatform" default to use
defaultUIPlatform() so this translation unit no longer depends on BitcoinGUI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 86c748b4-fd15-40ab-b799-57e77ef84e7a
📥 Commits
Reviewing files that changed from the base of the PR and between beca567 and 98ac82eb4125e28c909b6ee29a17e675289322ce.
📒 Files selected for processing (42)
src/Makefile.qt.includesrc/qt/addressbookpage.cppsrc/qt/addresstablemodel.cppsrc/qt/appearancewidget.cppsrc/qt/appearancewidget.hsrc/qt/askpassphrasedialog.cppsrc/qt/bitcoin.cppsrc/qt/bitcoingui.cppsrc/qt/bitcoingui.hsrc/qt/coincontroldialog.cppsrc/qt/descriptiondialog.cppsrc/qt/guiutil.cppsrc/qt/guiutil.hsrc/qt/guiutil_font.cppsrc/qt/guiutil_font.hsrc/qt/informationwidget.cppsrc/qt/masternodelist.cppsrc/qt/modaloverlay.cppsrc/qt/networkwidget.cppsrc/qt/openuridialog.cppsrc/qt/optionsdialog.cppsrc/qt/optionsmodel.cppsrc/qt/overviewpage.cppsrc/qt/proposalcreate.cppsrc/qt/proposalinfo.cppsrc/qt/proposallist.cppsrc/qt/proposalmodel.cppsrc/qt/proposalresume.cppsrc/qt/qrdialog.cppsrc/qt/qrimagewidget.cppsrc/qt/receivecoinsdialog.cppsrc/qt/receiverequestdialog.cppsrc/qt/rpcconsole.cppsrc/qt/sendcoinsdialog.cppsrc/qt/sendcoinsentry.cppsrc/qt/signverifymessagedialog.cppsrc/qt/splashscreen.cppsrc/qt/test/apptests.cppsrc/qt/trafficgraphwidget.cppsrc/qt/utilitydialog.cppsrc/qt/walletview.cpptest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (12)
- src/qt/splashscreen.cpp
- src/qt/receiverequestdialog.cpp
- src/qt/addresstablemodel.cpp
- src/qt/utilitydialog.cpp
- src/qt/openuridialog.cpp
- src/Makefile.qt.include
- src/qt/qrimagewidget.cpp
- src/qt/trafficgraphwidget.cpp
- src/qt/bitcoingui.h
- src/qt/addressbookpage.cpp
- src/qt/guiutil_font.h
- test/lint/lint-circular-dependencies.py
| // Adjust the headings | ||
| GUIUtil::setFont({&lblHeading}, GUIUtil::FontWeight::Bold, 16); | ||
| GUIUtil::setFont({&lblSubHeading}, GUIUtil::FontWeight::Normal, 14, true); | ||
| // Make sure the dialog closes and accepts the settings if save has been pressed | ||
| QObject::connect(&buttonBox, &QDialogButtonBox::accepted, [&]() { | ||
| QSettings().setValue("fAppearanceSetupDone", true); | ||
| appearance.accept(); | ||
| dlg.accept(); | ||
| }); | ||
| // And fire it! | ||
| dlg.exec(); |
There was a problem hiding this comment.
Apply the heading styles before showing the dialog.
GUIUtil::setFont() only queues the update. Without an GUIUtil::updateFonts() pass here, these labels keep their default styling unless some unrelated font/theme refresh happens while the dialog is open.
Suggested fix
// Adjust the headings
GUIUtil::setFont({&lblHeading}, GUIUtil::FontWeight::Bold, 16);
GUIUtil::setFont({&lblSubHeading}, GUIUtil::FontWeight::Normal, 14, true);
+ GUIUtil::updateFonts();
// Make sure the dialog closes and accepts the settings if save has been pressed
QObject::connect(&buttonBox, &QDialogButtonBox::accepted, [&]() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/qt/appearancewidget.cpp` around lines 330 - 340, The heading font changes
are only queued by GUIUtil::setFont for lblHeading and lblSubHeading and won't
apply immediately; after calling GUIUtil::setFont(...) for those labels, call
GUIUtil::updateFonts() before showing the dialog (i.e., before dlg.exec()) so
the queued font updates are applied; ensure you keep the existing
QObject::connect and dlg.exec() calls and only insert the GUIUtil::updateFonts()
call after the setFont calls and before dlg.exec().
98ac82e to
95a3867
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified against HEAD 98ac82eb4125e28c909b6ee29a17e675289322ce and the requested diff. Both reported issues are introduced by this PR: the weight sliders now expose raw weight-arg ranges instead of dense supported-weight indices, which can leave the slider and registry out of sync for sparse supported sets, and fixedPitchFont() was changed from getFont({... , FontWeight::Normal}) to constructing a plain QFont, so monospace callers no longer inherit the configured normal weight.
🔴 1 blocking | 🟡 1 suggestion(s)
Reviewed queued commit 98ac82eb. The PR head has since advanced to 95a3867a; a follow-up queue item exists for the newer commit.
Inline comment posting failed; posting findings in the review body instead.
Blocking: Weight sliders can now select unsupported font weights and leave UI/model out of sync
src/qt/appearancewidget.cpp:245
This PR changes the sliders from dense indices over GetSupportedWeights() to raw weight args. updateWeightSlider() now sets the range to supported.front()..supported.back(), while updateFontWeightNormal() / updateFontWeightBold() pass the slider value directly to setWeightFromArg(). supportedWeightArgs() is built from the active font's supported subset, and CalcSupportedWeights() can produce sparse sets, so intermediate slider positions are possible. For those positions setWeightFromArg() returns false and leaves the registry unchanged, but the return value is ignored, so the slider can move to a value the model did not accept.
Suggestion: fixedPitchFont() no longer respects the configured normal font weight
src/qt/guiutil.cpp:241
Before this PR, fixedPitchFont() returned getFont({use_embedded_font ? ROBOTO_MONO_FONT_STR : OS_MONO_FONT_STR, FontWeight::Normal}), which resolved through the font registry and therefore used the configured normal-weight selection. This PR replaces that with a plain QFont that only sets family/style-hint, so monospace callers such as OptionsModel::getFontForChoice(), ProposalModel, and RPCConsole stop tracking the user's configured normal weight.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/qt/proposalresume.cpp (1)
258-263:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRecalculate the description height after changing the HTML.
entry.descriptionis kept at a fixed height fromrecalculateDescHeights(), but this refresh path replaces the document content without recomputing that height. When the status text wraps differently, the text edit can clip until the dialog is reopened.💡 Suggested fix
void ProposalResume::refreshConfirmations() { bool all_confirmed{true}; + bool layout_changed{false}; for (auto& entry : m_entries) { const int confs = queryConfirmations(entry.proposal.collateralHash); if (confs != entry.collateral_confs) { entry.collateral_confs = confs; GUIUtil::setStyledHtml(entry.description, formatProposalHtml(entry.proposal, confs)); entry.broadcast_btn->setEnabled(confs >= m_relay_confs); + layout_changed = true; } if (confs < m_relay_confs) { all_confirmed = false; } } + + if (layout_changed) { + recalculateDescHeights(); + } // Stop polling when all entries are confirmed if (all_confirmed && m_refresh_timer) { m_refresh_timer->stop(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/qt/proposalresume.cpp` around lines 258 - 263, After updating an entry's HTML via GUIUtil::setStyledHtml(entry.description, formatProposalHtml(...)) you must recompute the stored text heights so the QTextEdit doesn't clip; call the existing recalculateDescHeights() (or at minimum recalculate the single entry's height) after changing entry.description and before/after enabling the broadcast button. Locate the update inside the loop over m_entries and invoke recalculateDescHeights() (or the per-entry height recalculation routine) so entry.description's fixed height matches the new content.src/qt/guiutil_font.cpp (1)
398-427:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProbe
SystemMonospacewith the real fixed-pitch family.
getFontWithWeight()mirrors the sentinel handling forOS_FONT_STR, but not forOS_MONO_FONT_STR. Right now the support/default-weight probing for the system monospace path runs against the literal"SystemMonospace"family name instead ofQFontDatabase::systemFont(QFontDatabase::FixedFont), so the registry can learn the wrong weights for that font.💡 Suggested fix
QFont getFontWithWeight(const QString& font_name, QFont::Weight weight, double point_size) { QFont font; if (font_name == MONTSERRAT_FONT_STR) { if (mapMontserrat.count(weight)) { `#ifdef` Q_OS_MACOS font.setFamily(font_name); font.setStyleName(QString::fromStdString(mapMontserrat.at(weight).first)); `#else` if (weight == QFont::Normal || weight == QFont::Bold) { font.setFamily(font_name); } else { font.setFamily(qstrprintf("%s %s", font_name.toStdString(), mapMontserrat.at(weight).first)); } `#endif` } } else if (font_name == OS_FONT_STR && g_default_font) { font.setFamily(g_default_font->family()); + } else if (font_name == OS_MONO_FONT_STR) { + font.setFamily(QFontDatabase::systemFont(QFontDatabase::FixedFont).family()); } else { font.setFamily(font_name); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/qt/guiutil_font.cpp` around lines 398 - 427, getFontWithWeight currently treats OS_MONO_FONT_STR like a literal family name, causing probes to use "SystemMonospace" instead of the real fixed-pitch family; change the function to handle OS_MONO_FONT_STR similarly to OS_FONT_STR by resolving the actual system monospace family (e.g. use QFontDatabase::systemFont(QFontDatabase::FixedFont).family() or the existing g_default_font equivalent for monospace) before setting the family and weight so probing uses the real fixed-pitch family when font_name == OS_MONO_FONT_STR and g_default_mono_font (or systemFont) is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/qt/guiutil_font.cpp`:
- Around line 398-427: getFontWithWeight currently treats OS_MONO_FONT_STR like
a literal family name, causing probes to use "SystemMonospace" instead of the
real fixed-pitch family; change the function to handle OS_MONO_FONT_STR
similarly to OS_FONT_STR by resolving the actual system monospace family (e.g.
use QFontDatabase::systemFont(QFontDatabase::FixedFont).family() or the existing
g_default_font equivalent for monospace) before setting the family and weight so
probing uses the real fixed-pitch family when font_name == OS_MONO_FONT_STR and
g_default_mono_font (or systemFont) is available.
In `@src/qt/proposalresume.cpp`:
- Around line 258-263: After updating an entry's HTML via
GUIUtil::setStyledHtml(entry.description, formatProposalHtml(...)) you must
recompute the stored text heights so the QTextEdit doesn't clip; call the
existing recalculateDescHeights() (or at minimum recalculate the single entry's
height) after changing entry.description and before/after enabling the broadcast
button. Locate the update inside the loop over m_entries and invoke
recalculateDescHeights() (or the per-entry height recalculation routine) so
entry.description's fixed height matches the new content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 38165005-bb6a-4d3d-9671-bbe42085840b
📥 Commits
Reviewing files that changed from the base of the PR and between 98ac82eb4125e28c909b6ee29a17e675289322ce and 95a3867a9bd1f652221accd91d8628460de20f4f.
📒 Files selected for processing (41)
src/Makefile.qt.includesrc/qt/addressbookpage.cppsrc/qt/addresstablemodel.cppsrc/qt/appearancewidget.cppsrc/qt/askpassphrasedialog.cppsrc/qt/bitcoin.cppsrc/qt/bitcoingui.cppsrc/qt/bitcoingui.hsrc/qt/coincontroldialog.cppsrc/qt/descriptiondialog.cppsrc/qt/guiutil.cppsrc/qt/guiutil.hsrc/qt/guiutil_font.cppsrc/qt/guiutil_font.hsrc/qt/informationwidget.cppsrc/qt/masternodelist.cppsrc/qt/modaloverlay.cppsrc/qt/networkwidget.cppsrc/qt/openuridialog.cppsrc/qt/optionsdialog.cppsrc/qt/optionsmodel.cppsrc/qt/overviewpage.cppsrc/qt/proposalcreate.cppsrc/qt/proposalinfo.cppsrc/qt/proposallist.cppsrc/qt/proposalmodel.cppsrc/qt/proposalresume.cppsrc/qt/qrdialog.cppsrc/qt/qrimagewidget.cppsrc/qt/receivecoinsdialog.cppsrc/qt/receiverequestdialog.cppsrc/qt/rpcconsole.cppsrc/qt/sendcoinsdialog.cppsrc/qt/sendcoinsentry.cppsrc/qt/signverifymessagedialog.cppsrc/qt/splashscreen.cppsrc/qt/test/apptests.cppsrc/qt/trafficgraphwidget.cppsrc/qt/utilitydialog.cppsrc/qt/walletview.cpptest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (33)
- src/qt/modaloverlay.cpp
- src/qt/coincontroldialog.cpp
- src/qt/openuridialog.cpp
- src/qt/bitcoingui.h
- src/qt/utilitydialog.cpp
- src/qt/networkwidget.cpp
- src/qt/receiverequestdialog.cpp
- src/qt/trafficgraphwidget.cpp
- src/qt/qrimagewidget.cpp
- src/qt/descriptiondialog.cpp
- src/qt/proposalinfo.cpp
- src/qt/walletview.cpp
- src/qt/guiutil_font.h
- src/Makefile.qt.include
- src/qt/bitcoingui.cpp
- src/qt/splashscreen.cpp
- test/lint/lint-circular-dependencies.py
- src/qt/optionsdialog.cpp
- src/qt/informationwidget.cpp
- src/qt/overviewpage.cpp
- src/qt/sendcoinsentry.cpp
- src/qt/addressbookpage.cpp
- src/qt/addresstablemodel.cpp
- src/qt/masternodelist.cpp
- src/qt/qrdialog.cpp
- src/qt/signverifymessagedialog.cpp
- src/qt/sendcoinsdialog.cpp
- src/qt/askpassphrasedialog.cpp
- src/qt/rpcconsole.cpp
- src/qt/proposallist.cpp
- src/qt/receivecoinsdialog.cpp
- src/qt/appearancewidget.cpp
- src/qt/optionsmodel.cpp
✅ Files skipped from review due to trivial changes (2)
- src/qt/test/apptests.cpp
- src/qt/proposalmodel.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95a3867a9b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ui->fontWeightNormalSlider->setMinimum(0); | ||
| ui->fontWeightNormalSlider->setMaximum(nMaximum); | ||
| ui->fontWeightBoldSlider->setMinimum(nMin); | ||
| ui->fontWeightBoldSlider->setMaximum(nMax); |
There was a problem hiding this comment.
updateWeightSlider() now maps sliders to a continuous [min,max] argument range, but many fonts expose only a sparse subset of supported weight args. In that case users can select in-between values that setWeightFromArg(...) rejects, and updateFontWeightNormal/Bold() ignore that failure and still keep the slider at the rejected value. This leaves the UI and persisted setting out of sync with the actual applied font weight after changing fonts or moving the slider.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (98ac82eb..95a3867a) only updates the stylesheet default platform lookup and removes dead includes; I found no new latest-delta findings. Cumulatively, both prior Qt font findings are STILL VALID at 95a3867a: the unsupported weight-slider issue remains blocking, and fixedPitchFont() still bypasses the configured normal weight.
🔴 1 blocking | 🟡 1 suggestion(s)
Findings
🔴 Blocking: Weight sliders still allow unsupported values and can persist a font weight the active font never accepted
src/qt/appearancewidget.cpp:245-299
Prior review finding revalidated: STILL VALID. updateWeightSlider() sets each slider to the numeric range between supported.front() and supported.back(), but supportedWeightArgs() is built from FontInfo::CalcSupportedWeights(), which can return a sparse subset of the 0..8 weight args. That lets the sliders stop on values that are inside the range but not actually supported by the active font. When that happens, updateFontWeightNormal() and updateFontWeightBold() pass the raw slider value into GUIUtil::setWeightFromArg(), which rejects unsupported weights and leaves the registry unchanged, but the return value is ignored. Because the sliders are mapped directly to OptionsModel::FontWeightNormal and OptionsModel::FontWeightBold and submitted on accept, the UI can save an unsupported weight arg even though the live font state never switched to it.
🟡 Suggestion: fixedPitchFont() no longer follows the configured normal-weight selection
src/qt/guiutil.cpp:241-247
Prior review finding revalidated: STILL VALID. Before this refactor, fixedPitchFont() returned a registry-backed font via getFont(...), so the monospace helper inherited the current normal-weight selection. The current implementation constructs a fresh QFont from either "Roboto Mono" or QFontDatabase::systemFont(QFontDatabase::FixedFont) and only sets the monospace style hint, so the selected normal weight is dropped. That difference is observable at current HEAD in direct consumers such as ProposalModel::data() and RPCConsole::clear(), which use the returned QFont as-is instead of overriding its weight afterward.
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
please handle automated reviews
95a3867 to
ac906bb
Compare
ac906bb to
77beedd
Compare
|
✅ Review complete (commit d4badd1) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Qt GUI font-API refactor that hides FontRegistry/FontInfo/FontAttrib behind free functions, inlines guiutil_font.h into guiutil.h, and replaces BitcoinGUI::DEFAULT_UIPLATFORM with GUIUtil::defaultUIPlatform. Both independent agents converge on no in-scope findings, and prior regressions (weight snapping in appearancewidget, fixedPitchFont routing) are resolved at HEAD.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/guiutil_font.cpp (1)
261-270:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve weights against the requested family, not the active family.
Lines 269-270 ignore
FontAttrib::m_fontand always pull bold/normal from the current active family. That breaks the newsetFont(vecWidgets, const QString& font, ...)contract: any widget pinned to another family inherits whatever weight selection belongs to the active family instead of that family's cached/default weights.Suggested fix
class FontRegistry { public: + QFont::Weight GetWeight(const QString& font, GUIUtil::FontWeight slot) const + { + if (auto it = m_weights.find(font); it != m_weights.end()) { + return slot == GUIUtil::FontWeight::Bold ? it->second.m_bold : it->second.m_normal; + } + return slot == GUIUtil::FontWeight::Bold ? TARGET_WEIGHT_BOLD : TARGET_WEIGHT_NORMAL; + } + QFont::Weight GetWeightBold() const { if (auto it = m_weights.find(m_font); it != m_weights.end()) { return it->second.m_bold; } return TARGET_WEIGHT_BOLD; @@ - const QFont::Weight weight = (font_attrib.m_weight_type == GUIUtil::FontWeight::Bold) - ? g_font_registry.GetWeightBold() : g_font_registry.GetWeightNormal(); + const QFont::Weight weight = g_font_registry.GetWeight(font_attrib.m_font, font_attrib.m_weight_type);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/qt/guiutil_font.cpp` around lines 261 - 270, getFont currently resolves weight using the active family via g_font_registry.GetWeightBold()/GetWeightNormal(), which ignores FontAttrib::m_font; change the resolution to use the requested family in font_attrib (e.g., call g_font_registry.GetWeightBold(font_attrib.m_font) or GetWeightNormal(font_attrib.m_font) or the equivalent API) so weight selection honors the specified family; update the branch that sets const QFont::Weight weight (inside getFont) to derive weight from font_attrib.m_font rather than the global/active family.
♻️ Duplicate comments (1)
src/qt/appearancewidget.cpp (1)
337-339:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply the queued heading fonts before showing the setup dialog.
GUIUtil::setFont()only records the requests forlblHeadingandlblSubHeading. Without aGUIUtil::updateFonts()pass beforedlg.exec(), the dialog opens with default styling until some unrelated font refresh happens.Suggested fix
// Adjust the headings GUIUtil::setFont({&lblHeading}, GUIUtil::FontWeight::Bold, 16); GUIUtil::setFont({&lblSubHeading}, GUIUtil::FontWeight::Normal, 14, true); + GUIUtil::updateFonts(); // Make sure the dialog closes and accepts the settings if save has been pressed QObject::connect(&buttonBox, &QDialogButtonBox::accepted, [&]() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/qt/appearancewidget.cpp` around lines 337 - 339, The queued font changes applied via GUIUtil::setFont for lblHeading and lblSubHeading are not materialized before the dialog is shown; call GUIUtil::updateFonts() after setting the fonts and before invoking dlg.exec() so the queued font updates are applied to the dialog (ensure this happens in the same scope where GUIUtil::setFont({&lblHeading}, ...) and GUIUtil::setFont({&lblSubHeading}, ...) are called, just prior to dlg.exec()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/qt/appearancewidget.cpp`:
- Around line 303-305: The current code in the fForce branch calls
updateFontWeightNormal(...) and updateFontWeightBold(...) with
GUIUtil::defaultWeightArg(...), which overwrites the selected family's cached
weights in FontRegistry::m_weights; change this to initialize the sliders from
the active family's stored values by calling GUIUtil::currentWeightArg(...) (or
equivalent accessor that reads FontRegistry::m_weights for the current family)
and pass those results into updateFontWeightNormal(...) and
updateFontWeightBold(...), so previewing a different family does not wipe its
saved bold/normal selections.
---
Outside diff comments:
In `@src/qt/guiutil_font.cpp`:
- Around line 261-270: getFont currently resolves weight using the active family
via g_font_registry.GetWeightBold()/GetWeightNormal(), which ignores
FontAttrib::m_font; change the resolution to use the requested family in
font_attrib (e.g., call g_font_registry.GetWeightBold(font_attrib.m_font) or
GetWeightNormal(font_attrib.m_font) or the equivalent API) so weight selection
honors the specified family; update the branch that sets const QFont::Weight
weight (inside getFont) to derive weight from font_attrib.m_font rather than the
global/active family.
---
Duplicate comments:
In `@src/qt/appearancewidget.cpp`:
- Around line 337-339: The queued font changes applied via GUIUtil::setFont for
lblHeading and lblSubHeading are not materialized before the dialog is shown;
call GUIUtil::updateFonts() after setting the fonts and before invoking
dlg.exec() so the queued font updates are applied to the dialog (ensure this
happens in the same scope where GUIUtil::setFont({&lblHeading}, ...) and
GUIUtil::setFont({&lblSubHeading}, ...) are called, just prior to dlg.exec()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8892ac5b-5852-493f-972f-a17d61e5c7f3
📥 Commits
Reviewing files that changed from the base of the PR and between 95a3867a9bd1f652221accd91d8628460de20f4f and 77beedd49d8002b4155b00e7c16c1fa87e8b3332.
📒 Files selected for processing (42)
src/Makefile.qt.includesrc/qt/addressbookpage.cppsrc/qt/addresstablemodel.cppsrc/qt/appearancewidget.cppsrc/qt/appearancewidget.hsrc/qt/askpassphrasedialog.cppsrc/qt/bitcoin.cppsrc/qt/bitcoingui.cppsrc/qt/bitcoingui.hsrc/qt/coincontroldialog.cppsrc/qt/descriptiondialog.cppsrc/qt/guiutil.cppsrc/qt/guiutil.hsrc/qt/guiutil_font.cppsrc/qt/guiutil_font.hsrc/qt/informationwidget.cppsrc/qt/masternodelist.cppsrc/qt/modaloverlay.cppsrc/qt/networkwidget.cppsrc/qt/openuridialog.cppsrc/qt/optionsdialog.cppsrc/qt/optionsmodel.cppsrc/qt/overviewpage.cppsrc/qt/proposalcreate.cppsrc/qt/proposalinfo.cppsrc/qt/proposallist.cppsrc/qt/proposalmodel.cppsrc/qt/proposalresume.cppsrc/qt/qrdialog.cppsrc/qt/qrimagewidget.cppsrc/qt/receivecoinsdialog.cppsrc/qt/receiverequestdialog.cppsrc/qt/rpcconsole.cppsrc/qt/sendcoinsdialog.cppsrc/qt/sendcoinsentry.cppsrc/qt/signverifymessagedialog.cppsrc/qt/splashscreen.cppsrc/qt/test/apptests.cppsrc/qt/trafficgraphwidget.cppsrc/qt/utilitydialog.cppsrc/qt/walletview.cpptest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (12)
- src/qt/openuridialog.cpp
- src/qt/bitcoingui.h
- test/lint/lint-circular-dependencies.py
- src/qt/qrimagewidget.cpp
- src/qt/receiverequestdialog.cpp
- src/qt/addresstablemodel.cpp
- src/qt/utilitydialog.cpp
- src/qt/trafficgraphwidget.cpp
- src/qt/guiutil_font.h
- src/qt/splashscreen.cpp
- src/qt/addressbookpage.cpp
- src/Makefile.qt.include
✅ Files skipped from review due to trivial changes (6)
- src/qt/test/apptests.cpp
- src/qt/modaloverlay.cpp
- src/qt/qrdialog.cpp
- src/qt/proposallist.cpp
- src/qt/networkwidget.cpp
- src/qt/proposalmodel.cpp
🚧 Files skipped from review as they are similar to previous changes (17)
- src/qt/proposalcreate.cpp
- src/qt/masternodelist.cpp
- src/qt/descriptiondialog.cpp
- src/qt/optionsdialog.cpp
- src/qt/appearancewidget.h
- src/qt/informationwidget.cpp
- src/qt/rpcconsole.cpp
- src/qt/coincontroldialog.cpp
- src/qt/askpassphrasedialog.cpp
- src/qt/walletview.cpp
- src/qt/overviewpage.cpp
- src/qt/guiutil.h
- src/qt/sendcoinsdialog.cpp
- src/qt/optionsmodel.cpp
- src/qt/signverifymessagedialog.cpp
- src/qt/bitcoin.cpp
- src/qt/bitcoingui.cpp
| if (fForce) { | ||
| updateFontWeightNormal(GUIUtil::defaultWeightArg(GUIUtil::FontWeight::Normal), true); | ||
| updateFontWeightBold(GUIUtil::defaultWeightArg(GUIUtil::FontWeight::Bold), true); |
There was a problem hiding this comment.
Don't wipe a font family's cached weights when switching families.
src/qt/guiutil_font.cpp now keeps bold/normal selections per family in FontRegistry::m_weights, but Lines 304-305 immediately replace the newly selected family's cached values with defaultWeightArg(...). That means just previewing another family silently loses any previously chosen weights for that family before the user touches either slider. Initialize the sliders from currentWeightArg(...) for the active family instead.
Suggested fix
- if (fForce) {
- updateFontWeightNormal(GUIUtil::defaultWeightArg(GUIUtil::FontWeight::Normal), true);
- updateFontWeightBold(GUIUtil::defaultWeightArg(GUIUtil::FontWeight::Bold), true);
- }
+ if (fForce) {
+ updateFontWeightNormal(GUIUtil::currentWeightArg(GUIUtil::FontWeight::Normal), true);
+ updateFontWeightBold(GUIUtil::currentWeightArg(GUIUtil::FontWeight::Bold), true);
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/qt/appearancewidget.cpp` around lines 303 - 305, The current code in the
fForce branch calls updateFontWeightNormal(...) and updateFontWeightBold(...)
with GUIUtil::defaultWeightArg(...), which overwrites the selected family's
cached weights in FontRegistry::m_weights; change this to initialize the sliders
from the active family's stored values by calling GUIUtil::currentWeightArg(...)
(or equivalent accessor that reads FontRegistry::m_weights for the current
family) and pass those results into updateFontWeightNormal(...) and
updateFontWeightBold(...), so previewing a different family does not wipe its
saved bold/normal selections.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
utACK 77beedd49d8002b4155b00e7c16c1fa87e8b3332
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
…IPlatform It helps to break circular dependency over qt/guiutil
e0e1925 to
d4badd1
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review of PR #7336 at d4badd1. The latest delta (77beedd..d4badd1) is a one-line comment fix updating a header reference in guiutil_font.cpp following the prior header-inlining commit. The cumulative refactor encapsulates the Qt font registry behind a free-function GUIUtil API, removes two real circular dependencies (qt/appearancewidget and qt/bitcoingui via qt/guiutil), and relocates DEFAULT_UIPLATFORM. No prior findings were carried forward (prior review had none). Codex and CodeRabbit reported no findings. Claude's three nitpicks all describe either pre-existing behavior preserved by the refactor or purely speculative future-proofing; none are concrete bugs.
4 participants
Issue being fixed or feature implemented
There's overcomplicated public header
guiutil_fonts.hwhich introduces bunch of static methods inside global variables, such as g_fonts_known, g_font_registryguiutil.h has internal classes
FontInfoandFontRegistryin public namespace.class
FontAttribis also non-needed abstraction.What was done?
This PR simplifies public API for GUIUtil [fonts].
Particularly, done:
setWeightFromArg,currentWeightArg,defaultWeightArg,supportedWeightArgsfor UI settingsFontInfo,FontRegistryand global objectsg_fonts_known,g_font_registryare removed from public API GUIUtil [font]weightFromArg,weightToArg,WeightToIdxandIdxToWeighthelpers from public API GUIUtil [font]getFonts,registerFontandsetActiveFontto justsetActiveFont()isValidWeightArgandsetWeightFromArgtobool setWeightFromArg()LoadFonthelper that is super-seeded by multiple rich dash core helpersqt/guiutil_font.his inlined to toguiutil.hUnrelated extra changes:
registerWidgettosetStyledHtmlwhich is more clear for develop.As a result of this PR multiple usages of GUIUtil[font] became much simplier, for example:
How Has This Been Tested?
Run
dash-qtand played with all sliders and combo-boxes on Appearance Tab. Seems as nothing is broken.Two circular dependencies are gone:
Breaking Changes
N/A
Checklist: