backport: merge bitcoin-core/gui#600, #601, #602, #701, #603, merge bitcoin#15936 (migrate some QSettings to settings.json) #6833
Conversation
…,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.
…nsModel constructor
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe change introduces a new Qt font subsystem (qt/guiutil_font.{h,cpp}) and removes font APIs from guiutil.{h,cpp}, updating many files to include the new header. OptionsModel is refactored to require a Node reference, adds new APIs, returns errors from Init, and introduces a UseEmbeddedMonospacedFont option with corresponding UI and signals. BitcoinApplication’s createOptionsModel now returns bool and GUI startup order is adjusted. The public Node interface gains settings-management methods with implementations. util::GetSetting adds an ignore_nonpersistent flag; ArgsManager gains backup-capable settings file writes and GetPersistentSetting plus conversion helpers. UI forms gain a monospaced font group. Tests are updated accordingly. Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
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 CI: clang-format include-order violation.The pipeline failed due to include order changes. Please run the repo’s clang-format step (e.g., contrib/devtools/clang-format-diff.py -p1) or reorder the includes per project style to satisfy CI.
🧹 Nitpick comments (28)
src/qt/test/test_main.cpp (1)
63-66: Prefer explicit scope over IILE for BasicTestingSetup; also keep creation within the same lifetime as directory creationThis avoids subtle lifetime semantics and reads clearer. Functionally equivalent.
- fs::create_directories([] { - BasicTestingSetup dummy{CBaseChainParams::REGTEST}; - return gArgs.GetDataDirNet() / "blocks"; - }()); + { + BasicTestingSetup dummy{CBaseChainParams::REGTEST}; + fs::create_directories(gArgs.GetDataDirNet() / "blocks"); + }Additionally, consider checking the return value or logging failures from create_directories.
src/qt/forms/optionsdialog.ui (1)
1058-1169: Monospaced font selector UI: looks good; add basic UX affordances
- Add short tooltips to both radio buttons to clarify “embedded” vs “system” choice.
- Optionally group them in a QButtonGroup for explicit exclusivity (not required, but future-proof if the layout changes).
- <widget class="QRadioButton" name="embeddedFont_radioButton"> + <widget class="QRadioButton" name="embeddedFont_radioButton"> <property name="text"> <string>embedded "%1"</string> </property> + <property name="toolTip"> + <string>Use the bundled monospaced font for amounts.</string> + </property> </widget> ... - <widget class="QRadioButton" name="systemFont_radioButton"> + <widget class="QRadioButton" name="systemFont_radioButton"> <property name="text"> <string>closest matching "%1"</string> </property> + <property name="toolTip"> + <string>Use the closest system-installed monospaced font for amounts.</string> + </property> </widget>src/qt/masternodelist.cpp (3)
233-240: Localize “UNKNOWN” next-payment text.Untranslated literal leaks into UI; use tr() for i18n.
- QString strNextPayment = "UNKNOWN"; + QString strNextPayment = tr("UNKNOWN");
149-151: Typo: nMnListUpdateSecods → nMnListUpdateSeconds.Minor readability fix; avoids propagating the misspelling.
- int64_t nMnListUpdateSecods = clientModel->masternodeSync().isBlockchainSynced() ? MASTERNODELIST_UPDATE_SECONDS : MASTERNODELIST_UPDATE_SECONDS * 10; - int64_t nSecondsToWait = nTimeUpdatedDIP3 - GetTime() + nMnListUpdateSecods; + int64_t nMnListUpdateSeconds = clientModel->masternodeSync().isBlockchainSynced() ? MASTERNODELIST_UPDATE_SECONDS : MASTERNODELIST_UPDATE_SECONDS * 10; + int64_t nSecondsToWait = nTimeUpdatedDIP3 - GetTime() + nMnListUpdateSeconds;
141-147: Avoid briefly showing negative countdown.Clamp the displayed value to 0 to prevent negative numbers flashing in the label.
- ui->countLabelDIP3->setText(tr("Please wait…") + " " + QString::number(nSecondsToWait)); + const auto displayWait = nSecondsToWait > 0 ? nSecondsToWait : 0; + ui->countLabelDIP3->setText(tr("Please wait…") + " " + QString::number(displayWait));src/qt/coincontroldialog.cpp (1)
16-16: Good: proper dependency for GUIUtil::FontWeight.One question: this file still persists UI prefs via QSettings (Lines 135–139, 153–157). If the PR goal is migrating settings to settings.json where feasible, consider whether these should be moved too, or explicitly documented as remaining UI-local prefs.
src/qt/appearancewidget.h (1)
12-12: Appropriate to include in a header due to member types/default initializersprevFontFamily and weight members (with default initializers calling GUIUtil getters) require guiutil_font.h in this header. This is the right place to include it.
If compile time becomes a concern later, consider moving function-based default initializers (e.g., GUIUtil::getFontFamily()) into the .cpp constructor to reduce header coupling. Not required for this backport.
src/qt/rpcconsole.cpp (1)
68-71: Consider migrating console font size setting away from QSettingsGiven the PR’s “migrate some QSettings to settings.json,” consider moving consoleFontSize to the new persistent settings path (via Node/ArgsManager helpers) for consistency and headless configurability. Low priority; can be follow-up.
src/qt/appearancewidget.cpp (1)
31-33: Populate font families from GUIUtil::AVAILABLE_FONTS — good step; add fallback and verify localization
- Good: dynamic population with “selectable” filter.
- Risks:
- If the stored OptionsModel FontFamily is not selectable on the current platform, currentIndex may be -1 after mapper->toFirst(). Add a graceful fallback to the first available entry and apply it.
- Ensure family_str strings are already localized in GUIUtil; otherwise, user-facing labels like “System default” should be translated.
Suggested fallback (in setModel(), after mapper->toFirst()):
@@ void AppearanceWidget::setModel(OptionsModel* _model) - mapper->toFirst(); + mapper->toFirst(); + // Fallback if stored value is not present (e.g., font now non-selectable) + if (ui->fontFamily->currentIndex() < 0 && ui->fontFamily->count() > 0) { + ui->fontFamily->setCurrentIndex(0); + updateFontFamily(0); + }Also verify that guiutil_font.h is included indirectly via appearancewidget.h. If not, include it here:
+#include <qt/guiutil_font.h>src/qt/bitcoin.h (1)
49-49: Call sites already handle [[nodiscard]] return; add a header comment for failure reasons and optionally inline‐initialize optionsModel.--- a/src/qt/bitcoin.h +++ b/src/qt/bitcoin.h @@ - /// Create options model - [[nodiscard]] bool createOptionsModel(bool resetSettings); + /// Create options model; returns false on Init() failure (e.g. settings load error) + [[nodiscard]] bool createOptionsModel(bool resetSettings); @@ - OptionsModel *optionsModel; + OptionsModel *optionsModel{nullptr};src/qt/test/addressbooktests.cpp (1)
122-125: Prefer QVERIFY2 to surface Init() failure reason.Improves debuggability without changing behavior.
- bilingual_str error; - QVERIFY(optionsModel.Init(error)); + bilingual_str error; + QVERIFY2(optionsModel.Init(error), error.translated.c_str());src/qt/test/wallettests.cpp (1)
155-158: Use QVERIFY2 with message for OptionsModel::Init.Shows the translated error on failure.
- bilingual_str error; - QVERIFY(optionsModel.Init(error)); + bilingual_str error; + QVERIFY2(optionsModel.Init(error), error.translated.c_str());src/util/settings.cpp (1)
180-182: Nonpersistent filter logic looks correct; consider parity helpers and testsSkipping COMMAND_LINE and FORCED when
ignore_nonpersistentis true aligns with “persistent-only” reads. Two follow-ups:
- Consider a parallel API for lists (e.g.,
GetPersistentSettingsList) if the GUI needs array semantics without CLI/forced influence.- Add tests covering forced and negated values when
ignore_nonpersistent=trueto prevent regressions.I can draft tests mirroring existing settings_tests with forced/CLI sources. Want a patch?
src/node/interfaces.cpp (1)
479-486: Consider error handling during resetSettings
resetSettings()ignores the return ofWriteSettingsFile(...). A failed backup or rewrite would be silent. Capture the boolean and log or surface an error to the caller/UI.Apply:
void resetSettings() override { - gArgs.WriteSettingsFile(/*errors=*/nullptr, /*backup=*/true); + if (!gArgs.WriteSettingsFile(/*errors=*/nullptr, /*backup=*/true)) { + LogPrintf("resetSettings: failed to write backup settings file\n"); + } gArgs.LockSettings([&](util::Settings& settings) { settings.rw_settings.clear(); }); - gArgs.WriteSettingsFile(); + if (!gArgs.WriteSettingsFile()) { + LogPrintf("resetSettings: failed to write cleared settings file\n"); + } }src/qt/optionsdialog.cpp (1)
376-376: Map the radio button’s checked property explicitlyRelying on the widget’s user property works, but making the mapping explicit avoids surprises if UI changes later.
Apply:
- mapper->addMapping(ui->embeddedFont_radioButton, OptionsModel::UseEmbeddedMonospacedFont); + mapper->addMapping(ui->embeddedFont_radioButton, OptionsModel::UseEmbeddedMonospacedFont, "checked");src/interfaces/node.h (1)
197-214: Clarify override semantics (CLI vs forced) and return contractsDocs say “overridden by the command line,” but implementation likely should also consider
forced_settings. Either:
- Update
isSettingIgnoreddocs to include “or forced settings,” or- Intentionally exclude forced and state that explicitly.
Also,
updateRwSettinghere isvoidwhile an existing overload returnsbool. Consider harmonizing the contract in a follow-up (even if just by documenting error handling expectations).src/test/settings_tests.cpp (1)
145-148: NullOverride assertions remain valid with new flag.Using ignore_nonpersistent=false ensures forced null overrides are tested. Consider adding a complementary test with ignore_nonpersistent=true to lock in persistent-only semantics.
I can add a small test exercising GetPersistentSetting/ignore_nonpersistent=true if helpful.
src/util/system.h (3)
179-182: Avoid leaking helpers into global namespace.Consider placing SettingToString/SettingToInt/SettingToBool into a namespace (e.g., util) to minimize global symbol surface.
-std::string SettingToString(const util::SettingsValue&, const std::string&); -int64_t SettingToInt(const util::SettingsValue&, int64_t); -bool SettingToBool(const util::SettingsValue&, bool); +namespace util { +std::string SettingToString(const util::SettingsValue&, const std::string&); +int64_t SettingToInt(const util::SettingsValue&, int64_t); +bool SettingToBool(const util::SettingsValue&, bool); +} // namespace utilNote: adjust call sites accordingly.
477-477: Mark GetSettingsPath as nodiscard.This is typically used for control flow; marking nodiscard avoids accidental ignoring of failure.
- bool GetSettingsPath(fs::path* filepath = nullptr, bool temp = false, bool backup = false) const; + [[nodiscard]] bool GetSettingsPath(fs::path* filepath = nullptr, bool temp = false, bool backup = false) const;
490-495: New persistent accessor: verify thread-safety note.The method is const and acquires cs_args in the impl—good. Consider noting in the comment that it acquires the internal lock.
src/qt/test/optiontests.cpp (1)
31-62: Clean up artifacts after migration test.To avoid influencing subsequent tests, consider removing settings.json (and clearing QSettings) at the end of migrateSettings().
src/util/system.cpp (2)
563-577: Backup path suffix ordering: confirm intent.GetSettingsPath builds "settings.json.bak.tmp" for temp backups. If tooling expects ".tmp" to be the final suffix, consider swapping the order:
- if (backup) { - settings += ".bak"; - } - if (filepath) { - *filepath = fsbridge::AbsPathJoin(GetDataDirNet(), temp ? settings + ".tmp" : settings); - } + if (filepath) { + fs::path base = settings; + if (temp) base += ".tmp"; + if (backup) base += ".bak"; + *filepath = fsbridge::AbsPathJoin(GetDataDirNet(), base); + }Please verify no consumers depend on the current ordering.
632-638: Persistent-only accessor correctly ignores nonpersistent layers.Lock + util::GetSetting(..., /ignore_nonpersistent=/true) is correct. Consider a unit test exercising this path.
Happy to add a small test demonstrating forced/CLI values being ignored.
src/qt/bitcoin.cpp (1)
509-512: Avoid duplicating the list of valid font families.Consider deriving the “Possible values” string from the enum/registry used by GUIUtil::fontFamilyFromString to keep help text in sync.
src/qt/guiutil_font.h (2)
43-48: Unify macOS guard with .cpp for consistency.
.cpp uses Q_OS_MAC; header uses Q_OS_MACOS. Align to one macro to avoid confusion and divergent defaults.-#ifdef Q_OS_MACOS +#ifdef Q_OS_MAC QFont::ExtraLight #else QFont::Light -#endif // Q_OS_MAC +#endif // Q_OS_MAC
85-89: Typo in comment.
“appliciation” → “application”.-/** Load dash specific appliciation fonts */ +/** Load Dash-specific application fonts */src/qt/optionsmodel.cpp (2)
334-339: Store boolean default, not string, for UseEmbeddedMonospacedFont.
Avoid type drift in QSettings.- if (!settings.contains("UseEmbeddedMonospacedFont")) { - settings.setValue("UseEmbeddedMonospacedFont", "true"); - } + if (!settings.contains("UseEmbeddedMonospacedFont")) { + settings.setValue("UseEmbeddedMonospacedFont", true); + }
755-760: Signal name may be misleading.
Changing sessions emits coinJoinRoundsChanged(). Consider a dedicated sessions-changed signal for clarity.
📜 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 (52)
src/Makefile.qt.include(2 hunks)src/init.cpp(0 hunks)src/interfaces/node.h(2 hunks)src/node/interfaces.cpp(1 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(5 hunks)src/qt/bitcoin.h(1 hunks)src/qt/bitcoingui.cpp(2 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/forms/optionsdialog.ui(2 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(9 hunks)src/qt/optionsmodel.h(8 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/addressbooktests.cpp(1 hunks)src/qt/test/apptests.cpp(2 hunks)src/qt/test/optiontests.cpp(3 hunks)src/qt/test/optiontests.h(1 hunks)src/qt/test/test_main.cpp(1 hunks)src/qt/test/wallettests.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)src/test/settings_tests.cpp(3 hunks)src/util/settings.cpp(2 hunks)src/util/settings.h(1 hunks)src/util/system.cpp(6 hunks)src/util/system.h(2 hunks)
💤 Files with no reviewable changes (3)
- src/init.cpp
- src/qt/guiutil.h
- src/qt/guiutil.cpp
🧰 Additional context used
🧠 Learnings (3)
📓 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-08-08T07:01:47.332Z
Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/qt/test/wallettests.cpp
📚 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.cppsrc/qt/test/optiontests.h
🪛 GitHub Actions: Clang Diff Format Check
src/qt/masternodelist.cpp
[error] 5-13: Clang format differences found in src/qt/masternodelist.cpp. The diff shows an include order change (added '#include <coins.h>' before 'evo/deterministicmns.h' and removed the previous '#include <coins.h>'). The check failed with exit code 1. Run 'clang-format-diff.py -p1' (or 'clang-format') to fix.
⏰ 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-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (55)
src/qt/forms/optionsdialog.ui (1)
1215-1215: String change: confirm translations are updatedThe bottom info label text changed; please run lupdate / refresh .ts files so translators see the new string.
src/util/settings.h (1)
58-71: All GetSetting call sites and tests updated to 6-arg signature Verified no lingering 5-arg calls remain.src/qt/utilitydialog.cpp (1)
15-15: Include aligns with font subsystem migrationHeader inclusion matches the use of GUIUtil::updateFonts and the new split of font utilities. LGTM.
src/qt/transactiondescdialog.cpp (1)
9-9: Include aligns with font subsystem migrationConsistent with calling GUIUtil::updateFonts(). LGTM.
src/qt/masternodelist.cpp (1)
12-12: Header include looks correct.Including qt/guiutil_font.h is required for GUIUtil::setFont/updateFonts. No issues.
src/qt/sendcoinsdialog.cpp (1)
18-18: Header include is appropriate.qt/guiutil_font.h is needed for font helpers used in this file. Looks good.
src/qt/openuridialog.cpp (1)
10-10: Include addition is correct.Required for GUIUtil::updateFonts usage. No further changes needed.
src/qt/addresstablemodel.cpp (1)
9-9: Include addition aligns with FontRole usage.Bringing in qt/guiutil_font.h matches GUIUtil::getFontNormal() usage. All good.
src/qt/splashscreen.cpp (1)
19-19: Include addition is correct.Needed for GUIUtil::getFontNormal/getFontBold; no issues.
src/qt/addressbookpage.cpp (1)
17-17: Include for font utilities is correct.Matches the migration of font APIs out of guiutil.h; aligns with GUIUtil::updateFonts() usage later. No issues.
src/qt/qrimagewidget.cpp (1)
8-8: Header addition aligns with font API split.Required for getFontNormal/calculateIdealFontSize. Looks good.
src/qt/sendcoinsentry.cpp (1)
16-16: Review OK; include needed for setFont(FontWeight)
<qt/guiutil_font.h>is required forGUIUtil::setFont(FontWeight)— LGTM.labellLabelis defined insrc/qt/forms/sendcoinsentry.ui(line 118); name matches the code, no typo.src/qt/modaloverlay.cpp (1)
10-10: Correct header include for font utilities.Consistent with GUIUtil::setFont usage below. No further action.
src/qt/signverifymessagedialog.cpp (1)
11-11: Approve: guiutil_font header integration verified
Makefile.qt.include lists guiutil_font.h/.cpp and all translation units using GUIUtil::FontWeight include<qt/guiutil_font.h>.src/qt/receivecoinsdialog.cpp (1)
10-10: Header addition aligns with centralized font utilitiesIncluding qt/guiutil_font.h here is consistent with using GUIUtil::setFont/updateFonts. No issues.
src/qt/qrdialog.cpp (1)
11-11: Good: pulls in FontWeight after font helpers moved out of guiutil.hKeeps this TU compiling with the new font subsystem. Looks good.
src/qt/governancelist.cpp (1)
20-20: Correct dependency on guiutil_font.hNeeded for the GUIUtil::setFont calls used in this widget. Change is safe and scoped.
src/qt/askpassphrasedialog.cpp (1)
15-15: Include for font utilities looks correctUnblocks usage of GUIUtil::FontWeight in this TU. No further action.
src/qt/rpcconsole.cpp (1)
21-21: Add guiutil_font include — OKMatches usage of GUIUtil::setFont/updateFonts and font weights below.
src/qt/trafficgraphwidget.cpp (1)
9-9: Include addition is fineAligns this TU with GUI font helpers; no behavior change.
src/qt/walletview.cpp (1)
15-15: Include addition is fineEnables explicit font weight usage in this TU. No issues.
src/qt/appearancewidget.cpp (1)
31-33: AVAILABLE_FONTS header inclusion verified
Declaration exists in src/qt/guiutil_font.h (line 26) and appearancewidget.h includes it (line 11) — no further changes needed.src/qt/receiverequestdialog.cpp (1)
10-10: Include for moved font utilities looks correct.This aligns with updateFonts() migration to guiutil_font.*. No further action.
src/qt/proposalwizard.cpp (1)
16-16: Correct header added for GUIUtil::setFont and FontWeight.Header placement is fine and resolves the new font API usage.
src/qt/overviewpage.h (1)
74-74: No issues—slot implemented and signal connected.OverviewPage::setMonospacedFont(bool) is defined in overviewpage.cpp (line 379) and wired to OptionsModel::useEmbeddedMonospacedFontChanged in overviewpage.cpp (line 305); no further action required.
src/Makefile.qt.include (1)
140-140: Confirmed no Q_OBJECT in guiutil_font.h; no moc entry needed.src/qt/test/apptests.cpp (1)
11-11: LGTM on include and return-value assert.The new guiutil_font include and the updated QVERIFY for createOptionsModel(bool) look correct and align with the API change.
Also applies to: 79-79
src/qt/bitcoingui.cpp (1)
13-13: LGTM: guiutil_font include.Consistent with the new font utilities usage elsewhere.
src/qt/test/optiontests.h (3)
9-11: LGTM: adds required headers for settings/JSON use.
18-18: Constructor change acknowledged.Node-injecting ctor is appropriate for node-backed settings tests.
21-26: Ensure settings backup/restore around tests.Please verify that m_previous_settings is populated before mutation (e.g., in init()) and restored afterward (e.g., dedicated cleanup slot or destructor) to avoid cross-test contamination.
Also applies to: 29-29
src/util/settings.cpp (1)
144-146: All util::GetSetting call sites updated and docs accurate
Verified the three invocations in system.cpp (lines 635–637, 1113–1115, 1183–1185) now pass bothignore_nonpersistentandget_chain_namein the correct order, and the header comment already documents thatignore_nonpersistentfilters out forced and command-line values.src/node/interfaces.cpp (1)
1127-1154: All boolean-returning calls to updateRwSetting use the Chain::updateRwSetting 3-arg overload (explicit or via default write flag); the 2-arg Node::updateRwSetting is only used by Qt code where its void return is correctly ignored.src/qt/overviewpage.cpp (2)
379-395: LGTM: focused, minimal font applicationUsing
GUIUtil::setFont(..., RobotoMono|DefaultMonospace, Bold)andGUIUtil::updateFonts()is consistent with the new font subsystem and limited in scope to balance labels.
305-307: [awaiting_run_scripts]src/qt/optionsdialog.cpp (1)
205-218: LGTM: clear font previews and sane defaultPreview labels use the actual families via
QFontInfo, and defaulting to system-monospaced is reasonable until the model mapping loads. No functional issues spotted.src/interfaces/node.h (1)
8-16: Header include adjustments look fineAdding
util/settings.hforutil::SettingsValueis correct, and other includes are appropriately ordered.src/test/settings_tests.cpp (2)
109-116: API update applied correctly in helper.Passing ignore_nonpersistent=false preserves previous behavior. LGTM.
228-229: Merge test call site aligns with new GetSetting signature.Explicit comments for the two new booleans are helpful. LGTM.
src/util/system.h (1)
485-489: Docs sync with backup behavior.Comment clarifies backup path behavior. LGTM.
src/qt/test/optiontests.cpp (3)
24-29: Fixture reset is sound.Restoring m_previous_settings and clearing the path cache prevents cross-test contamination. LGTM.
74-86: Regression guard looks good.Constructing OptionsModel with Node and calling Init(error) directly covers the reported regression. LGTM.
108-110: parametersInteraction now validates via OptionsModel::Init.Good shift to Node-aware initialization. LGTM.
src/util/system.cpp (4)
612-631: WriteSettingsFile backup plumbing is correct.Using the same backup flag for both tmp and final paths is consistent. LGTM.
644-675: Centralized conversions reduce duplication.Refactoring GetArg/GetIntArg/GetBoolArg to use SettingTo* helpers improves consistency. LGTM.
1109-1118: Chain selection call updated with new GetSetting flags.Explicit get_chain_name=true and ignore_nonpersistent=false maintain existing semantics. LGTM.
1184-1186: ArgsManager::GetSetting updated with new flag.Explicit ignore_nonpersistent=false preserves prior behavior. LGTM.
src/qt/bitcoin.cpp (3)
350-353: Prune API change accounted for.Dropping the extra boolean matches the new signature. LGTM.
691-700: Startup flow improvement: create splash and node earlier; gate on options init.Great for UX and error handling. LGTM.
704-705: Default font family fallback is consistent.Using GUIUtil::g_font_defaults.family for default arg is correct. LGTM.
src/qt/optionsmodel.h (3)
103-105: Public getOption/setOption and embedded-mono getter are a welcome simplification.
API surface reads clean and matches the data()/setData() rework.Also applies to: 114-115
45-47: All OptionsModel instantiations updated and no setNode usage remains
Verified that OptionsModel is only constructed via the newOptionsModel(interfaces::Node&, …)constructor (e.g. inbitcoin.cpp:273) and no.setNode(calls remain.
123-124: Confirm downstream usage update
Grep shows only a single call site in bitcoin.cpp using the new one-parameter signature; no force parameter remains.src/qt/guiutil_font.cpp (1)
367-381: Update loop is careful and two-phase; LGTM.
The cleanup of dead QPointers and deferred apply avoids inheritance glitches.Also applies to: 383-392, 439-447, 449-463
src/qt/optionsmodel.cpp (1)
99-115: Back-compat workaround with 22.x numerics: solid approach.
Writing specific numeric settings as strings to dodge UniValue exceptions is pragmatic and isolated.
| bool isSettingIgnored(const std::string& name) override | ||
| { | ||
| bool ignored = false; | ||
| gArgs.LockSettings([&](util::Settings& settings) { | ||
| if (auto* options = util::FindKey(settings.command_line_options, name)) { | ||
| ignored = !options->empty(); | ||
| } | ||
| }); | ||
| return ignored; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also treat forced settings as “ignored” overrides
isSettingIgnored only checks command_line_options. If a setting is overridden via forced_settings, the GUI will not flag it as ignored. Either update the docstring in node.h or include forced settings here for consistency with ignore_nonpersistent.
Apply:
bool isSettingIgnored(const std::string& name) override
{
- bool ignored = false;
- gArgs.LockSettings([&](util::Settings& settings) {
- if (auto* options = util::FindKey(settings.command_line_options, name)) {
- ignored = !options->empty();
- }
- });
- return ignored;
+ bool ignored = false;
+ gArgs.LockSettings([&](const util::Settings& settings) {
+ if (const auto* v = util::FindKey(settings.command_line_options, name)) {
+ ignored = !v->empty();
+ }
+ if (!ignored) {
+ if (const auto* f = util::FindKey(settings.forced_settings, name)) {
+ ignored = !f->empty();
+ }
+ }
+ });
+ return ignored;
}📝 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.
| bool isSettingIgnored(const std::string& name) override | |
| { | |
| bool ignored = false; | |
| gArgs.LockSettings([&](util::Settings& settings) { | |
| if (auto* options = util::FindKey(settings.command_line_options, name)) { | |
| ignored = !options->empty(); | |
| } | |
| }); | |
| return ignored; | |
| } | |
| bool isSettingIgnored(const std::string& name) override | |
| { | |
| bool ignored = false; | |
| gArgs.LockSettings([&](const util::Settings& settings) { | |
| if (const auto* v = util::FindKey(settings.command_line_options, name)) { | |
| ignored = !v->empty(); | |
| } | |
| if (!ignored) { | |
| if (const auto* f = util::FindKey(settings.forced_settings, name)) { | |
| ignored = !f->empty(); | |
| } | |
| } | |
| }); | |
| return ignored; | |
| } |
🤖 Prompt for AI Agents
In src/node/interfaces.cpp around lines 447 to 456, isSettingIgnored currently
only checks command_line_options and therefore misses settings present in
forced_settings; update the function to also check settings.forced_settings
(using util::FindKey or equivalent) and mark the setting as ignored if a
forced_settings entry exists or is non-empty, mirroring ignore_nonpersistent
behavior; keep the gArgs.LockSettings scope and combine the checks so that
ignored becomes true if either command_line_options or forced_settings indicates
an override.
| void updateRwSetting(const std::string& name, const util::SettingsValue& value) override | ||
| { | ||
| gArgs.LockSettings([&](util::Settings& settings) { | ||
| if (value.isNull()) { | ||
| settings.rw_settings.erase(name); | ||
| } else { | ||
| settings.rw_settings[name] = value; | ||
| } | ||
| }); | ||
| gArgs.WriteSettingsFile(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify update path and handle write failures
This void updateRwSetting duplicates the logic of the existing bool updateRwSetting(name, value, write) and ignores the write result. Delegate to the boolean overload to centralize behavior and optionally log failures.
Apply:
void updateRwSetting(const std::string& name, const util::SettingsValue& value) override
{
- gArgs.LockSettings([&](util::Settings& settings) {
- if (value.isNull()) {
- settings.rw_settings.erase(name);
- } else {
- settings.rw_settings[name] = value;
- }
- });
- gArgs.WriteSettingsFile();
+ (void)updateRwSetting(name, value, /*write=*/true);
}🤖 Prompt for AI Agents
In src/node/interfaces.cpp around lines 458 to 468, this void updateRwSetting
duplicates logic of the existing bool updateRwSetting(name, value, write) and
ignores whether the write succeeded; replace the implementation to call the
boolean overload (e.g. updateRwSetting(name, value, /*write*/ true)) and if it
returns false, emit a failure log (use the existing logging facility in this
file) so write failures are not silently ignored, thereby centralizing behavior
and preserving the original semantics.
| bool BitcoinApplication::createOptionsModel(bool resetSettings) | ||
| { | ||
| optionsModel = new OptionsModel(this, resetSettings); | ||
| optionsModel = new OptionsModel(node(), this); | ||
| if (resetSettings) { | ||
| optionsModel->Reset(); | ||
| } | ||
| bilingual_str error; | ||
| if (!optionsModel->Init(error)) { | ||
| fs::path settings_path; | ||
| if (gArgs.GetSettingsPath(&settings_path)) { | ||
| error += Untranslated("\n"); | ||
| std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path))); | ||
| error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path); | ||
| error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString(); | ||
| } | ||
| InitError(error); | ||
| QMessageBox::critical(nullptr, PACKAGE_NAME, QString::fromStdString(error.translated)); | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Compile error: use std::quoted (not fs::quoted) and include .
fs has no quoted; std::quoted is in . Fix as below.
@@
-#include <memory>
+#include <memory>
+#include <iomanip> // for std::quoted
@@
- if (gArgs.GetSettingsPath(&settings_path)) {
+ if (gArgs.GetSettingsPath(&settings_path)) {
error += Untranslated("\n");
- std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path)));
+ std::string quoted_path = strprintf("%s", std::quoted(fs::PathToString(settings_path)));
error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path);
error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString();
}📝 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.
| bool BitcoinApplication::createOptionsModel(bool resetSettings) | |
| { | |
| optionsModel = new OptionsModel(this, resetSettings); | |
| optionsModel = new OptionsModel(node(), this); | |
| if (resetSettings) { | |
| optionsModel->Reset(); | |
| } | |
| bilingual_str error; | |
| if (!optionsModel->Init(error)) { | |
| fs::path settings_path; | |
| if (gArgs.GetSettingsPath(&settings_path)) { | |
| error += Untranslated("\n"); | |
| std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path))); | |
| error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path); | |
| error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString(); | |
| } | |
| InitError(error); | |
| QMessageBox::critical(nullptr, PACKAGE_NAME, QString::fromStdString(error.translated)); | |
| return false; | |
| } | |
| return true; | |
| } | |
| #include <memory> | |
| #include <iomanip> // for std::quoted | |
| bool BitcoinApplication::createOptionsModel(bool resetSettings) | |
| { | |
| optionsModel = new OptionsModel(node(), this); | |
| if (resetSettings) { | |
| optionsModel->Reset(); | |
| } | |
| bilingual_str error; | |
| if (!optionsModel->Init(error)) { | |
| fs::path settings_path; | |
| if (gArgs.GetSettingsPath(&settings_path)) { | |
| error += Untranslated("\n"); | |
| std::string quoted_path = strprintf("%s", std::quoted(fs::PathToString(settings_path))); | |
| error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path); | |
| error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString(); | |
| } | |
| InitError(error); | |
| QMessageBox::critical(nullptr, PACKAGE_NAME, QString::fromStdString(error.translated)); | |
| return false; | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In src/qt/bitcoin.cpp around lines 271 to 291, the code incorrectly calls
fs::quoted (which doesn't exist) and doesn't include the header for std::quoted;
change fs::quoted to std::quoted and add #include <iomanip> to the file (or a
common header included by this translation unit) so std::quoted is available;
ensure the call uses std::quoted(std::string) (i.e., keep the strprintf
wrapping) and fully-qualify as std::quoted to avoid relying on using directives.
| m_mask_values_action->setChecked(_clientModel->getOptionsModel()->getOption(OptionsModel::OptionID::MaskValues).toBool()); | ||
| } else { |
There was a problem hiding this comment.
Null-deref risk if OptionsModel is null.
_setClientModel() unconditionally dereferences _clientModel->getOptionsModel() to set m_mask_values_action. This can crash when options model is not yet available. Guard it.
- m_mask_values_action->setChecked(_clientModel->getOptionsModel()->getOption(OptionsModel::OptionID::MaskValues).toBool());
+ if (OptionsModel* opt = _clientModel->getOptionsModel()) {
+ m_mask_values_action->setChecked(opt->getOption(OptionsModel::OptionID::MaskValues).toBool());
+ } else {
+ m_mask_values_action->setChecked(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.
| m_mask_values_action->setChecked(_clientModel->getOptionsModel()->getOption(OptionsModel::OptionID::MaskValues).toBool()); | |
| } else { | |
| if (OptionsModel* opt = _clientModel->getOptionsModel()) { | |
| m_mask_values_action->setChecked(opt->getOption(OptionsModel::OptionID::MaskValues).toBool()); | |
| } else { | |
| m_mask_values_action->setChecked(false); | |
| } | |
| } else { |
🤖 Prompt for AI Agents
In src/qt/bitcoingui.cpp around lines 893-894, the code unconditionally calls
_clientModel->getOptionsModel()->getOption(...), risking a null-deref if
getOptionsModel() returns nullptr; change the logic to first check that
_clientModel and _clientModel->getOptionsModel() are non-null before calling
getOption, and only call m_mask_values_action->setChecked(...) when the options
model exists (otherwise set a safe default or leave the action unchecked);
ensure any subsequent logic that assumes the options model exists is similarly
guarded or deferred until the model is available.
| auto& vecSupported = mapSupportedWeights[fontFamily]; | ||
| auto it = std::find(vecSupported.begin(), vecSupported.end(),normalWeight); | ||
| if (++it != vecSupported.end()) { | ||
| boldWeight = *it; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: wrong map index when computing default bold weight for non-current family.
Uses global fontFamily instead of the function parameter, producing incorrect defaults for families other than the currently selected one.
Apply:
- auto& vecSupported = mapSupportedWeights[fontFamily];
+ auto& vecSupported = mapSupportedWeights[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.
| auto& vecSupported = mapSupportedWeights[fontFamily]; | |
| auto it = std::find(vecSupported.begin(), vecSupported.end(),normalWeight); | |
| if (++it != vecSupported.end()) { | |
| boldWeight = *it; | |
| } | |
| } | |
| auto& vecSupported = mapSupportedWeights[family]; | |
| auto it = std::find(vecSupported.begin(), vecSupported.end(),normalWeight); | |
| if (++it != vecSupported.end()) { | |
| boldWeight = *it; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/qt/guiutil_font.cpp around lines 286 to 291, the code uses the global
fontFamily when indexing mapSupportedWeights and increments the iterator before
checking it, which can produce wrong results or undefined behavior for
non-current families; change the map lookup to use the function parameter (e.g.
family) instead of fontFamily, ensure you check that the find() result is not
vecSupported.end() before incrementing the iterator, then increment and check
again before assigning boldWeight.
| static ProxySetting ParseProxyString(const QString& proxy) | ||
| { | ||
| static const ProxySetting default_val = {false, DEFAULT_GUI_PROXY_HOST, QString("%1").arg(DEFAULT_GUI_PROXY_PORT)}; | ||
| // Handle the case that the setting is not set at all | ||
| if (!settings.contains(name)) { | ||
| if (proxy.isEmpty()) { | ||
| return default_val; | ||
| } | ||
| // contains IP at index 0 and port at index 1 | ||
| QStringList ip_port = GUIUtil::SplitSkipEmptyParts(settings.value(name).toString(), ":"); | ||
| QStringList ip_port = GUIUtil::SplitSkipEmptyParts(proxy, ":"); | ||
| if (ip_port.size() == 2) { | ||
| return {true, ip_port.at(0), ip_port.at(1)}; | ||
| } else { // Invalid: return default | ||
| return default_val; | ||
| } | ||
| } |
There was a problem hiding this comment.
Proxy parsing is not IPv6-safe; also emit bracketed host on write.
Current split on ":" breaks IPv6 and ambiguous hosts. Parse by bracket/last-colon; bracket on serialization when needed.
-static ProxySetting ParseProxyString(const QString& proxy)
+static ProxySetting ParseProxyString(const QString& proxy)
{
- static const ProxySetting default_val = {false, DEFAULT_GUI_PROXY_HOST, QString("%1").arg(DEFAULT_GUI_PROXY_PORT)};
- // Handle the case that the setting is not set at all
- if (proxy.isEmpty()) {
+ static const ProxySetting default_val = {false, DEFAULT_GUI_PROXY_HOST, QString::number(DEFAULT_GUI_PROXY_PORT)};
+ const QString s = proxy.trimmed();
+ if (s.isEmpty()) {
return default_val;
}
- // contains IP at index 0 and port at index 1
- QStringList ip_port = GUIUtil::SplitSkipEmptyParts(proxy, ":");
- if (ip_port.size() == 2) {
- return {true, ip_port.at(0), ip_port.at(1)};
- } else { // Invalid: return default
- return default_val;
- }
+ QString ip, port;
+ if (s.startsWith('[')) {
+ const int rb = s.indexOf(']');
+ if (rb <= 0) return default_val;
+ ip = s.mid(1, rb - 1);
+ const int colon = s.indexOf(':', rb);
+ port = colon >= 0 ? s.mid(colon + 1) : QString();
+ } else {
+ const int colon = s.lastIndexOf(':');
+ if (colon <= 0) return default_val;
+ ip = s.left(colon);
+ port = s.mid(colon + 1);
+ }
+ if (ip.isEmpty() || port.isEmpty()) return default_val;
+ return {true, ip, port};
}
static ProxySetting ParseProxyString(const std::string& proxy)
{
return ParseProxyString(QString::fromStdString(proxy));
}
-static std::string ProxyString(bool is_set, QString ip, QString port)
+static std::string ProxyString(bool is_set, QString ip, QString port)
{
- return is_set ? QString(ip + ":" + port).toStdString() : "";
+ if (!is_set) return "";
+ const bool needs_brackets = ip.contains(':') && !ip.startsWith('[');
+ const QString host = needs_brackets ? QString("[" + ip + "]") : ip;
+ return QString(host + ":" + port).toStdString();
}Also applies to: 410-414, 415-418
🤖 Prompt for AI Agents
In src/qt/optionsmodel.cpp around lines 394-408 (and also apply same fix to
410-414 and 415-418): the current ParseProxyString splits on ":" which breaks
IPv6 and ambiguous host:port formats; instead, detect if the host is bracketed
(starts with '[' and contains ']') and extract the host between brackets and the
port after the closing ']', otherwise split on the last ':' to separate host and
port; when serializing the proxy setting, emit the host in brackets if it
contains ':' (i.e. is IPv6) so the saved string is unambiguous; validate that
port is present and fall back to the default proxy when parsing fails.
src/qt/optionsmodel.cpp
Outdated
| case CoinControlFeatures: | ||
| fCoinControlFeatures = value.toBool(); | ||
| settings.setValue("fCoinControlFeatures", fCoinControlFeatures); | ||
| Q_EMIT coinControlFeaturesChanged(fCoinControlFeatures); | ||
| break; | ||
| case KeepChangeAddress: | ||
| fKeepChangeAddress = value.toBool(); | ||
| settings.setValue("fKeepChangeAddress", fKeepChangeAddress); | ||
| Q_EMIT keepChangeAddressChanged(fKeepChangeAddress); | ||
| break; | ||
| case Prune: | ||
| if (changed()) { | ||
| if (suffix.empty() && !value.toBool()) setOption(option, true, "-prev"); | ||
| update(PruneSetting(value.toBool(), getOption(PruneSize).toInt())); | ||
| if (suffix.empty() && value.toBool()) UpdateRwSetting(node(), option, "-prev", {}); | ||
| if (suffix.empty()) setRestartRequired(true); | ||
| } | ||
| break; | ||
| case PruneSize: | ||
| if (changed()) { | ||
| if (suffix.empty() && !getOption(Prune).toBool()) { | ||
| setOption(option, value, "-prev"); | ||
| } else { | ||
| update(PruneSetting(true, ParsePruneSizeGB(value))); | ||
| } | ||
| break; | ||
| if (suffix.empty() && getOption(Prune).toBool()) setRestartRequired(true); | ||
| } | ||
| break; | ||
| #endif // ENABLE_WALLET | ||
| case DatabaseCache: | ||
| if (settings.value("nDatabaseCache") != value) { | ||
| settings.setValue("nDatabaseCache", value); | ||
| setRestartRequired(true); | ||
| } | ||
| break; | ||
| case ThreadsScriptVerif: | ||
| if (settings.value("nThreadsScriptVerif") != value) { | ||
| settings.setValue("nThreadsScriptVerif", value); | ||
| setRestartRequired(true); | ||
| } | ||
| break; | ||
| case Listen: | ||
| if (settings.value("fListen") != value) { | ||
| settings.setValue("fListen", value); | ||
| setRestartRequired(true); | ||
| } | ||
| break; | ||
| case Server: | ||
| if (settings.value("server") != value) { | ||
| settings.setValue("server", value); | ||
| setRestartRequired(true); | ||
| } | ||
| break; | ||
| default: | ||
| break; | ||
| case DatabaseCache: |
There was a problem hiding this comment.
Prune options are incorrectly gated by ENABLE_WALLET.
Pruning is core-wide, not wallet-specific. With wallet disabled, setOption() will ignore Prune/PruneSize.
Move Prune/PruneSize cases outside the ENABLE_WALLET block:
#ifdef ENABLE_WALLET
case CoinControlFeatures:
fCoinControlFeatures = value.toBool();
settings.setValue("fCoinControlFeatures", fCoinControlFeatures);
Q_EMIT coinControlFeaturesChanged(fCoinControlFeatures);
break;
case KeepChangeAddress:
fKeepChangeAddress = value.toBool();
settings.setValue("fKeepChangeAddress", fKeepChangeAddress);
Q_EMIT keepChangeAddressChanged(fKeepChangeAddress);
break;
- case Prune:
+ #endif // ENABLE_WALLET
+ case Prune:
if (changed()) {
if (suffix.empty() && !value.toBool()) setOption(option, true, "-prev");
update(PruneSetting(value.toBool(), getOption(PruneSize).toInt()));
if (suffix.empty() && value.toBool()) UpdateRwSetting(node(), option, "-prev", {});
if (suffix.empty()) setRestartRequired(true);
}
break;
- case PruneSize:
+ case PruneSize:
if (changed()) {
if (suffix.empty() && !getOption(Prune).toBool()) {
setOption(option, value, "-prev");
} else {
update(PruneSetting(true, ParsePruneSizeGB(value)));
}
if (suffix.empty() && getOption(Prune).toBool()) setRestartRequired(true);
}
break;
-#endif // ENABLE_WALLET📝 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.
| case CoinControlFeatures: | |
| fCoinControlFeatures = value.toBool(); | |
| settings.setValue("fCoinControlFeatures", fCoinControlFeatures); | |
| Q_EMIT coinControlFeaturesChanged(fCoinControlFeatures); | |
| break; | |
| case KeepChangeAddress: | |
| fKeepChangeAddress = value.toBool(); | |
| settings.setValue("fKeepChangeAddress", fKeepChangeAddress); | |
| Q_EMIT keepChangeAddressChanged(fKeepChangeAddress); | |
| break; | |
| case Prune: | |
| if (changed()) { | |
| if (suffix.empty() && !value.toBool()) setOption(option, true, "-prev"); | |
| update(PruneSetting(value.toBool(), getOption(PruneSize).toInt())); | |
| if (suffix.empty() && value.toBool()) UpdateRwSetting(node(), option, "-prev", {}); | |
| if (suffix.empty()) setRestartRequired(true); | |
| } | |
| break; | |
| case PruneSize: | |
| if (changed()) { | |
| if (suffix.empty() && !getOption(Prune).toBool()) { | |
| setOption(option, value, "-prev"); | |
| } else { | |
| update(PruneSetting(true, ParsePruneSizeGB(value))); | |
| } | |
| break; | |
| if (suffix.empty() && getOption(Prune).toBool()) setRestartRequired(true); | |
| } | |
| break; | |
| #endif // ENABLE_WALLET | |
| case DatabaseCache: | |
| if (settings.value("nDatabaseCache") != value) { | |
| settings.setValue("nDatabaseCache", value); | |
| setRestartRequired(true); | |
| } | |
| break; | |
| case ThreadsScriptVerif: | |
| if (settings.value("nThreadsScriptVerif") != value) { | |
| settings.setValue("nThreadsScriptVerif", value); | |
| setRestartRequired(true); | |
| } | |
| break; | |
| case Listen: | |
| if (settings.value("fListen") != value) { | |
| settings.setValue("fListen", value); | |
| setRestartRequired(true); | |
| } | |
| break; | |
| case Server: | |
| if (settings.value("server") != value) { | |
| settings.setValue("server", value); | |
| setRestartRequired(true); | |
| } | |
| break; | |
| default: | |
| break; | |
| case DatabaseCache: | |
| #ifdef ENABLE_WALLET | |
| case CoinControlFeatures: | |
| fCoinControlFeatures = value.toBool(); | |
| settings.setValue("fCoinControlFeatures", fCoinControlFeatures); | |
| Q_EMIT coinControlFeaturesChanged(fCoinControlFeatures); | |
| break; | |
| case KeepChangeAddress: | |
| fKeepChangeAddress = value.toBool(); | |
| settings.setValue("fKeepChangeAddress", fKeepChangeAddress); | |
| Q_EMIT keepChangeAddressChanged(fKeepChangeAddress); | |
| break; | |
| #endif // ENABLE_WALLET | |
| case Prune: | |
| if (changed()) { | |
| if (suffix.empty() && !value.toBool()) setOption(option, true, "-prev"); | |
| update(PruneSetting(value.toBool(), getOption(PruneSize).toInt())); | |
| if (suffix.empty() && value.toBool()) UpdateRwSetting(node(), option, "-prev", {}); | |
| if (suffix.empty()) setRestartRequired(true); | |
| } | |
| break; | |
| case PruneSize: | |
| if (changed()) { | |
| if (suffix.empty() && !getOption(Prune).toBool()) { | |
| setOption(option, value, "-prev"); | |
| } else { | |
| update(PruneSetting(true, ParsePruneSizeGB(value))); | |
| } | |
| if (suffix.empty() && getOption(Prune).toBool()) setRestartRequired(true); | |
| } | |
| break; | |
| case DatabaseCache: |
🤖 Prompt for AI Agents
In src/qt/optionsmodel.cpp around lines 850 to 879, the Prune and PruneSize case
handlers are currently inside the #ifdef ENABLE_WALLET block which causes
setOption() to be skipped when wallet support is disabled; move the entire case
Prune: ... break; and case PruneSize: ... break; blocks out of the ENABLE_WALLET
conditional so they live before the #endif (i.e., unconditionally compiled),
preserving their current logic and surrounding suffix/restart behavior so
pruning settings apply whether or not the wallet is enabled.
| clientModel->getOptionsModel()->setOption(OptionsModel::OptionID::MaskValues, privacy); | ||
| if (m_balances.balance != -1) { | ||
| setBalance(m_balances); |
There was a problem hiding this comment.
Null check before persisting privacy option
clientModel or its OptionsModel can be null during early lifecycle; this will dereference unconditionally.
Apply:
- clientModel->getOptionsModel()->setOption(OptionsModel::OptionID::MaskValues, privacy);
+ if (clientModel && clientModel->getOptionsModel()) {
+ clientModel->getOptionsModel()->setOption(OptionsModel::OptionID::MaskValues, privacy);
+ }📝 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.
| clientModel->getOptionsModel()->setOption(OptionsModel::OptionID::MaskValues, privacy); | |
| if (m_balances.balance != -1) { | |
| setBalance(m_balances); | |
| if (clientModel && clientModel->getOptionsModel()) { | |
| clientModel->getOptionsModel()->setOption(OptionsModel::OptionID::MaskValues, privacy); | |
| } | |
| if (m_balances.balance != -1) { | |
| setBalance(m_balances); | |
| } |
🤖 Prompt for AI Agents
In src/qt/overviewpage.cpp around lines 210 to 212, the code unconditionally
calls clientModel->getOptionsModel()->setOption(...), which can dereference a
null clientModel or a null OptionsModel during early lifecycle; guard this by
checking clientModel is non-null and that clientModel->getOptionsModel() returns
a non-null pointer before calling setOption (fetch the OptionsModel into a local
variable and only call setOption when both are valid); leave the existing
setBalance(m_balances) as-is since the comment targets persisting the privacy
option.
| std::ifstream file(gArgs.GetDataDirNet() / "settings.json"); | ||
| QCOMPARE(std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()).c_str(), "{\n" | ||
| " \"dbcache\": \"600\",\n" | ||
| " \"listen\": false,\n" | ||
| " \"onion\": \"onion:234\",\n" | ||
| " \"par\": \"12\",\n" | ||
| " \"proxy\": \"proxy:123\",\n" | ||
| " \"prune\": \"2861\"\n" | ||
| "}\n"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make JSON verification robust; avoid comparing full file text and dangling c_str.
Comparing the whole serialized file string is brittle (ordering/whitespace) and uses c_str() on temporaries. Parse JSON and assert fields instead.
- std::ifstream file(gArgs.GetDataDirNet() / "settings.json");
- QCOMPARE(std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()).c_str(), "{\n"
- " \"dbcache\": \"600\",\n"
- " \"listen\": false,\n"
- " \"onion\": \"onion:234\",\n"
- " \"par\": \"12\",\n"
- " \"proxy\": \"proxy:123\",\n"
- " \"prune\": \"2861\"\n"
- "}\n");
+ std::ifstream file(gArgs.GetDataDirNet() / "settings.json");
+ QVERIFY(file.is_open());
+ const std::string data((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>());
+ UniValue json;
+ QVERIFY(json.read(data));
+ QVERIFY(json.isObject());
+ QCOMPARE(QString::fromStdString(json["dbcache"].get_str()), QString("600"));
+ QCOMPARE(json["listen"].get_bool(), false);
+ QCOMPARE(QString::fromStdString(json["onion"].get_str()), QString("onion:234"));
+ QCOMPARE(QString::fromStdString(json["par"].get_str()), QString("12"));
+ QCOMPARE(QString::fromStdString(json["proxy"].get_str()), QString("proxy:123"));
+ QCOMPARE(QString::fromStdString(json["prune"].get_str()), QString("2861"));📝 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.
| std::ifstream file(gArgs.GetDataDirNet() / "settings.json"); | |
| QCOMPARE(std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()).c_str(), "{\n" | |
| " \"dbcache\": \"600\",\n" | |
| " \"listen\": false,\n" | |
| " \"onion\": \"onion:234\",\n" | |
| " \"par\": \"12\",\n" | |
| " \"proxy\": \"proxy:123\",\n" | |
| " \"prune\": \"2861\"\n" | |
| "}\n"); | |
| } | |
| std::ifstream file(gArgs.GetDataDirNet() / "settings.json"); | |
| QVERIFY(file.is_open()); | |
| const std::string data((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>()); | |
| UniValue json; | |
| QVERIFY(json.read(data)); | |
| QVERIFY(json.isObject()); | |
| QCOMPARE(QString::fromStdString(json["dbcache"].get_str()), QString("600")); | |
| QCOMPARE(json["listen"].get_bool(), false); | |
| QCOMPARE(QString::fromStdString(json["onion"].get_str()), QString("onion:234")); | |
| QCOMPARE(QString::fromStdString(json["par"].get_str()), QString("12")); | |
| QCOMPARE(QString::fromStdString(json["proxy"].get_str()), QString("proxy:123")); | |
| QCOMPARE(QString::fromStdString(json["prune"].get_str()), QString("2861")); | |
| } |
🤖 Prompt for AI Agents
In src/qt/test/optiontests.cpp around lines 63 to 72, the test currently
compares the entire file text and calls c_str() on temporaries which is brittle
and unsafe; instead, read the file into a string or QByteArray, parse it as JSON
(e.g. QJsonDocument::fromJson or your project JSON parser), verify the parse
succeeded, and assert each expected key/value individually (dbcache as string or
number, listen as bool, onion/proxy as strings, par/prune as strings or numbers)
using QCOMPARE/QVERIFY; remove the direct full-string equality check and the
c_str() usage and add explicit error messages on parse failure.
The migration to CoinJoin was done in dash#4038, included in v0.17 and there's more than a few major releases since then, we should be able to safely assume that upgrading users are using more recent clients.
…settings Changes related to external signers are not included as we don't have support for them. Additionally, pruning affects governance and txindex.
We have a lot more configurable options and the current approach will get unwieldy as it grows
… onion-prev settings
Additional Information
Breaking Changes
Work in progress.
Checklist