backport: merge bitcoin-core/gui#79, #497 (allow different font for overview page balances) #6831
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@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.
📥 Commits
Reviewing files that changed from the base of the PR and between 873dee0 and b50aafd6463d8d7b6a34d9bc24333027bab3924f.
📒 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.
|
@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.
📥 Commits
Reviewing files that changed from the base of the PR and between b50aafd6463d8d7b6a34d9bc24333027bab3924f and 3abaeee.
📒 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.
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.
📥 Commits
Reviewing files that changed from the base of the PR and between 3abaeee and 8dba4fb09178bf4cf4795d7d9371140f29dc43eb.
📒 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.
📥 Commits
Reviewing files that changed from the base of the PR and between 8dba4fb09178bf4cf4795d7d9371140f29dc43eb and ee6865bf3b92952a627cf38e8b9ddbbdb44643c5.
📒 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;
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
LGTM overall, but check comments!
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
📥 Commits
Reviewing files that changed from the base of the PR and between ee6865bf3b92952a627cf38e8b9ddbbdb44643c5 and faa5f3d4f6926fa82e4f011bfa8834691db33d15.
📒 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.
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
utACK faa5f3d4f6926fa82e4f011bfa8834691db33d15
|
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:
|
kwvg
requested review from
PastaPastaPasta,
UdjinM6 and
knst
and removed request for
PastaPastaPasta and
UdjinM6
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
… font specifically
Dash already has an appearance widget for managing font parameters, the selector shouldn't be in "Display". Review with `git log -p -n1 --color-moved=dimmed_zebra`.
Also: - Refactor `AppearanceWidget` ctor to bunch `connect` calls - Add refresh of money preview if using default font and font choice or boldness changed
Wallet model is initialized earlier, this will apply our font preferences sooner which should prevent the font from not showing correctly until tabs are changed.
- Add updateMoneyFont() slot to update font immediately on selection - Save original font choice to restore on Cancel - Include full optionsmodel.h header for FontChoice type usage - Matches behavior of other appearance settings (theme, font family, etc.)
- Remove moneyFont_preview label from appearance dialog since Overview page now shows live preview - Remove updateMoneyPreview() slot and related signal connections - Simplify setupFontOptions() to remove preview parameter - Remove moneyFont_preview from vecIgnoreObjects in guiutil_font.cpp - Simplify getFontForChoice() by removing unused weight/size settings since callers only use the font family (weight and size are overridden by setMonospacedFont()) - Use g_font_registry.GetFont() directly for ApplicationFont case - Set consistent width (282px) for moneyFont combobox to match other comboboxes in the dialog
…603 (migrate some QSettings to settings.json) 076ce3d fix: only reset GUI-managed settings in -resetguisettings (Kittywhiskers Van Gogh) 0c3b224 merge bitcoin-core/gui#603: Add settings.json prune-prev, proxy-prev, onion-prev settings (Kittywhiskers Van Gogh) 45976c7 merge bitcoin-core/gui#701: Persist Mask Values option (Kittywhiskers Van Gogh) e429437 qt: migrate `-font-weight-bold` from QSettings to settings.json (Kittywhiskers Van Gogh) 70ff8ef qt: migrate `-font-weight-normal` from QSettings to settings.json (Kittywhiskers Van Gogh) c3a5ba1 qt: migrate `-font-scale` setting from QSettings to settings.json (Kittywhiskers Van Gogh) a5b5ede qt: migrate `-font-family` setting from QSettings to settings.json (Kittywhiskers Van Gogh) 2bb8106 qt: migrate `-enablecoinjoin` setting from QSettings to settings.json (Kittywhiskers Van Gogh) da15040 qt: migrate `-coinjoindenomshardcap` setting from QSettings to settings.json (Kittywhiskers Van Gogh) 4f744c2 qt: migrate `-coinjoindenomsgoal` setting from QSettings to settings.json (Kittywhiskers Van Gogh) ea60d79 qt: migrate `-coinjoinmultisession` setting from QSettings to settings.json (Kittywhiskers Van Gogh) fb97375 qt: migrate `-coinjoinamount` setting from QSettings to settings.json (Kittywhiskers Van Gogh) 2174fc6 qt: migrate `-coinjoinrounds` setting from QSettings to settings.json (Kittywhiskers Van Gogh) 44833d9 qt: migrate `-coinjoinsessions` setting from QSettings to settings.json (Kittywhiskers Van Gogh) eed631a refactor: spin-off list of option IDs that require string workaround (Kittywhiskers Van Gogh) dd21992 merge bitcoin-core/gui#602: Unify bitcoin-qt and bitcoind persistent settings (Kittywhiskers Van Gogh) d7cc771 qt: move Prune{,Size} handling outside `ENABLE_WALLET` gate (Kittywhiskers Van Gogh) bb2efec qt: remove PrivateSend -> CoinJoin migration logic, remove old values (Kittywhiskers Van Gogh) 4fc25af merge bitcoin-core/gui#601: Pass interfaces::Node references to OptionsModel constructor (Kittywhiskers Van Gogh) 6c9f1ae merge bitcoin-core/gui#600: Add OptionsModel getOption/setOption methods (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6901 * Depends on #7068 * Depends on #6831 ## Breaking Changes See release notes. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: ACK 076ce3d Tree-SHA512: 5ee94e3fbad13792fa01ae4269f2184356a9359aba9ee1e0b0d5195fa1c32febeed56d41aa2add121c1cbc5d20dc0305bf3a617dea0c8eb35e92b29fd2974f42
4 participants
Additional Information
Depends on feat(qt): extract Dash-specific font infrastructure to
qt/guiutil_font.{cpp,h}, encapsulate globals intoFont{Info,Registry}, add arbitrary font support #7068Depends on fix: don't assume that successful
weightFromArglinks to valid index #7084Dependency for backport: merge bitcoin-core/gui#600, #601, #602, #701, #603 (migrate some QSettings to settings.json) #6833
Based on review suggestions given (comment), bitcoin-core/gui#497 was also backported to allow arbitrary font specification. This also allowed us to offer a "Use existing font" option that doesn't use any monospace font but instead inherits whatever font is used in the rest of the application. This is set as the default (source), rendering monospace balance counters opt-in.
Unlike Bitcoin, Dash has a dedicated appearance widget for managing font settings specifically. We have opted to deviate from upstream by moving the money font control from "Display" to "Appearance" for the sake of consistency.
We set the money font when setting the wallet model as client model updates happen after the UI is visible to the user while wallet model updates happen before, there is little point in setting them to any values in the constructor as during construction, we don't have access to the options model and thus don't know what font is supposed to be used to begin with.
If a font is passed through the command line, it is considered "selectable" (source) because we still need to populate that font in the drop-down menu so that we don't report stale data. Though, the font cannot be set to that overridden value because command-line overridden controls are auto-disabled to prevent settings corruption (see 2f30002), which makes the "selectable" state more-or-less an internal detail.
An example is that the user tries to run Dash Core, the embedded monospace font, Roboto Mono, is not a selectable font (i.e. you cannot ordinarily set your client to use Roboto Mono, see below).
Screenshot:
But if an override is done, it will be respected, the client will start with Roboto Mono and the selectable status will be overwritten so that it is presented in the drop-down menu, albeit, with the menu disabled so it doesn't contaminate UI settings.
Screenshot:
Breaking Changes
None expected.
Checklist