backport: merge bitcoin-core/gui#79, refactor font management code (use monospace font for overview page labels) #6831
Conversation
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughFont management was removed from guiutil.{h,cpp} and replaced by a new dedicated module qt/guiutil_font.{h,cpp} providing FontFamily/FontWeight enums, FontSettings (g_font_defaults), font loading, per-family weight mapping, scaling, getFont APIs, setFont/updateFonts for widgets, and runtime defaults. Many Qt sources added includes for guiutil_font.h and were updated to call the new APIs (appearance, options, overview, QR, dialogs, models, bitcoin.cpp, etc.). OptionsDialog UI gained embedded/system monospaced font controls; OptionsModel added UseEmbeddedMonospacedFont state, signal, persistence, and consumers (OverviewPage, OptionsDialog) were wired to support dynamic switching. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches
🧪 Generate unit tests
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/masternodelist.cpp (1)
5-13: Fix clang-format include ordering to unblock CI.CI failed the Clang Diff Format Check on this file due to include ordering changes. Please re-run project formatting or adjust includes to the expected order.
Apply this minimal include reordering (adjust if your repo’s clang-format enforces a different order), then run the formatter:
#include <qt/masternodelist.h> #include <qt/forms/ui_masternodelist.h> -#include <evo/deterministicmns.h> -#include <qt/clientmodel.h> -#include <coins.h> -#include <qt/guiutil.h> -#include <qt/guiutil_font.h> -#include <qt/walletmodel.h> +#include <coins.h> +#include <evo/deterministicmns.h> +#include <qt/clientmodel.h> +#include <qt/guiutil.h> +#include <qt/guiutil_font.h> +#include <qt/walletmodel.h>If available in your environment, run: git clang-format -- src/qt/masternodelist.cpp
🧹 Nitpick comments (13)
src/qt/guiutil_font.h (2)
82-85: Typo in public API comment.“appliciation” → “application”.
-/** Load dash specific appliciation fonts */ +/** Load Dash-specific application fonts */
23-28: UI label strings likely user-visible; consider i18n-friendly names.“SystemDefault”/“DefaultMonospace” are internal-ish. If shown to users, prefer humanized, translatable labels surfaced from .cpp (e.g., via QCoreApplication::translate) rather than QStrings in a header.
src/qt/appearancewidget.h (2)
8-12: Make header self-sufficient: include QFont.QFont::Weight is used as a data member type; ensure is directly included here to avoid relying on transitive includes.
Apply this diff:
#include <QWidget> +#include <QFont> #include <qt/guiutil.h> #include <qt/guiutil_font.h>Also applies to: 54-55
27-29: Modernize nullptr usage (optional).Prefer nullptr over 0 for pointer defaults.
- explicit AppearanceWidget(QWidget* parent = 0); + explicit AppearanceWidget(QWidget* parent = nullptr);src/qt/walletview.cpp (1)
72-76: Consider avoiding repeated global font updates.If many widgets call GUIUtil::updateFonts(), it can be redundant/expensive. Prefer batching or calling once at app/init scope after all setFont calls.
src/qt/proposalwizard.cpp (1)
16-16: Header inclusion aligns with new GUI font API usage.No functional impact here. As a minor note, if multiple widgets also call GUIUtil::updateFonts() in constructors project-wide, consider centralizing to reduce duplicate global refreshes.
src/qt/overviewpage.cpp (1)
304-306: Guard against a null OptionsModel (defensive).Unlikely in practice, but add a null-check to avoid potential crashes if clientModel exists but optionsModel is not set yet.
- connect(model->getOptionsModel(), &OptionsModel::useEmbeddedMonospacedFontChanged, this, &OverviewPage::setMonospacedFont); - setMonospacedFont(model->getOptionsModel()->getUseEmbeddedMonospacedFont()); + if (model->getOptionsModel()) { + connect(model->getOptionsModel(), &OptionsModel::useEmbeddedMonospacedFontChanged, this, &OverviewPage::setMonospacedFont); + setMonospacedFont(model->getOptionsModel()->getUseEmbeddedMonospacedFont()); + }src/qt/forms/optionsdialog.ui (1)
1058-1169: UI copy/style nits and accessibility.
- Capitalize option labels for consistency: “Embedded” and “Closest matching”.
- Add accessibleName/description for radio buttons to help screen-readers.
Apply this minimal text tweak:
- <string>embedded "%1"</string> + <string>Embedded "%1"</string> ... - <string>closest matching "%1"</string> + <string>Closest matching "%1"</string>Optionally add accessibility hints (example):
<widget class="QRadioButton" name="embeddedFont_radioButton"> + <property name="accessibleName"> + <string>Use embedded monospaced font</string> + </property> + <property name="accessibleDescription"> + <string>Use the bundled Roboto Mono for numbers on the Overview tab</string> + </property> ... <widget class="QRadioButton" name="systemFont_radioButton"> + <property name="accessibleName"> + <string>Use system monospaced font</string> + </property> + <property name="accessibleDescription"> + <string>Use the closest matching monospaced font available on this system</string> + </property>src/qt/guiutil_font.cpp (5)
1-3: Copyright year range inconsistency.The copyright header shows "2014-2025" but the current month is August 2025. Since this appears to be a new file (all lines marked as added), the copyright should start from 2025.
-// Copyright (c) 2014-2025 The Dash Core developers +// Copyright (c) 2025 The Dash Core developers
63-64: Use translated exception message for user-facing errors.The exception message contains an untranslated string that could be shown to users. Consider using the translation system for better internationalization support.
- throw std::invalid_argument(strprintf("Invalid font-family: %s", strFamily.toStdString())); + throw std::invalid_argument(strprintf(QObject::tr("Invalid font-family: %1").toStdString(), strFamily.toStdString()));
270-276: Usestd::absfrom<cstdlib>for consistency.Line 270 and 272 use
abswithout thestd::prefix. Since the file already includes<cmath>, consider usingstd::absfor consistency with C++ standard library usage patterns.- int nBestDiff = abs(*it - targetWeight); + int nBestDiff = std::abs(*it - targetWeight); while (++it != vecSupported.end()) { - int nDiff = abs(*it - targetWeight); + int nDiff = std::abs(*it - targetWeight);
286-287: Typo in variable name on line 287.There's a missing space after the comma in the
std::findcall. This is a minor formatting issue.- auto it = std::find(vecSupported.begin(), vecSupported.end(),normalWeight); + auto it = std::find(vecSupported.begin(), vecSupported.end(), normalWeight);
362-365: CheckfontsLoaded()instead of checkingosDefaultFontdirectly.The function checks
osDefaultFontdirectly but should use thefontsLoaded()function for consistency with the rest of the codebase.// Fonts need to be loaded by GUIUtil::loadFonts(), if not just return. - if (!osDefaultFont) { + if (!fontsLoaded()) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (38)
src/Makefile.qt.include(2 hunks)src/qt/addressbookpage.cpp(1 hunks)src/qt/addresstablemodel.cpp(1 hunks)src/qt/appearancewidget.cpp(1 hunks)src/qt/appearancewidget.h(1 hunks)src/qt/askpassphrasedialog.cpp(1 hunks)src/qt/bitcoin.cpp(3 hunks)src/qt/bitcoingui.cpp(1 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/forms/optionsdialog.ui(1 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/guiutil.cpp(0 hunks)src/qt/guiutil.h(0 hunks)src/qt/guiutil_font.cpp(1 hunks)src/qt/guiutil_font.h(1 hunks)src/qt/masternodelist.cpp(1 hunks)src/qt/modaloverlay.cpp(1 hunks)src/qt/openuridialog.cpp(1 hunks)src/qt/optionsdialog.cpp(2 hunks)src/qt/optionsmodel.cpp(8 hunks)src/qt/optionsmodel.h(4 hunks)src/qt/overviewpage.cpp(3 hunks)src/qt/overviewpage.h(1 hunks)src/qt/proposalwizard.cpp(1 hunks)src/qt/qrdialog.cpp(1 hunks)src/qt/qrimagewidget.cpp(1 hunks)src/qt/receivecoinsdialog.cpp(1 hunks)src/qt/receiverequestdialog.cpp(1 hunks)src/qt/rpcconsole.cpp(1 hunks)src/qt/sendcoinsdialog.cpp(1 hunks)src/qt/sendcoinsentry.cpp(1 hunks)src/qt/signverifymessagedialog.cpp(1 hunks)src/qt/splashscreen.cpp(1 hunks)src/qt/test/apptests.cpp(1 hunks)src/qt/trafficgraphwidget.cpp(1 hunks)src/qt/transactiondescdialog.cpp(1 hunks)src/qt/utilitydialog.cpp(1 hunks)src/qt/walletview.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- src/qt/guiutil.cpp
- src/qt/guiutil.h
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Applied to files:
src/qt/test/apptests.cpp
🪛 GitHub Actions: Clang Diff Format Check
src/qt/masternodelist.cpp
[error] 5-13: Clang format differences detected by clang-format-diff.py for src/qt/masternodelist.cpp. The patch shows added '#include <coins.h>' before '#include <evo/deterministicmns.h>' and removal of the original '#include <coins.h>'. Step exited with code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: linux64_ubsan-test / Test source
- GitHub Check: linux64_sqlite-test / Test source
- GitHub Check: linux64_nowallet-test / Test source
- GitHub Check: linux64-test / Test source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
🔇 Additional comments (44)
src/qt/optionsmodel.h (4)
110-116: Getter looks fine.No issues; keep it const-correct and inexpensive.
139-146: Initialize and migrate safely.Confirm m_use_embedded_monospaced_font is initialized in Init() and cleared/set in Reset(); if old settings exist, handle migration to avoid undefined reads.
161-162: Emit signal on change and hook consumers.Ensure setData() emits useEmbeddedMonospacedFontChanged(value) on actual change and that OverviewPage connects/disconnects safely during model swaps.
69-93: No changes needed: UseEmbeddedMonospacedFont wiring is complete and correct. Default (true) is initialized in Init/Reset, data()/setData() handle the enum, the QSettings key “UseEmbeddedMonospacedFont” is stable, QDataWidgetMapper bindsui->embeddedFont_radioButtonviaOptionsModel::UseEmbeddedMonospacedFont, and OverviewPage listens/emits the change—enum insertion did not break any indices.src/qt/guiutil_font.h (1)
93-95: Forward-declare QWidget to keep header lean.You already accept QWidget*; the forward declaration added above avoids pulling in here and keeps compile units smaller.
src/qt/openuridialog.cpp (1)
10-10: Include addition is correct.Required for GUIUtil::updateFonts(); no behavioral changes.
src/qt/coincontroldialog.cpp (2)
16-16: Include addition is correct.Needed for GUIUtil::FontWeight and setFont(); consistent with project-wide refactor.
56-63: Good use of centralized font helper.Batching bold labels via GUIUtil::setFont improves consistency and theme-change handling.
src/qt/addressbookpage.cpp (1)
17-17: Include addition is correct.Required for GUIUtil::updateFonts(); no other changes.
src/qt/bitcoingui.cpp (1)
13-13: Header inclusion is correct for new font API.Including qt/guiutil_font.h here is necessary for GUIUtil::setFont/updateFonts usage elsewhere in this file. No issues.
src/qt/addresstablemodel.cpp (1)
9-9: Good: adopt centralized font helpers.qt/guiutil_font.h inclusion aligns this model with the new font subsystem; FontRole usage compiles against the new API.
src/qt/masternodelist.cpp (1)
12-12: Good: add guiutil_font.h for setFont/updateFonts.Required for the bold/normal weights set during UI init. Looks fine.
src/qt/modaloverlay.cpp (1)
10-10: Good: header added for font weights.Matches the Bold font setup used in this widget. No further changes needed.
src/Makefile.qt.include (2)
140-140: Header added to BITCOIN_QT_H is correct.qt/guiutil_font.h should be exported to all Qt TU’s; this ensures consistent inclusion.
231-231: Source added to BITCOIN_QT_BASE_CPP is correct.qt/guiutil_font.cpp will now be compiled into libbitcoinqt; linkage should resolve across wallet/non-wallet builds.
If the overview page uses both normal and bold monospaced faces, verify required Roboto Mono weights are present in QT_RES_FONTS; add any missing weights to avoid fallback differences across platforms.
src/qt/transactiondescdialog.cpp (1)
9-9: Include of guiutil_font.h looks correct.Required after the font API split; keeps this TU compiling without relying on transitive includes.
src/qt/governancelist.cpp (1)
20-20: Header addition is appropriate.This file calls GUIUtil::setFont/updateFonts; after the split, pulling in guiutil_font.h is the right move.
src/qt/receiverequestdialog.cpp (1)
10-10: LGTM: add guiutil_font.h to match refactor.Ensures GUIUtil::updateFonts() and related font helpers are available here.
src/qt/utilitydialog.cpp (1)
15-15: LGTM: explicit font-API include.Consistent with the new font management split; avoids transitive-dependency pitfalls.
src/qt/appearancewidget.h (1)
11-11: Include added is correct for new font APIs.This resolves dependencies on GUIUtil::FontFamily and font helpers.
src/qt/sendcoinsdialog.cpp (1)
18-19: Correct header inclusion for font utilities.Needed for GUIUtil::FontWeight and updateFonts(); no behavioral change introduced here.
src/qt/trafficgraphwidget.cpp (1)
9-10: Include aligns with use of GUIUtil::getFont.Ensures explicit dependency; looks good.
src/qt/askpassphrasedialog.cpp (1)
14-16: Font header inclusion is appropriate.Required for GUIUtil::setFont/updateFonts calls; safe change.
src/qt/qrdialog.cpp (1)
11-12: Good: adds explicit dependency on font utilities.Matches usage of GUIUtil::setFont in this TU.
src/qt/sendcoinsentry.cpp (1)
16-16: Header include aligns with new font API usage.Consistent with the centralized font utilities; no functional change. Keep this PR move-only per your prior backport preference.
src/qt/test/apptests.cpp (1)
11-11: Good: tests now have access to GUIUtil::loadFonts.Load occurs before window creation, which is fine. Please ensure loadFonts is idempotent for repeated appTests runs.
If helpful, add a quick assertion to confirm at least one expected font family is available during tests (e.g., Roboto Mono or system monospace).
src/qt/signverifymessagedialog.cpp (1)
11-11: Appropriate dependency added.Required for setFont/updateFonts calls in this dialog; no behavioral impact.
src/qt/qrimagewidget.cpp (1)
8-8: Correct include for font helpers used in setQR().getFontNormal/calculateIdealFontSize come from guiutil_font; include is needed for portability and build clarity.
src/qt/splashscreen.cpp (1)
19-19: Include looks correct and necessary after font refactor.Header adds access to GUIUtil font APIs used in this file. No issues.
src/qt/walletview.cpp (1)
14-14: Correct header addition for centralized font utilities.Matches usage of GUIUtil::setFont/FontWeight in this TU. Good.
src/qt/overviewpage.h (1)
74-74: Resolved:OverviewPage::setMonospacedFontis implemented, connected toOptionsModel::useEmbeddedMonospacedFontChanged, and invoked on initialization.src/qt/optionsdialog.cpp (1)
379-379: LGTM: correct mapper binding.Mapping embeddedFont_radioButton to UseEmbeddedMonospacedFont (bool/checked) is the right choice; the paired radio will auto-uncheck.
src/qt/overviewpage.cpp (2)
12-12: LGTM: includes updated for font APIs.
378-394: LGTM: focused application of monospaced font to value labels with update propagation.src/qt/bitcoin.cpp (3)
26-26: LGTM: pulls in centralized font defaults.
492-495: LGTM: help-text defaults wired to centralized g_font_defaults.
680-680: LGTM: fallback default now consistent with centralized defaults.src/qt/optionsmodel.cpp (5)
15-16: LGTM: include for centralized font helpers.
100-101: LGTM: defaults sourced from g_font_defaults (family/scale/weights).Also applies to: 110-112, 120-122, 137-139
559-560: LGTM: data() exposes the new option as a bool for the mapper.
807-811: LGTM: setData persists, updates cache, and emits change signal.
332-337: Use boolean for UseEmbeddedMonospacedFont and unify defaultReplace the string literal in src/qt/optionsmodel.cpp (lines 332–334) with a bool and pick the intended default:
- if (!settings.contains("UseEmbeddedMonospacedFont")) { - settings.setValue("UseEmbeddedMonospacedFont", "true"); - } + if (!settings.contains("UseEmbeddedMonospacedFont")) { + settings.setValue("UseEmbeddedMonospacedFont", false); // or true if embedded is the desired default + }Confirm whether the default should be
false(system) ortrue(embedded) so the model and QDataWidgetMapper in OptionsDialog remain in sync.src/qt/guiutil_font.cpp (2)
128-133: Potential null pointer dereference in getFontWeightNormal().The function accesses
mapWeights[fontFamily]without verifying that the fonts have been loaded first. IfloadFonts()hasn't been called or failed, this could lead to unexpected behavior. Consider checkingfontsLoaded()before accessing the map.QFont::Weight getFontWeightNormal() { + if (!fontsLoaded()) { + return g_font_defaults.weight_normal; + } if (!mapWeights.count(fontFamily)) { return g_font_defaults.weight_normal; } return mapWeights[fontFamily].first; }Likely an incorrect or invalid review comment.
240-241: Ignore undefined function warning – TextWidth is declared in guiutil.h and defined in guiutil.cpp, and guiutil_font.cpp is in the GUIUtil namespace so the unqualified call is valid.Likely an incorrect or invalid review comment.
| QFont::Weight getFontWeightBold() | ||
| { | ||
| if (!mapWeights.count(fontFamily)) { | ||
| return g_font_defaults.weight_bold; | ||
| } | ||
| return mapWeights[fontFamily].second; | ||
| } |
There was a problem hiding this comment.
Potential null pointer dereference in getFontWeightBold().
Similar to getFontWeightNormal(), this function should verify fonts are loaded before accessing the map.
QFont::Weight getFontWeightBold()
{
+ if (!fontsLoaded()) {
+ return g_font_defaults.weight_bold;
+ }
if (!mapWeights.count(fontFamily)) {
return g_font_defaults.weight_bold;
}
return mapWeights[fontFamily].second;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| QFont::Weight getFontWeightBold() | |
| { | |
| if (!mapWeights.count(fontFamily)) { | |
| return g_font_defaults.weight_bold; | |
| } | |
| return mapWeights[fontFamily].second; | |
| } | |
| QFont::Weight getFontWeightBold() | |
| { | |
| if (!fontsLoaded()) { | |
| return g_font_defaults.weight_bold; | |
| } | |
| if (!mapWeights.count(fontFamily)) { | |
| return g_font_defaults.weight_bold; | |
| } | |
| return mapWeights[fontFamily].second; | |
| } |
🤖 Prompt for AI Agents
In src/qt/guiutil_font.cpp around lines 144 to 150, getFontWeightBold()
currently assumes the font map is ready and directly indexes
mapWeights[fontFamily], which can cause a null/invalid access; update the
function to mirror getFontWeightNormal() by first checking that fonts are loaded
or that mapWeights contains fontFamily (e.g., if (!fontsLoaded) return
g_font_defaults.weight_bold; or if (!mapWeights.count(fontFamily)) return
g_font_defaults.weight_bold;) and only then access
mapWeights[fontFamily].second, ensuring a safe default is returned when the map
isn’t initialized or the family key is missing.
src/qt/guiutil_font.cpp
Outdated
| assert(mapMontserrat.count(qWeight)); | ||
| #ifdef Q_OS_MAC | ||
| font.setFamily(fontFamilyToString(FontFamily::Montserrat)); | ||
| font.setStyleName([&](){ | ||
| QString ret{std::get<0>(mapMontserrat.at(qWeight))}; | ||
| if (fItalic) { | ||
| if (ret == "Regular") { | ||
| ret = "Italic"; | ||
| } else { | ||
| ret += " Italic"; | ||
| } | ||
| } | ||
| return ret; | ||
| }()); | ||
| #else | ||
| if (qWeight == QFont::Normal || qWeight == QFont::Bold) { | ||
| font.setFamily(fontFamilyToString(FontFamily::Montserrat)); | ||
| } else { | ||
| font.setFamily(fontFamilyToString(FontFamily::Montserrat) + " " + std::get<0>(mapMontserrat.at(qWeight))); | ||
| } | ||
| #endif // Q_OS_MAC |
There was a problem hiding this comment.
Ensure mapMontserrat contains the provided weight.
On line 474, there's an assert that checks if qWeight exists in mapMontserrat. However, this assert will only fire in debug builds. Consider adding proper error handling for release builds to prevent potential crashes.
if (family == FontFamily::Montserrat) {
- assert(mapMontserrat.count(qWeight));
+ if (!mapMontserrat.count(qWeight)) {
+ qDebug() << __func__ << ": Unsupported Montserrat weight:" << qWeight << ", falling back to Normal";
+ qWeight = QFont::Normal;
+ }
#ifdef Q_OS_MAC📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert(mapMontserrat.count(qWeight)); | |
| #ifdef Q_OS_MAC | |
| font.setFamily(fontFamilyToString(FontFamily::Montserrat)); | |
| font.setStyleName([&](){ | |
| QString ret{std::get<0>(mapMontserrat.at(qWeight))}; | |
| if (fItalic) { | |
| if (ret == "Regular") { | |
| ret = "Italic"; | |
| } else { | |
| ret += " Italic"; | |
| } | |
| } | |
| return ret; | |
| }()); | |
| #else | |
| if (qWeight == QFont::Normal || qWeight == QFont::Bold) { | |
| font.setFamily(fontFamilyToString(FontFamily::Montserrat)); | |
| } else { | |
| font.setFamily(fontFamilyToString(FontFamily::Montserrat) + " " + std::get<0>(mapMontserrat.at(qWeight))); | |
| } | |
| #endif // Q_OS_MAC | |
| if (family == FontFamily::Montserrat) { | |
| // Replace assert-only guard with a runtime check and fallback | |
| if (!mapMontserrat.count(qWeight)) { | |
| qDebug() << __func__ | |
| << ": Unsupported Montserrat weight:" << qWeight | |
| << ", falling back to Normal"; | |
| qWeight = QFont::Normal; | |
| } | |
| #ifdef Q_OS_MAC | |
| font.setFamily(fontFamilyToString(FontFamily::Montserrat)); | |
| font.setStyleName([&](){ | |
| QString ret{std::get<0>(mapMontserrat.at(qWeight))}; | |
| if (fItalic) { | |
| if (ret == "Regular") { | |
| ret = "Italic"; | |
| } else { | |
| ret += " Italic"; | |
| } | |
| } | |
| return ret; | |
| }()); | |
| #else | |
| if (qWeight == QFont::Normal || qWeight == QFont::Bold) { | |
| font.setFamily(fontFamilyToString(FontFamily::Montserrat)); | |
| } else { | |
| font.setFamily(fontFamilyToString(FontFamily::Montserrat) | |
| " " + std::get<0>(mapMontserrat.at(qWeight))); | |
| } | |
| #endif // Q_OS_MAC | |
| } |
🤖 Prompt for AI Agents
In src/qt/guiutil_font.cpp around lines 474 to 494, the code relies on
assert(mapMontserrat.count(qWeight)) which only triggers in debug builds;
replace that assumption with a runtime guard that checks if mapMontserrat
contains qWeight and handles the missing case safely (e.g., choose a safe
default like QFont::Normal or the "Regular" entry, log or qWarning() that an
unknown weight was requested), then proceed to use the retrieved mapping in both
the Q_OS_MAC and non-Mac branches; ensure no direct .at(...) is called without
verifying presence so release builds won't crash.
| assert(mapSupportedWeights.count(fontFamily)); | ||
| return mapSupportedWeights[fontFamily]; | ||
| } |
There was a problem hiding this comment.
Replace assert with proper error handling in getSupportedWeights().
The assert on line 556 will only trigger in debug builds. For production code, proper error handling should be implemented.
std::vector<QFont::Weight> getSupportedWeights()
{
- assert(mapSupportedWeights.count(fontFamily));
+ if (!mapSupportedWeights.count(fontFamily)) {
+ throw std::runtime_error(strprintf("%s: Font family not loaded: %s", __func__, fontFamilyToString(fontFamily).toStdString()));
+ }
return mapSupportedWeights[fontFamily];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert(mapSupportedWeights.count(fontFamily)); | |
| return mapSupportedWeights[fontFamily]; | |
| } | |
| std::vector<QFont::Weight> getSupportedWeights() | |
| { | |
| if (!mapSupportedWeights.count(fontFamily)) { | |
| throw std::runtime_error(strprintf( | |
| "%s: Font family not loaded: %s", | |
| __func__, | |
| fontFamilyToString(fontFamily).toStdString() | |
| )); | |
| } | |
| return mapSupportedWeights[fontFamily]; | |
| } |
🤖 Prompt for AI Agents
In src/qt/guiutil_font.cpp around lines 556–558, replace the debug-only assert
by explicitly checking whether mapSupportedWeights contains fontFamily; if it
does, return the stored value, otherwise emit a warning (e.g., qWarning() with
the fontFamily), and return a safe default (such as an empty weight list) or
throw a descriptive exception; ensure no use of operator[] on missing keys so
production builds won't crash.
src/qt/guiutil_font.h
Outdated
| static std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS{{ | ||
| {FontFamily::SystemDefault, QString{"SystemDefault"}, true}, | ||
| {FontFamily::Montserrat, QString{"Montserrat"}, true}, | ||
| {FontFamily::RobotoMono, QString{"Roboto Mono"}, false}, | ||
| {FontFamily::DefaultMonospace, QString{"DefaultMonospace"}, false}, | ||
| }}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid TU-local mutable state; prefer a single definition.
AVAILABLE_FONTS as a non-const static in a header creates one mutable copy per TU. Make it an inline const (C++17) or move to the .cpp with an extern decl to prevent divergence and reduce footprint.
Example (header-only approach):
-static std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS{{
+inline const std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS{{
{FontFamily::SystemDefault, QString{"SystemDefault"}, true},
{FontFamily::Montserrat, QString{"Montserrat"}, true},
{FontFamily::RobotoMono, QString{"Roboto Mono"}, false},
{FontFamily::DefaultMonospace, QString{"DefaultMonospace"}, false},
}};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS{{ | |
| {FontFamily::SystemDefault, QString{"SystemDefault"}, true}, | |
| {FontFamily::Montserrat, QString{"Montserrat"}, true}, | |
| {FontFamily::RobotoMono, QString{"Roboto Mono"}, false}, | |
| {FontFamily::DefaultMonospace, QString{"DefaultMonospace"}, false}, | |
| }}; | |
| inline const std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS{{ | |
| {FontFamily::SystemDefault, QString{"SystemDefault"}, true}, | |
| {FontFamily::Montserrat, QString{"Montserrat"}, true}, | |
| {FontFamily::RobotoMono, QString{"Roboto Mono"}, false}, | |
| {FontFamily::DefaultMonospace, QString{"DefaultMonospace"}, false}, | |
| }}; |
🤖 Prompt for AI Agents
In src/qt/guiutil_font.h around lines 23-28, AVAILABLE_FONTS is a non-const
static in a header which produces a mutable copy in every TU; change it to a
single shared definition by either (A) making it header-only: declare it as
inline const (or inline constexpr if all types permit) so there is one shared
read-only instance across TUs, or (B) move the definition into the .cpp and
leave an extern const declaration in the header; update callers to use the const
symbol and remove any non-const usage.
src/qt/guiutil_font.h
Outdated
| QFont::Weight weight_bold{ | ||
| #ifdef Q_OS_MAC | ||
| QFont::ExtraLight | ||
| #else | ||
| QFont::Light | ||
| #endif // Q_OS_MAC | ||
| }; | ||
| QFont::Weight weight_normal{QFont::Medium}; | ||
| }; |
There was a problem hiding this comment.
Bold/normal defaults inverted.
weight_bold is set lighter than weight_normal (ExtraLight/Light vs Medium). This will render “bold” text lighter than normal. Fix the defaults.
Apply:
- QFont::Weight weight_bold{
-#ifdef Q_OS_MAC
- QFont::ExtraLight
-#else
- QFont::Light
-#endif // Q_OS_MAC
- };
- QFont::Weight weight_normal{QFont::Medium};
+ QFont::Weight weight_bold{
+#ifdef Q_OS_MAC
+ QFont::DemiBold
+#else
+ QFont::Bold
+#endif // Q_OS_MAC
+ };
+ QFont::Weight weight_normal{QFont::Normal};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| QFont::Weight weight_bold{ | |
| #ifdef Q_OS_MAC | |
| QFont::ExtraLight | |
| #else | |
| QFont::Light | |
| #endif // Q_OS_MAC | |
| }; | |
| QFont::Weight weight_normal{QFont::Medium}; | |
| }; | |
| QFont::Weight weight_bold{ | |
| #ifdef Q_OS_MAC | |
| QFont::DemiBold | |
| #else | |
| QFont::Bold | |
| #endif // Q_OS_MAC | |
| }; | |
| QFont::Weight weight_normal{QFont::Normal}; | |
| }; |
🤖 Prompt for AI Agents
In src/qt/guiutil_font.h around lines 38 to 46, the bold/normal defaults are
inverted (weight_bold is lighter than weight_normal); change weight_bold to a
heavier value than weight_normal by setting weight_bold to QFont::Bold (use
QFont::Bold for both branches of the existing #ifdef) and keep weight_normal as
QFont::Medium so “bold” renders heavier than normal.
| QFont embedded_font{GUIUtil::getFont(GUIUtil::FontFamily::RobotoMono, toQFontWeight(GUIUtil::FontWeight::Bold), | ||
| /*fItalic=*/false, /*nPointSize=*/10)}; | ||
| ui->embeddedFont_radioButton->setText(ui->embeddedFont_radioButton->text().arg(QFontInfo(embedded_font).family())); | ||
| ui->embeddedFont_label_1->setFont(embedded_font); | ||
| ui->embeddedFont_label_9->setFont(embedded_font); | ||
|
|
||
| QFont system_font{GUIUtil::getFont(GUIUtil::FontFamily::DefaultMonospace, toQFontWeight(GUIUtil::FontWeight::Bold), | ||
| /*fItalic=*/false, /*nPointSize=*/10)}; | ||
| ui->systemFont_radioButton->setText(ui->systemFont_radioButton->text().arg(QFontInfo(system_font).family())); | ||
| ui->systemFont_label_1->setFont(system_font); | ||
| ui->systemFont_label_9->setFont(system_font); | ||
| // Checking the embeddedFont_radioButton automatically unchecks the systemFont_radioButton. |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prefer model-driven initial state; avoid hardcoding preview fonts’ default selection.
You set up both previews correctly. However, the initial selection is forced later (Line 221). Let the mapper seed state from OptionsModel instead to avoid flicker and mismatches.
Apply this diff:
- // Checking the embeddedFont_radioButton automatically unchecks the systemFont_radioButton.
- ui->systemFont_radioButton->setChecked(true);
+ // Initial checked state will be provided by QDataWidgetMapper from OptionsModel.Also, since this block uses font-family/weight enums and helpers (e.g., toQFontWeight, FontFamily::RobotoMono), ensure the non-transitive include is present:
Add outside this hunk near other includes:
#include <qt/guiutil_font.h>Run to confirm include presence:
🏁 Script executed:
#!/bin/bash
rg -n 'guiutil_font.h' src/qt/optionsdialog.cpp || echo "Missing #include <qt/guiutil_font.h> in optionsdialog.cpp"Length of output: 172
Remove hardcoded font-selection and seed initial state from OptionsModel
- Replace the
setChecked(true)call at line 221 with a comment so QDataWidgetMapper initializes the radio buttons:- // Checking the embeddedFont_radioButton automatically unchecks the systemFont_radioButton. - ui->systemFont_radioButton->setChecked(true); + // Initial checked state provided by QDataWidgetMapper from OptionsModel.
- Add the missing include for the font helpers in
src/qt/optionsdialog.cppalongside other headers:#include <qt/guiutil_font.h>
🤖 Prompt for AI Agents
In src/qt/optionsdialog.cpp around lines 209 to 221, remove the hardcoded
radio-button initialization: delete the setChecked(true) call (line ~221) and
replace it with a brief comment indicating QDataWidgetMapper will populate the
initial state from OptionsModel; ensure you do not programmatically toggle the
other radio button so the mapper can initialize both consistently. Also add the
missing include for the font helper header by adding #include
<qt/guiutil_font.h> alongside the other includes at the top of this file so
GUIUtil::getFont is declared.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/qt/guiutil_font.h (1)
26-31: Avoid per-TU static; make AVAILABLE_FONTS a single definition and use QStringLiteralHeader-level static creates one instance per translation unit and constructs QStrings repeatedly. Make it a single shared constant and reduce allocations.
Option A (header-only, C++17):
-static const std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS{{ - {FontFamily::SystemDefault, QString{"SystemDefault"}, true}, - {FontFamily::Montserrat, QString{"Montserrat"}, true}, - {FontFamily::RobotoMono, QString{"Roboto Mono"}, false}, - {FontFamily::DefaultMonospace, QString{"DefaultMonospace"}, false}, -}}; +inline const std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS{{ + {FontFamily::SystemDefault, QStringLiteral("SystemDefault"), true}, + {FontFamily::Montserrat, QStringLiteral("Montserrat"), true}, + {FontFamily::RobotoMono, QStringLiteral("Roboto Mono"), false}, + {FontFamily::DefaultMonospace, QStringLiteral("DefaultMonospace"), false}, +}};Option B (move to .cpp):
- Header:
- static const std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS{{ ... }}; + extern const std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS;
- In guiutil_font.cpp, define AVAILABLE_FONTS with QStringLiteral entries.
src/qt/guiutil_font.cpp (4)
144-150: Potential null pointer dereference in getFontWeightBold().This function should verify fonts are loaded before accessing the map to prevent crashes.
QFont::Weight getFontWeightBold() { + if (!fontsLoaded()) { + return g_font_defaults.weight_bold; + } if (!mapWeights.count(fontFamily)) { return g_font_defaults.weight_bold; } return mapWeights[fontFamily].second; }
474-494: EnsuremapMontserratcontains the provided weight.The assert will only fire in debug builds. Add proper error handling for release builds to prevent crashes.
if (family == FontFamily::Montserrat) { - assert(mapMontserrat.count(qWeight)); + if (!mapMontserrat.count(qWeight)) { + qDebug() << __func__ << ": Unsupported Montserrat weight:" << qWeight << ", falling back to Normal"; + qWeight = QFont::Normal; + } #ifdef Q_OS_MAC
556-558: Replace assert with proper error handling in getSupportedWeights().The assert will only trigger in debug builds. Production code needs proper error handling.
std::vector<QFont::Weight> getSupportedWeights() { - assert(mapSupportedWeights.count(fontFamily)); + if (!mapSupportedWeights.count(fontFamily)) { + qWarning() << __func__ << ": Font family not loaded:" << fontFamilyToString(fontFamily); + return {}; // Return empty vector as safe default + } return mapSupportedWeights[fontFamily]; }
560-565: Add bounds checking in supportedWeightFromIndex().The assert only works in debug builds. Add proper bounds checking to prevent out-of-bounds access.
QFont::Weight supportedWeightFromIndex(int nIndex) { auto vecWeights = getSupportedWeights(); - assert(vecWeights.size() > uint64_t(nIndex)); + if (nIndex < 0 || static_cast<size_t>(nIndex) >= vecWeights.size()) { + qWarning() << __func__ << ": Index" << nIndex << "out of range for supported weights"; + return vecWeights.empty() ? QFont::Normal : vecWeights.front(); + } return vecWeights[nIndex]; }
🧹 Nitpick comments (7)
src/qt/forms/optionsdialog.ui (1)
1058-1169: Wire up defaults and a11y for the new monospace font section
- Ensure one radio button is selected on dialog open (based on OptionsModel::UseEmbeddedMonospacedFont) to avoid an indeterminate state.
- Consider setting accessibleName/accessibleDescription on the group box and each radio for screen readers.
- Add a translator note (extracomment) clarifying that “%1” will be replaced with a font family.
If you prefer a UI-side default until OptionsDialog initializes, set the embedded option checked:
<widget class="QRadioButton" name="embeddedFont_radioButton"> <property name="text"> <string>embedded "%1"</string> </property> + <property name="checked"> + <bool>true</bool> + </property> </widget>src/qt/guiutil_font.h (2)
8-16: Reduce header heaviness: forward-declare QWidget in the headerIncluding in a widely-included header increases rebuild cost. Forward-declare instead; callers already include QWidget where needed.
-#include <QFont> -#include <QString> -#include <QWidget> +#include <QFont> +#include <QString> +class QWidget;Also applies to: 96-101
85-92: Typo in API doc comment“appliciation” → “application”.
-/** Load dash specific appliciation fonts */ +/** Load Dash-specific application fonts */src/qt/test/apptests.cpp (1)
80-85: Assert font loading in tests for early failure signalIf loadFonts() can fail, assert it so GUI rendering tests don’t proceed in a partial state.
- GUIUtil::loadFonts(); + QVERIFY(GUIUtil::loadFonts());src/qt/guiutil_font.cpp (3)
269-279: Consider more readable variable names in getBestMatch lambda.The variable names
nBestDiffandnDiffsuggest integer types but they hold differences between QFont::Weight enum values.auto getBestMatch = [&](FontFamily fontFamily, QFont::Weight targetWeight) { auto& vecSupported = mapSupportedWeights[fontFamily]; auto it = vecSupported.begin(); QFont::Weight bestWeight = *it; - int nBestDiff = abs(*it - targetWeight); + int bestWeightDiff = abs(*it - targetWeight); while (++it != vecSupported.end()) { - int nDiff = abs(*it - targetWeight); - if (nDiff < nBestDiff) { + int weightDiff = abs(*it - targetWeight); + if (weightDiff < bestWeightDiff) { bestWeight = *it; - nBestDiff = nDiff; + bestWeightDiff = weightDiff; } } return bestWeight; };
127-133: Consider adding fontsLoaded() check to getFontWeightNormal().While the current implementation is safe due to the map check, for consistency with the suggested fix for
getFontWeightBold(), consider adding the same check here.QFont::Weight getFontWeightNormal() { + if (!fontsLoaded()) { + return g_font_defaults.weight_normal; + } if (!mapWeights.count(fontFamily)) { return g_font_defaults.weight_normal; } return mapWeights[fontFamily].first; }
173-175: Consider validating scale factor range.The
getScaledFontSizefunction doesn't validate the input parameters. Consider adding bounds checking to prevent extreme scaling values.double getScaledFontSize(int nSize) { + // Clamp scale to reasonable bounds (-10 to 10 steps) + int clampedScale = std::max(-10, std::min(10, fontScale)); - return std::round(nSize * (1 + (fontScale * g_font_defaults.scale_steps)) * 4) / 4.0; + return std::round(nSize * (1 + (clampedScale * g_font_defaults.scale_steps)) * 4) / 4.0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (38)
src/Makefile.qt.include(2 hunks)src/qt/addressbookpage.cpp(1 hunks)src/qt/addresstablemodel.cpp(1 hunks)src/qt/appearancewidget.cpp(1 hunks)src/qt/appearancewidget.h(1 hunks)src/qt/askpassphrasedialog.cpp(1 hunks)src/qt/bitcoin.cpp(3 hunks)src/qt/bitcoingui.cpp(1 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/forms/optionsdialog.ui(1 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/guiutil.cpp(0 hunks)src/qt/guiutil.h(0 hunks)src/qt/guiutil_font.cpp(1 hunks)src/qt/guiutil_font.h(1 hunks)src/qt/masternodelist.cpp(1 hunks)src/qt/modaloverlay.cpp(1 hunks)src/qt/openuridialog.cpp(1 hunks)src/qt/optionsdialog.cpp(3 hunks)src/qt/optionsmodel.cpp(8 hunks)src/qt/optionsmodel.h(4 hunks)src/qt/overviewpage.cpp(3 hunks)src/qt/overviewpage.h(1 hunks)src/qt/proposalwizard.cpp(1 hunks)src/qt/qrdialog.cpp(1 hunks)src/qt/qrimagewidget.cpp(1 hunks)src/qt/receivecoinsdialog.cpp(1 hunks)src/qt/receiverequestdialog.cpp(1 hunks)src/qt/rpcconsole.cpp(1 hunks)src/qt/sendcoinsdialog.cpp(1 hunks)src/qt/sendcoinsentry.cpp(1 hunks)src/qt/signverifymessagedialog.cpp(1 hunks)src/qt/splashscreen.cpp(1 hunks)src/qt/test/apptests.cpp(1 hunks)src/qt/trafficgraphwidget.cpp(1 hunks)src/qt/transactiondescdialog.cpp(1 hunks)src/qt/utilitydialog.cpp(1 hunks)src/qt/walletview.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- src/qt/guiutil.cpp
- src/qt/guiutil.h
✅ Files skipped from review due to trivial changes (2)
- src/qt/governancelist.cpp
- src/qt/openuridialog.cpp
🚧 Files skipped from review as they are similar to previous changes (28)
- src/qt/addresstablemodel.cpp
- src/qt/addressbookpage.cpp
- src/qt/bitcoingui.cpp
- src/qt/modaloverlay.cpp
- src/Makefile.qt.include
- src/qt/receiverequestdialog.cpp
- src/qt/rpcconsole.cpp
- src/qt/sendcoinsentry.cpp
- src/qt/walletview.cpp
- src/qt/overviewpage.h
- src/qt/utilitydialog.cpp
- src/qt/proposalwizard.cpp
- src/qt/appearancewidget.cpp
- src/qt/askpassphrasedialog.cpp
- src/qt/optionsdialog.cpp
- src/qt/splashscreen.cpp
- src/qt/qrdialog.cpp
- src/qt/qrimagewidget.cpp
- src/qt/masternodelist.cpp
- src/qt/transactiondescdialog.cpp
- src/qt/overviewpage.cpp
- src/qt/appearancewidget.h
- src/qt/optionsmodel.h
- src/qt/receivecoinsdialog.cpp
- src/qt/optionsmodel.cpp
- src/qt/signverifymessagedialog.cpp
- src/qt/sendcoinsdialog.cpp
- src/qt/bitcoin.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Applied to files:
src/qt/test/apptests.cpp
🔇 Additional comments (5)
src/qt/guiutil_font.h (1)
43-48: Qt macro guard: confirm correct platform symbolYou use Q_OS_MACOS but the trailing comment says Q_OS_MAC. If the project targets Qt5, Q_OS_MAC is the canonical symbol. Please confirm the intended minimum Qt version and unify the macro accordingly across this header and its .cpp.
Would you like a quick scan script to flag any Q_OS_MACOS/Q_OS_MAC inconsistencies project-wide?
src/qt/coincontroldialog.cpp (1)
16-16: LGTM: new font utils include aligns with GUIUtil::setFont usageThe include is appropriate for the FontWeight usage and subsequent GUIUtil::updateFonts() call.
src/qt/trafficgraphwidget.cpp (1)
9-9: LGTM: adopting GUIUtil font helpers in the graph widgetHeader inclusion matches use of GUIUtil::getFont for bold/normal text in paintEvent.
src/qt/test/apptests.cpp (1)
11-11: LGTM: bring in font loader for GUI testsIncluding guiutil_font.h is consistent with calling GUIUtil::loadFonts() in appTests().
src/qt/guiutil_font.cpp (1)
237-260: LGTM! Clever algorithm for detecting supported font weights.The implementation correctly identifies supported font weights by comparing text width differences. This is a smart approach that adapts to the actual rendering capabilities of each font family.
| assert(font.pointSize() > 0); | ||
| font.setFamily(qApp->font().family()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive check for font point size.
The assert that font.pointSize() > 0 should be replaced with proper error handling to prevent crashes in release builds.
QFont font = w->font();
- assert(font.pointSize() > 0);
+ if (font.pointSize() <= 0) {
+ qWarning() << __func__ << ": Invalid font point size for widget" << w->objectName();
+ continue; // Skip this widget
+ }
font.setFamily(qApp->font().family());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert(font.pointSize() > 0); | |
| font.setFamily(qApp->font().family()); | |
| QFont font = w->font(); | |
| if (font.pointSize() <= 0) { | |
| qWarning() << __func__ << ": Invalid font point size for widget" << w->objectName(); | |
| continue; // Skip this widget | |
| } | |
| font.setFamily(qApp->font().family()); |
🤖 Prompt for AI Agents
In src/qt/guiutil_font.cpp around lines 413-414, replace the
assert(font.pointSize() > 0) with a defensive runtime check: if font.pointSize()
is <= 0, obtain a safe fallback point size (e.g. qApp->font().pointSize() and if
that is also <= 0 use a hardcoded default like 10), optionally emit a warning,
then call font.setPointSize(fallback) before continuing to
font.setFamily(qApp->font().family()); this prevents crashes in release builds
while preserving sensible font sizing.
src/qt/guiutil_font.h
Outdated
| QFont::Weight weight_bold{QFont::Medium}; | ||
| QFont::Weight weight_normal{ | ||
| #ifdef Q_OS_MACOS | ||
| QFont::ExtraLight | ||
| #else | ||
| QFont::Light | ||
| #endif // Q_OS_MAC | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inverted font-weight defaults and macOS macro mismatch
Bold should be heavier than normal. Also, Qt5 typically defines Q_OS_MAC (not Q_OS_MACOS). Suggest:
- Normal: QFont::Normal
- Bold: QFont::Bold (or DemiBold on mac)
- Use Q_OS_MAC for Qt5; keep comments in sync.
- QFont::Weight weight_bold{QFont::Medium};
- QFont::Weight weight_normal{
-#ifdef Q_OS_MACOS
- QFont::ExtraLight
-#else
- QFont::Light
-#endif // Q_OS_MAC
- };
+ QFont::Weight weight_normal{QFont::Normal};
+ QFont::Weight weight_bold{
+#ifdef Q_OS_MAC
+ QFont::DemiBold
+#else
+ QFont::Bold
+#endif // Q_OS_MAC
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| QFont::Weight weight_bold{QFont::Medium}; | |
| QFont::Weight weight_normal{ | |
| #ifdef Q_OS_MACOS | |
| QFont::ExtraLight | |
| #else | |
| QFont::Light | |
| #endif // Q_OS_MAC | |
| }; | |
| }; | |
| QFont::Weight weight_normal{QFont::Normal}; | |
| QFont::Weight weight_bold{ | |
| #ifdef Q_OS_MAC | |
| QFont::DemiBold | |
| #else | |
| QFont::Bold | |
| #endif // Q_OS_MAC | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In src/qt/guiutil_font.h around lines 41 to 49, the font-weight defaults are
inverted and the platform macro is incorrect for Qt5; change the normal weight
to QFont::Normal and the bold weight to QFont::Bold (on mac use QFont::DemiBold
if desired for mac-specific styling), replace the Q_OS_MACOS check with
Q_OS_MAC, and update the comment to match the macro name so the normal/bold
semantics are correct and platform branch aligns with Qt5.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/optionsmodel.cpp (1)
542-551: Potential crash if fonts aren’t loaded yet (fallback path throws)If GUIUtil::loadFonts() hasn’t run, supportedWeightToIndex() returns -1 and getSupportedFontWeight*Default() throws (release builds skip asserts). Guard for fontsLoaded() or catch.
case FontWeightNormal: { QFont::Weight weight; GUIUtil::weightFromArg(settings.value("fontWeightNormal").toInt(), weight); int nIndex = GUIUtil::supportedWeightToIndex(weight); - if (nIndex == -1) { - nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightNormalDefault()); - } + if (nIndex == -1) { + if (GUIUtil::fontsLoaded()) { + nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightNormalDefault()); + } else { + // Safe fallback until fonts are initialized; UI will refresh after loadFonts()/updateFonts() + nIndex = 0; + } + } assert(nIndex != -1); return nIndex; } case FontWeightBold: { QFont::Weight weight; GUIUtil::weightFromArg(settings.value("fontWeightBold").toInt(), weight); int nIndex = GUIUtil::supportedWeightToIndex(weight); - if (nIndex == -1) { - nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightBoldDefault()); - } + if (nIndex == -1) { + if (GUIUtil::fontsLoaded()) { + nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightBoldDefault()); + } else { + nIndex = 0; + } + } assert(nIndex != -1); return nIndex; }Also applies to: 553-561
♻️ Duplicate comments (5)
src/qt/guiutil_font.h (1)
26-31: Avoid per-TU copies of AVAILABLE_FONTSMake it a single shared definition with inline const.
-static const std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS{{ +inline const std::array<std::tuple<FontFamily, QString, /*selectable=*/bool>, 4> AVAILABLE_FONTS{{ {FontFamily::SystemDefault, QString{"SystemDefault"}, true}, {FontFamily::Montserrat, QString{"Montserrat"}, true}, {FontFamily::RobotoMono, QString{"Roboto Mono"}, false}, {FontFamily::DefaultMonospace, QString{"DefaultMonospace"}, false}, }};src/qt/guiutil_font.cpp (4)
554-558: Replace debug-only assert with runtime guard in getSupportedWeights()Avoid default-inserting an empty vector in release builds.
std::vector<QFont::Weight> getSupportedWeights() { - assert(mapSupportedWeights.count(fontFamily)); - return mapSupportedWeights[fontFamily]; + auto it = mapSupportedWeights.find(fontFamily); + if (it == mapSupportedWeights.end()) { + throw std::runtime_error(strprintf("%s: Font family not loaded: %s", __func__, fontFamilyToString(fontFamily).toStdString())); + } + return it->second; }
560-565: Bounds-check supportedWeightFromIndex() in release buildsPrevent OOB access when vec is empty or index invalid.
QFont::Weight supportedWeightFromIndex(int nIndex) { auto vecWeights = getSupportedWeights(); - assert(vecWeights.size() > uint64_t(nIndex)); - return vecWeights[nIndex]; + if (nIndex < 0 || static_cast<size_t>(nIndex) >= vecWeights.size()) { + throw std::out_of_range(strprintf("%s: Index %d out of range for supported weights", __func__, nIndex)); + } + return vecWeights[static_cast<size_t>(nIndex)]; }
473-494: Don’t rely on assert for Montserrat weight presenceGracefully fall back in release builds.
- if (family == FontFamily::Montserrat) { - assert(mapMontserrat.count(qWeight)); + if (family == FontFamily::Montserrat) { + if (!mapMontserrat.count(qWeight)) { + qWarning() << __func__ << ": Unsupported Montserrat weight:" << qWeight << ", falling back to Normal"; + qWeight = QFont::Normal; + } #ifdef Q_OS_MAC
412-417: Avoid assert on font.pointSize() in updateFonts()Fallback instead of aborting in release.
- QFont font = w->font(); - assert(font.pointSize() > 0); + QFont font = w->font(); + if (font.pointSize() <= 0) { + qWarning() << __func__ << ": Invalid font point size for widget" << w->objectName(); + continue; + }
🧹 Nitpick comments (6)
src/qt/forms/optionsdialog.ui (2)
1101-1113: Exclude preview sample strings from translation.Prevent “111.11111111 DASH” and “909.09090909 DASH” from polluting TS catalogs; they’re samples, not UX strings. Also make the intent explicit with PlainText.
Apply this diff:
- <property name="text"> - <string>111.11111111 DASH</string> - </property> + <property name="text"> + <string notr="true">111.11111111 DASH</string> + </property> + <property name="textFormat"> + <enum>Qt::PlainText</enum> + </property> ... - <property name="text"> - <string>909.09090909 DASH</string> - </property> + <property name="text"> + <string notr="true">909.09090909 DASH</string> + </property> + <property name="textFormat"> + <enum>Qt::PlainText</enum> + </property> ... - <property name="text"> - <string>111.11111111 DASH</string> - </property> + <property name="text"> + <string notr="true">111.11111111 DASH</string> + </property> + <property name="textFormat"> + <enum>Qt::PlainText</enum> + </property> ... - <property name="text"> - <string>909.09090909 DASH</string> - </property> + <property name="text"> + <string notr="true">909.09090909 DASH</string> + </property> + <property name="textFormat"> + <enum>Qt::PlainText</enum> + </property>Also applies to: 1150-1162
1059-1063: Minor UX polish: capitalization, tooltips, and a11y.
- Capitalize radio labels to match surrounding UI.
- Add concise tooltips to clarify “embedded” vs “system”.
- Add accessibleName/Description on the group for screen readers.
Apply this diff:
<widget class="QGroupBox" name="font_groupBox"> <property name="title"> <string>Monospaced font in the Overview tab:</string> </property> + <property name="accessibleName"> + <string>Monospaced font selection</string> + </property> + <property name="accessibleDescription"> + <string>Choose which monospaced font to use on the Overview tab.</string> + </property> ... <widget class="QRadioButton" name="embeddedFont_radioButton"> <property name="text"> - <string>embedded "%1"</string> + <string>Embedded "%1"</string> </property> + <property name="toolTip"> + <string>Use the embedded monospaced font shipped with the application.</string> + </property> </widget> ... <widget class="QRadioButton" name="systemFont_radioButton"> <property name="text"> - <string>closest matching "%1"</string> + <string>Closest matching "%1"</string> </property> + <property name="toolTip"> + <string>Use the closest matching system monospace font.</string> + </property> </widget>Also applies to: 1079-1084, 1128-1133
src/qt/optionsmodel.cpp (1)
332-336: Store boolean, not string, for UseEmbeddedMonospacedFontUse a bool to avoid type drift in QSettings.
- if (!settings.contains("UseEmbeddedMonospacedFont")) { - settings.setValue("UseEmbeddedMonospacedFont", "true"); - } + if (!settings.contains("UseEmbeddedMonospacedFont")) { + settings.setValue("UseEmbeddedMonospacedFont", true); + }src/qt/guiutil_font.h (2)
41-49: Unify macOS macro; consider saner defaultsUse Q_OS_MAC for consistency with .cpp, and prefer Normal/Bold as defaults.
- QFont::Weight weight_bold{QFont::Medium}; - QFont::Weight weight_normal{ -#ifdef Q_OS_MACOS - QFont::ExtraLight -#else - QFont::Light -#endif // Q_OS_MAC - }; + QFont::Weight weight_normal{QFont::Normal}; + QFont::Weight weight_bold{ +#ifdef Q_OS_MAC + QFont::DemiBold +#else + QFont::Bold +#endif // Q_OS_MAC + };
85-88: Typo in comment“appliciation” -> “application”.
-/** Load dash specific appliciation fonts */ +/** Load Dash-specific application fonts */src/qt/guiutil_font.cpp (1)
398-406: Minor: avoid per-call allocations for ignore listsMake the ignore class/object lists static const.
- std::vector<QString> vecIgnoreClasses{ + static const std::vector<QString> vecIgnoreClasses{ "QWidget", "QDialog", "QFrame", "QStackedWidget", "QDesktopWidget", "QDesktopScreenWidget", "QTipLabel", "QMessageBox", "QMenu", "QComboBoxPrivateScroller", "QComboBoxPrivateContainer", "QScrollBar", "QListView", "BitcoinGUI", "WalletView", "WalletFrame", "QVBoxLayout", "QGroupBox" }; - std::vector<QString> vecIgnoreObjects{ + static const std::vector<QString> vecIgnoreObjects{ "embeddedFont_label_1", "embeddedFont_label_9", "messagesWidget", "systemFont_label_1", "systemFont_label_9" };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/qt/forms/optionsdialog.ui(1 hunks)src/qt/guiutil_font.cpp(1 hunks)src/qt/guiutil_font.h(1 hunks)src/qt/optionsdialog.cpp(3 hunks)src/qt/optionsmodel.cpp(8 hunks)src/qt/optionsmodel.h(4 hunks)src/qt/overviewpage.cpp(4 hunks)src/qt/overviewpage.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/qt/overviewpage.h
- src/qt/optionsmodel.h
- src/qt/optionsdialog.cpp
- src/qt/overviewpage.cpp
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/qt/guiutil_font.hsrc/qt/optionsmodel.cppsrc/qt/guiutil_font.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (3)
src/qt/guiutil_font.h (1)
src/qt/guiutil_font.cpp (60)
fontFamilyFromString(56-64)fontFamilyFromString(56-56)fontFamilyToString(66-70)fontFamilyToString(66-66)getFontFamily(72-75)getFontFamily(72-72)setFontFamily(77-82)setFontFamily(77-77)weightFromArg(84-103)weightFromArg(84-84)weightToArg(105-120)weightToArg(105-105)toQFontWeight(122-125)toQFontWeight(122-122)getFontWeightNormal(127-133)getFontWeightNormal(127-127)setFontWeightNormal(135-142)setFontWeightNormal(135-135)getFontWeightBold(144-150)getFontWeightBold(144-144)setFontWeightBold(152-159)setFontWeightBold(152-152)getFontScale(161-164)getFontScale(161-161)setFontScale(166-170)setFontScale(166-166)getScaledFontSize(172-175)getScaledFontSize(172-172)loadFonts(177-303)loadFonts(177-177)fontsLoaded(305-308)fontsLoaded(305-305)setApplicationFont(310-342)setApplicationFont(310-310)setFont(344-347)setFont(344-344)setFont(349-358)setFont(349-349)updateFonts(360-464)updateFonts(360-360)getFont(466-521)getFont(466-466)getFont(523-526)getFont(523-523)getFontNormal(528-531)getFontNormal(528-528)getFontBold(533-536)getFontBold(533-533)getSupportedFontWeightNormalDefault(538-544)getSupportedFontWeightNormalDefault(538-538)getSupportedFontWeightBoldDefault(546-552)getSupportedFontWeightBoldDefault(546-546)getSupportedWeights(554-558)getSupportedWeights(554-554)supportedWeightFromIndex(560-565)supportedWeightFromIndex(560-560)supportedWeightToIndex(567-576)supportedWeightToIndex(567-567)isSupportedWeight(578-581)isSupportedWeight(578-578)
src/qt/optionsmodel.cpp (1)
src/qt/guiutil_font.cpp (12)
fontFamilyToString(66-70)fontFamilyToString(66-66)weightToArg(105-120)weightToArg(105-105)supportedWeightToIndex(567-576)supportedWeightToIndex(567-567)getSupportedFontWeightNormalDefault(538-544)getSupportedFontWeightNormalDefault(538-538)weightFromArg(84-103)weightFromArg(84-84)getSupportedFontWeightBoldDefault(546-552)getSupportedFontWeightBoldDefault(546-546)
src/qt/guiutil_font.cpp (1)
src/qt/guiutil.cpp (4)
TextWidth(1268-1271)TextWidth(1268-1268)fixedPitchFont(238-244)fixedPitchFont(238-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: linux64_sqlite-test / Test source
- GitHub Check: linux64_nowallet-test / Test source
- GitHub Check: linux64-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
🔇 Additional comments (5)
src/qt/forms/optionsdialog.ui (2)
1058-1169: Monospace font selector UI: structure looks good.Block is cleanly scoped, naming is consistent, and it fits the Display page layout.
1079-1084: Radio exclusivity and model binding verified
Both QRadioButtons are children of the same font_groupBox (no runtime reparenting), so Qt’s auto-exclusivity applies. The QDataWidgetMapper maps embeddedFont_radioButton to UseEmbeddedMonospacedFont and OptionsModel’s getters/setters and signals correctly initialize and update the selection.src/qt/optionsmodel.cpp (2)
100-106: Defaults sourcing from g_font_defaults looks goodUsing GUIUtil::g_font_defaults for family/scale/weights and guarding runtime application with GUIUtil::fontsLoaded() is sound.
Also applies to: 110-118, 120-135, 137-154
812-816: Good: change persists and signal emittedThe new option is written to settings and broadcast with a dedicated signal. LGTM.
src/qt/guiutil_font.cpp (1)
266-279: LGTM: best-match search for supported weightsClosest-weight selection with tie handling is clear and efficient.
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 (2)
src/qt/optionsmodel.cpp (2)
120-135: Avoid using an uninitialized QFont::Weight when weightFromArg() fails.weightFromArg() returns bool; on failure,
weightremains uninitialized and is then used in isSupportedWeight(), leading to UB. Initialize and guard the parse.Apply this diff inside the
if (GUIUtil::fontsLoaded())block:- QFont::Weight weight; - GUIUtil::weightFromArg(settings.value("fontWeightNormal").toInt(), weight); + QFont::Weight weight = GUIUtil::getSupportedWeights().front(); // safe default + if (!GUIUtil::weightFromArg(settings.value("fontWeightNormal").toInt(), weight)) { + // keep default on parse failure + } if (!GUIUtil::isSupportedWeight(weight)) { // If the currently selected weight is not supported fallback to the lightest weight for normal font. weight = GUIUtil::getSupportedWeights().front(); settings.setValue("fontWeightNormal", GUIUtil::weightToArg(weight)); } GUIUtil::setFontWeightNormal(weight);
137-154: Same uninitialized-weight risk for bold path.Mirror the defensive parse and initialization for bold as well.
- QFont::Weight weight; - GUIUtil::weightFromArg(settings.value("fontWeightBold").toInt(), weight); + auto vecSupported = GUIUtil::getSupportedWeights(); + QFont::Weight weight = vecSupported[vecSupported.size() > 1 ? 1 : 0]; // default bold-ish + if (!GUIUtil::weightFromArg(settings.value("fontWeightBold").toInt(), weight)) { + // keep default on parse failure + } if (!GUIUtil::isSupportedWeight(weight)) { // If the currently selected weight is not supported fallback to the second lightest weight for bold font // or the lightest if there is only one. - auto vecSupported = GUIUtil::getSupportedWeights(); weight = vecSupported[vecSupported.size() > 1 ? 1 : 0]; settings.setValue("fontWeightBold", GUIUtil::weightToArg(weight)); } GUIUtil::setFontWeightBold(weight);
🧹 Nitpick comments (7)
src/qt/bitcoin.cpp (5)
492-496: Avoid hard-coding acceptable font-family values in help text.“SystemDefault, Montserrat” is duplicated knowledge and can drift from GUIUtil::AVAILABLE_FONTS (and from what fontFamilyFromString accepts, e.g., RobotoMono). Prefer deriving this string from a single source (e.g., a GUIUtil helper that returns a comma-separated list).
Would you like a small helper (e.g., GUIUtil::fontFamiliesHelp()) added to guiutil_font.{h,cpp} and used here to keep help text in sync?
680-680: Use QString::fromStdString for encoding correctness.Constructing QString from c_str() uses local 8-bit; fromStdString is clearer and consistent elsewhere.
- QString strFamily = gArgs.GetArg("-font-family", GUIUtil::fontFamilyToString(GUIUtil::g_font_defaults.family).toStdString()).c_str(); + QString strFamily = QString::fromStdString( + gArgs.GetArg("-font-family", GUIUtil::fontFamilyToString(GUIUtil::g_font_defaults.family).toStdString()));
693-693: Align default argument source with centralized defaults.Inside IsArgSet(), GetIntArg’s default isn’t used, but keeping it consistent reduces confusion.
- if (!GUIUtil::weightFromArg(gArgs.GetIntArg("-font-weight-normal", GUIUtil::weightToArg(GUIUtil::getFontWeightNormal())), weight)) { + if (!GUIUtil::weightFromArg(gArgs.GetIntArg("-font-weight-normal", GUIUtil::weightToArg(GUIUtil::g_font_defaults.weight_normal)), weight)) {
703-703: Same consistency tweak for bold weight.- if (!GUIUtil::weightFromArg(gArgs.GetIntArg("-font-weight-bold", GUIUtil::weightToArg(GUIUtil::getFontWeightBold())), weight)) { + if (!GUIUtil::weightFromArg(gArgs.GetIntArg("-font-weight-bold", GUIUtil::weightToArg(GUIUtil::g_font_defaults.weight_bold)), weight)) {
711-719: Also align -font-scale default to g_font_defaults for consistency.Same rationale as above.
- int nScale = gArgs.GetIntArg("-font-scale", GUIUtil::getFontScale()); + int nScale = gArgs.GetIntArg("-font-scale", GUIUtil::g_font_defaults.scale);src/qt/optionsmodel.cpp (2)
332-336: Store the default as a boolean, not a string.Minor type consistency nit: use a bool to avoid relying on string-to-bool conversion.
- if (!settings.contains("UseEmbeddedMonospacedFont")) { - settings.setValue("UseEmbeddedMonospacedFont", "true"); - } + if (!settings.contains("UseEmbeddedMonospacedFont")) { + settings.setValue("UseEmbeddedMonospacedFont", true); + }
812-816: Emit change signal only on actual change to prevent redundant updates.Bring in line with nearby options (e.g., ShowTrayIcon, CoinJoin flags).
- case UseEmbeddedMonospacedFont: - m_use_embedded_monospaced_font = value.toBool(); - settings.setValue("UseEmbeddedMonospacedFont", m_use_embedded_monospaced_font); - Q_EMIT useEmbeddedMonospacedFontChanged(m_use_embedded_monospaced_font); - break; + case UseEmbeddedMonospacedFont: { + const bool new_val = value.toBool(); + if (m_use_embedded_monospaced_font != new_val) { + m_use_embedded_monospaced_font = new_val; + settings.setValue("UseEmbeddedMonospacedFont", m_use_embedded_monospaced_font); + Q_EMIT useEmbeddedMonospacedFontChanged(m_use_embedded_monospaced_font); + } + break; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (38)
src/Makefile.qt.include(2 hunks)src/qt/addressbookpage.cpp(1 hunks)src/qt/addresstablemodel.cpp(1 hunks)src/qt/appearancewidget.cpp(1 hunks)src/qt/appearancewidget.h(1 hunks)src/qt/askpassphrasedialog.cpp(1 hunks)src/qt/bitcoin.cpp(3 hunks)src/qt/bitcoingui.cpp(1 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/forms/optionsdialog.ui(1 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/guiutil.cpp(0 hunks)src/qt/guiutil.h(0 hunks)src/qt/guiutil_font.cpp(1 hunks)src/qt/guiutil_font.h(1 hunks)src/qt/masternodelist.cpp(1 hunks)src/qt/modaloverlay.cpp(1 hunks)src/qt/openuridialog.cpp(1 hunks)src/qt/optionsdialog.cpp(3 hunks)src/qt/optionsmodel.cpp(8 hunks)src/qt/optionsmodel.h(4 hunks)src/qt/overviewpage.cpp(4 hunks)src/qt/overviewpage.h(1 hunks)src/qt/proposalwizard.cpp(1 hunks)src/qt/qrdialog.cpp(1 hunks)src/qt/qrimagewidget.cpp(1 hunks)src/qt/receivecoinsdialog.cpp(1 hunks)src/qt/receiverequestdialog.cpp(1 hunks)src/qt/rpcconsole.cpp(1 hunks)src/qt/sendcoinsdialog.cpp(1 hunks)src/qt/sendcoinsentry.cpp(1 hunks)src/qt/signverifymessagedialog.cpp(1 hunks)src/qt/splashscreen.cpp(1 hunks)src/qt/test/apptests.cpp(1 hunks)src/qt/trafficgraphwidget.cpp(1 hunks)src/qt/transactiondescdialog.cpp(1 hunks)src/qt/utilitydialog.cpp(1 hunks)src/qt/walletview.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- src/qt/guiutil.h
- src/qt/guiutil.cpp
🚧 Files skipped from review as they are similar to previous changes (32)
- src/qt/test/apptests.cpp
- src/qt/addressbookpage.cpp
- src/qt/openuridialog.cpp
- src/qt/appearancewidget.h
- src/qt/receivecoinsdialog.cpp
- src/qt/appearancewidget.cpp
- src/qt/splashscreen.cpp
- src/qt/utilitydialog.cpp
- src/qt/sendcoinsdialog.cpp
- src/qt/modaloverlay.cpp
- src/qt/transactiondescdialog.cpp
- src/qt/governancelist.cpp
- src/qt/signverifymessagedialog.cpp
- src/qt/trafficgraphwidget.cpp
- src/qt/walletview.cpp
- src/qt/sendcoinsentry.cpp
- src/qt/qrdialog.cpp
- src/qt/coincontroldialog.cpp
- src/qt/optionsmodel.h
- src/qt/addresstablemodel.cpp
- src/qt/masternodelist.cpp
- src/qt/askpassphrasedialog.cpp
- src/qt/receiverequestdialog.cpp
- src/qt/rpcconsole.cpp
- src/qt/overviewpage.h
- src/qt/guiutil_font.cpp
- src/qt/forms/optionsdialog.ui
- src/qt/bitcoingui.cpp
- src/qt/guiutil_font.h
- src/qt/overviewpage.cpp
- src/qt/optionsdialog.cpp
- src/qt/qrimagewidget.cpp
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/qt/bitcoin.cppsrc/qt/proposalwizard.cppsrc/qt/optionsmodel.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (2)
src/qt/bitcoin.cpp (1)
src/qt/guiutil_font.cpp (4)
fontFamilyToString(66-70)fontFamilyToString(66-66)weightToArg(105-120)weightToArg(105-105)
src/qt/optionsmodel.cpp (1)
src/qt/guiutil_font.cpp (12)
fontFamilyToString(66-70)fontFamilyToString(66-66)weightToArg(105-120)weightToArg(105-105)supportedWeightToIndex(567-576)supportedWeightToIndex(567-567)getSupportedFontWeightNormalDefault(538-544)getSupportedFontWeightNormalDefault(538-538)weightFromArg(84-103)weightFromArg(84-84)getSupportedFontWeightBoldDefault(546-552)getSupportedFontWeightBoldDefault(546-546)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (9)
src/Makefile.qt.include (2)
231-232: Adding qt/guiutil_font.cpp to sources is correct.This ensures the new font utility is compiled and linked into libbitcoinqt.
140-141: Build integration: no MOC required for guiutil_font.h
src/qt/guiutil_font.h contains no Q_OBJECT/Q_GADGET/Q_NAMESPACE/Q_ENUM usage; moc_guiutil_font.cpp entry is not needed.src/qt/proposalwizard.cpp (1)
16-16: Include placement LGTM.Including guiutil_font.h enables the GUIUtil::setFont usage below; no other changes needed here.
src/qt/bitcoin.cpp (1)
26-26: Include addition LGTM.Required for centralized font defaults and helpers.
src/qt/optionsmodel.cpp (5)
15-15: Include of guiutil_font.h looks correct.Header dependency is appropriate for the new centralized font APIs.
100-106: Default font family wired to g_font_defaults — LGTM.Keeps GUI defaults aligned with centralized font settings.
110-118: Default font scale wired to g_font_defaults — LGTM.Consistent with the new module.
564-566: Expose UseEmbeddedMonospacedFont via data() — LGTM.Matches the new option and signal plumbing.
546-551: Guard against uninitialized fonts to prevent data() from throwing
supportedWeightToIndex() can return –1 before fonts load, and getSupportedFontWeightNormalDefault() may throw in that state. Use the vector of currently available weights and fall back to index 0 instead of invoking the throwing helper:- int nIndex = GUIUtil::supportedWeightToIndex(weight); - if (nIndex == -1) { - nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightNormalDefault()); - } - assert(nIndex != -1); - return nIndex; + const auto vec = GUIUtil::getSupportedWeights(); + if (vec.empty()) { + return 0; // safest fallback until fonts load + } + int nIndex = GUIUtil::supportedWeightToIndex(weight); + if (nIndex == -1) { + nIndex = 0; // use lightest/first as “normal” fallback + } + return nIndex;
| if (nIndex == -1) { | ||
| nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightBoldDefault()); | ||
| } | ||
| assert(nIndex != -1); | ||
| return nIndex; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Apply the same non-throwing fallback for bold weight.
Mirror the robust logic used above and avoid calling getSupportedFontWeightBoldDefault() in data().
- int nIndex = GUIUtil::supportedWeightToIndex(weight);
- if (nIndex == -1) {
- nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightBoldDefault());
- }
- assert(nIndex != -1);
- return nIndex;
+ const auto vec = GUIUtil::getSupportedWeights();
+ if (vec.empty()) {
+ return 0; // safest fallback
+ }
+ int nIndex = GUIUtil::supportedWeightToIndex(weight);
+ if (nIndex == -1) {
+ nIndex = vec.size() > 1 ? 1 : 0; // second-lightest for "bold" when possible
+ }
+ return nIndex;Gather file context and utility function definitions
🏁 Script executed:
#!/bin/bash
# Locate optionsmodel.cpp
path=$(fd -HI optionsmodel.cpp)
echo "File: $path"
# Show context around the snippet (lines ~540–610)
sed -n '540,610p' "$path"
# Find getSupportedWeights in the codebase
rg -nP 'getSupportedWeights' -C2 .
# Find getSupportedFontWeightBoldDefault in the codebase
rg -nP 'getSupportedFontWeightBoldDefault' -C2 .Length of output: 7299
Inspect default weight getters
🏁 Script executed:
#!/bin/bash
# Show getSupportedFontWeightNormalDefault and BoldDefault implementations
sed -n '530,600p' src/qt/guiutil_font.cppLength of output: 1458
Inspect population of default and supported weight maps
🏁 Script executed:
#!/bin/bash
# Find where mapDefaultWeights is defined and populated
grep -R "mapDefaultWeights" -n src/qt/guiutil_font.cpp
# Find where mapSupportedWeights is defined and populated
grep -R "mapSupportedWeights" -n src/qt/guiutil_font.cpp
# Show file header to see initialization logic
sed -n '1,120p' src/qt/guiutil_font.cppLength of output: 4727
Use non-throwing fallback for bold weight in data()
Replace the current logic in the FontWeightBold case to mirror the setData() fallback and avoid calling the throwing getSupportedFontWeightBoldDefault():
const auto vec = GUIUtil::getSupportedWeights();
if (vec.empty()) return 0;
int nIndex = GUIUtil::supportedWeightToIndex(weight);
if (nIndex == -1) nIndex = vec.size() > 1 ? 1 : 0;
return nIndex;This ensures a safe index even when the requested weight isn’t supported.
🤖 Prompt for AI Agents
In src/qt/optionsmodel.cpp around lines 556 to 561, the FontWeightBold branch in
data() currently calls the potentially-throwing
getSupportedFontWeightBoldDefault() and asserts the result; instead, use a
non-throwing fallback like setData() does: retrieve the vector from
GUIUtil::getSupportedWeights(), return 0 if empty, compute nIndex via
GUIUtil::supportedWeightToIndex(weight), and if nIndex == -1 set it to 1 if
vec.size() > 1 else 0, then return nIndex so a safe index is always returned
without throwing.
| @@ -11,6 +11,7 @@ | |||
| #include <qt/askpassphrasedialog.h> | |||
| #include <qt/clientmodel.h> | |||
| #include <qt/guiutil.h> | |||
| #include <qt/guiutil_font.h> | |||
There was a problem hiding this comment.
I see there a lot of guiutil_font.h is added but I see not a single include guiutil.h removed; is it expecting?
@coderabbitai please check if there is any include <qt/guiutil.h> is no more needed in any file that changed in d6ca4efb899b92099dbfc9fbb2d6e5cd11c99f17
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| namespace GUIUtil { |
There was a problem hiding this comment.
it's a bit strange that fixedPitchFont, calculateIdealFontSize, TextWidth are still in guiutil.h; but I guess it's fine.
There was a problem hiding this comment.
They're all from upstream and thus not movable as Dash-specific code
fixedPitchFontwas introduced in bitcoin#6864 (source)calculateIdealFontSizewas introduced in bitcoin#12173 (source)TextWidthwas introduced in bitcoin#16701 (source)
src/qt/guiutil_font.cpp
Outdated
| assert(false); | ||
| } | ||
| assert(family <= FontFamily::MaxVal); | ||
| return AVAILABLE_FONTS[static_cast<size_t>(family)].second; |
There was a problem hiding this comment.
probably you need here ToUnderlying from util/underlying.h
| @@ -302,7 +302,8 @@ void OverviewPage::setClientModel(ClientModel *model) | |||
| // Show warning, for example if this is a prerelease version | |||
| connect(model, &ClientModel::alertsChanged, this, &OverviewPage::updateAlerts); | |||
| updateAlerts(model->getStatusBarWarnings()); | |||
| setMonospacedFont(false); | |||
| connect(model->getOptionsModel(), &OptionsModel::useEmbeddedMonospacedFontChanged, this, &OverviewPage::setMonospacedFont); | |||
| setMonospacedFont(model->getOptionsModel()->getUseEmbeddedMonospacedFont()); | |||
There was a problem hiding this comment.
does it really need to be configured via settings?
Too many settings for users is not always better :)
I'd suggest to just enable it for everyone
Is it backported code, right?
| @@ -543,13 +543,19 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const | |||
| QFont::Weight weight; | |||
| GUIUtil::weightFromArg(settings.value("fontWeightNormal").toInt(), weight); | |||
| int nIndex = GUIUtil::supportedWeightToIndex(weight); | |||
| if (nIndex == -1) { | |||
| nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightNormalDefault()); | |||
| } | |||
| assert(nIndex != -1); | |||
There was a problem hiding this comment.
-assert(nIndex != -1)It's a bit abundant now
There was a problem hiding this comment.
Would still prefer to keep it, there is still a conversion going on and if defaults break for some reason, we should know about it.
| assert(nIndex != -1); | ||
| return nIndex; | ||
| } | ||
| case FontWeightBold: { | ||
| QFont::Weight weight; | ||
| GUIUtil::weightFromArg(settings.value("fontWeightBold").toInt(), weight); | ||
| int nIndex = GUIUtil::supportedWeightToIndex(weight); | ||
| if (nIndex == -1) { | ||
| nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightBoldDefault()); | ||
| } | ||
| assert(nIndex != -1); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/qt/optionsmodel.cpp (1)
556-561: Mirror non-throwing bold fallback in data(); drop assert.
Same rationale as above; prefer index fallback over throwing default.- QFont::Weight weight; - GUIUtil::weightFromArg(settings.value("fontWeightBold").toInt(), weight); - int nIndex = GUIUtil::supportedWeightToIndex(weight); - if (nIndex == -1) { - nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightBoldDefault()); - } - assert(nIndex != -1); - return nIndex; + QFont::Weight weight; + GUIUtil::weightFromArg(settings.value("fontWeightBold").toInt(), weight); + const auto vec = GUIUtil::getSupportedWeights(); + if (vec.empty()) return 0; + int nIndex = GUIUtil::supportedWeightToIndex(weight); + if (nIndex == -1) nIndex = vec.size() > 1 ? 1 : 0; // second-lightest for "bold" when possible + return nIndex;
🧹 Nitpick comments (4)
src/qt/optionsmodel.cpp (4)
120-135: Avoid potential UB when supported weights vector is empty.
Guard before using front(); pick a sane fallback.Apply this diff within the unsupported-weight branch:
- // If the currently selected weight is not supported fallback to the lightest weight for normal font. - weight = GUIUtil::getSupportedWeights().front(); - settings.setValue("fontWeightNormal", GUIUtil::weightToArg(weight)); + // If the selected weight isn't supported, choose lightest supported + // (or Normal if none reported for any reason). + const auto vecSupported = GUIUtil::getSupportedWeights(); + if (vecSupported.empty()) { + weight = QFont::Normal; + } else { + weight = vecSupported.front(); + } + settings.setValue("fontWeightNormal", GUIUtil::weightToArg(weight));
137-154: Bold fallback can OOB-index when no supported weights are reported.
Protect against empty vector; choose Bold (or the best available index).- auto vecSupported = GUIUtil::getSupportedWeights(); - weight = vecSupported[vecSupported.size() > 1 ? 1 : 0]; + const auto vecSupported = GUIUtil::getSupportedWeights(); + if (vecSupported.empty()) { + weight = QFont::Bold; + } else { + weight = vecSupported.size() > 1 ? vecSupported[1] : vecSupported[0]; + } settings.setValue("fontWeightBold", GUIUtil::weightToArg(weight));
332-337: Use a boolean literal for the default and (optionally) gate the signal.
The current default writes a string "true". Prefer a boolean; emitting only on change avoids redundant UI work.- settings.setValue("UseEmbeddedMonospacedFont", "true"); + settings.setValue("UseEmbeddedMonospacedFont", true); - m_use_embedded_monospaced_font = settings.value("UseEmbeddedMonospacedFont").toBool(); - Q_EMIT useEmbeddedMonospacedFontChanged(m_use_embedded_monospaced_font); + const bool new_val = settings.value("UseEmbeddedMonospacedFont").toBool(); + if (m_use_embedded_monospaced_font != new_val) { + m_use_embedded_monospaced_font = new_val; + Q_EMIT useEmbeddedMonospacedFontChanged(m_use_embedded_monospaced_font); + } else { + m_use_embedded_monospaced_font = new_val; + }
812-816: Emit signal only on change to avoid redundant updates.
Currently always emits even if value unchanged.- m_use_embedded_monospaced_font = value.toBool(); - settings.setValue("UseEmbeddedMonospacedFont", m_use_embedded_monospaced_font); - Q_EMIT useEmbeddedMonospacedFontChanged(m_use_embedded_monospaced_font); + const bool new_val = value.toBool(); + if (m_use_embedded_monospaced_font != new_val) { + m_use_embedded_monospaced_font = new_val; + settings.setValue("UseEmbeddedMonospacedFont", m_use_embedded_monospaced_font); + Q_EMIT useEmbeddedMonospacedFontChanged(m_use_embedded_monospaced_font); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
src/Makefile.qt.include(2 hunks)src/qt/addressbookpage.cpp(1 hunks)src/qt/addresstablemodel.cpp(1 hunks)src/qt/appearancewidget.cpp(1 hunks)src/qt/appearancewidget.h(1 hunks)src/qt/askpassphrasedialog.cpp(1 hunks)src/qt/bitcoin.cpp(3 hunks)src/qt/bitcoingui.cpp(1 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/forms/optionsdialog.ui(1 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/guiutil.cpp(0 hunks)src/qt/guiutil.h(0 hunks)src/qt/guiutil_font.cpp(1 hunks)src/qt/guiutil_font.h(1 hunks)src/qt/masternodelist.cpp(1 hunks)src/qt/modaloverlay.cpp(1 hunks)src/qt/openuridialog.cpp(1 hunks)src/qt/optionsdialog.cpp(3 hunks)src/qt/optionsmodel.cpp(8 hunks)src/qt/optionsmodel.h(4 hunks)src/qt/overviewpage.cpp(4 hunks)src/qt/overviewpage.h(1 hunks)src/qt/proposalwizard.cpp(1 hunks)src/qt/qrdialog.cpp(1 hunks)src/qt/qrimagewidget.cpp(1 hunks)src/qt/receivecoinsdialog.cpp(1 hunks)src/qt/receiverequestdialog.cpp(1 hunks)src/qt/rpcconsole.cpp(1 hunks)src/qt/sendcoinsdialog.cpp(1 hunks)src/qt/sendcoinsentry.cpp(1 hunks)src/qt/signverifymessagedialog.cpp(1 hunks)src/qt/splashscreen.cpp(1 hunks)src/qt/test/apptests.cpp(1 hunks)src/qt/trafficgraphwidget.cpp(1 hunks)src/qt/transactiondescdialog.cpp(1 hunks)src/qt/utilitydialog.cpp(1 hunks)src/qt/walletview.cpp(1 hunks)test/util/data/non-backported.txt(1 hunks)
💤 Files with no reviewable changes (2)
- src/qt/guiutil.h
- src/qt/guiutil.cpp
✅ Files skipped from review due to trivial changes (9)
- src/qt/modaloverlay.cpp
- test/util/data/non-backported.txt
- src/qt/receivecoinsdialog.cpp
- src/qt/sendcoinsentry.cpp
- src/qt/transactiondescdialog.cpp
- src/qt/trafficgraphwidget.cpp
- src/qt/sendcoinsdialog.cpp
- src/qt/proposalwizard.cpp
- src/qt/splashscreen.cpp
🚧 Files skipped from review as they are similar to previous changes (26)
- src/qt/bitcoingui.cpp
- src/qt/utilitydialog.cpp
- src/qt/rpcconsole.cpp
- src/qt/openuridialog.cpp
- src/qt/qrdialog.cpp
- src/qt/walletview.cpp
- src/qt/receiverequestdialog.cpp
- src/qt/test/apptests.cpp
- src/qt/masternodelist.cpp
- src/qt/addressbookpage.cpp
- src/qt/bitcoin.cpp
- src/qt/appearancewidget.h
- src/qt/askpassphrasedialog.cpp
- src/qt/qrimagewidget.cpp
- src/qt/appearancewidget.cpp
- src/qt/optionsmodel.h
- src/qt/addresstablemodel.cpp
- src/qt/governancelist.cpp
- src/qt/coincontroldialog.cpp
- src/qt/optionsdialog.cpp
- src/qt/overviewpage.cpp
- src/qt/overviewpage.h
- src/qt/guiutil_font.cpp
- src/Makefile.qt.include
- src/qt/guiutil_font.h
- src/qt/forms/optionsdialog.ui
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/qt/signverifymessagedialog.cppsrc/qt/optionsmodel.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (1)
src/qt/optionsmodel.cpp (1)
src/qt/guiutil_font.cpp (12)
fontFamilyToString(74-78)fontFamilyToString(74-74)weightToArg(113-128)weightToArg(113-113)supportedWeightToIndex(581-590)supportedWeightToIndex(581-581)getSupportedFontWeightNormalDefault(550-557)getSupportedFontWeightNormalDefault(550-550)weightFromArg(92-111)weightFromArg(92-92)getSupportedFontWeightBoldDefault(559-566)getSupportedFontWeightBoldDefault(559-559)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (5)
src/qt/signverifymessagedialog.cpp (1)
11-11: LGTM: correct direct include for font utilitiesIncluding qt/guiutil_font.h here properly declares GUIUtil::FontWeight and setFont/updateFonts used below, avoiding reliance on transitive includes. No functional changes.
src/qt/optionsmodel.cpp (4)
15-15: LGTM: correct header inclusion for new font utilities.
100-106: Default fontFamily sourced from g_font_defaults — good.
Aligns runtime defaults with GUIUtil state.
110-118: Default fontScale sourced from g_font_defaults — good.
564-566: LGTM: data() for UseEmbeddedMonospacedFont returns cached member.
| QFont::Weight weight; | ||
| GUIUtil::weightFromArg(settings.value("fontWeightNormal").toInt(), weight); | ||
| int nIndex = GUIUtil::supportedWeightToIndex(weight); | ||
| if (nIndex == -1) { | ||
| nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightNormalDefault()); | ||
| } | ||
| assert(nIndex != -1); | ||
| return nIndex; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Non-throwing fallback for normal weight in data().
Avoid calling getSupportedFontWeightNormalDefault() (can throw) and remove the assert; return a safe index.
- QFont::Weight weight;
- GUIUtil::weightFromArg(settings.value("fontWeightNormal").toInt(), weight);
- int nIndex = GUIUtil::supportedWeightToIndex(weight);
- if (nIndex == -1) {
- nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightNormalDefault());
- }
- assert(nIndex != -1);
- return nIndex;
+ QFont::Weight weight;
+ GUIUtil::weightFromArg(settings.value("fontWeightNormal").toInt(), weight);
+ const auto vec = GUIUtil::getSupportedWeights();
+ if (vec.empty()) return 0;
+ int nIndex = GUIUtil::supportedWeightToIndex(weight);
+ if (nIndex == -1) nIndex = 0; // lightest supported
+ return nIndex;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| QFont::Weight weight; | |
| GUIUtil::weightFromArg(settings.value("fontWeightNormal").toInt(), weight); | |
| int nIndex = GUIUtil::supportedWeightToIndex(weight); | |
| if (nIndex == -1) { | |
| nIndex = GUIUtil::supportedWeightToIndex(GUIUtil::getSupportedFontWeightNormalDefault()); | |
| } | |
| assert(nIndex != -1); | |
| return nIndex; | |
| } | |
| QFont::Weight weight; | |
| GUIUtil::weightFromArg(settings.value("fontWeightNormal").toInt(), weight); | |
| const auto vec = GUIUtil::getSupportedWeights(); | |
| if (vec.empty()) return 0; | |
| int nIndex = GUIUtil::supportedWeightToIndex(weight); | |
| if (nIndex == -1) nIndex = 0; // lightest supported | |
| return nIndex; |
🤖 Prompt for AI Agents
In src/qt/optionsmodel.cpp around lines 543-551, avoid calling the
potentially-throwing GUIUtil::getSupportedFontWeightNormalDefault(), remove the
assert, and return a safe index instead: if
GUIUtil::supportedWeightToIndex(weight) returns -1, set nIndex to a safe
fallback (for example 0 or the first supported index) without calling
getSupportedFontWeightNormalDefault(), drop the assert(nIndex != -1), and return
that safe index.
src/qt/guiutil_font.cpp
Outdated
| QString fontFamilyToString(FontFamily family) | ||
| { | ||
| assert(family <= FontFamily::MaxVal); | ||
| return AVAILABLE_FONTS[static_cast<size_t>(family)].family_name; |
There was a problem hiding this comment.
nit: use here ToUnderlying
| return AVAILABLE_FONTS[static_cast<size_t>(family)].family_name; | |
| return AVAILABLE_FONTS[ToUnderlying(family)].family_name; |
and also
+#include <util/underlying.h>
src/qt/guiutil_font.cpp
Outdated
| qDebug() << __func__ << ": " << qApp->font().toString() << " family: " << qApp->font().family() | ||
| << ", style: " << qApp->font().styleName() << " match: " << qApp->font().exactMatch(); |
There was a problem hiding this comment.
nit: qDebug with too many params is a bit messy. Consider:
| qDebug() << __func__ << ": " << qApp->font().toString() << " family: " << qApp->font().family() | |
| << ", style: " << qApp->font().styleName() << " match: " << qApp->font().exactMatch(); | |
| qDebug() << strprintf("%s: %s family: %s, style: %s, match: %s", __func__, qApp->font().toString(), qApp->font().family(), qApp->font().styleName(), qApp->font().exactMatch()); |
src/qt/guiutil_font.cpp
Outdated
| ++nUpdated; | ||
| } | ||
| } | ||
| qDebug().nospace() << __func__ << " - widget counts: updated/updatable/total(" << nUpdated << "/" << nUpdatable |
There was a problem hiding this comment.
nit: consider using here strprintf() to increase readability
src/qt/guiutil_font.cpp
Outdated
| } | ||
|
|
||
| if (gArgs.GetBoolArg("-debug-ui", false)) { | ||
| qDebug() << __func__ << ": font size: " << font.pointSizeF() << " family: " << font.family() |
|
Code wise looks good but I don't like the new look at all tbh, would rather prefer an option to disable monospace font completely and preserve the current look. Also, we don't really need examples in Suggestions:
|
…,h}` Review with `git log -p -n1 --color-moved=dimmed_zebra`.
Currently they're a bunch of loose variables with dedicated getter functions, we can cut down on it a fair bit.
Additional Information
develop(e23a658)The monospace font preferences aren't applied during construction, this is because preferences are read when the client model is set (source). To avoid the labels appearing as unexpectedly variable-width until the preferences are applied,
setMonospacedFont()is called during construction (source) to ensure that it is at least monospace.This doesn't entirely avoid the mismatch if there the user opts for the pre-packaged font as the client model needs to be set for that preference to be applied but the goal is to ensure that a monospace font is still applied.
Adjustments were required to the padding and font size in the monospace font selector as defaults from the upstream pull request were visually off (see below, compare with screenshot above). They were also blocklisted from font updates to ensure they aren't affected by customization changes.
Screenshot:
While Roboto Mono is not a selectable font during program startup (source), it is recognized as a valid argument (alongside the similarly placed
DefaultMonospace) by-font-family. This is not a supported case and may be liable to break in future releases.Selection Pane:
Client started with command line setting:
Additional fixes have been included to deal with crash cases encountered when working on this pull request, the first fix uses fallback weights if
fontWeight{Bold,Normal}is set to a value not supported by the font. This is possible if the values are manually edited or overridden.Crash:
The second fix uses the argument passed to the lambda
addBestDefaultsinstead of a global font, this has also been flagged by CodeRabbit (comment)Crash:
Breaking Changes
Balance labels and some CoinJoin control labels on the overview page now utilize monospace fonts.
Checklist