backport: merge bitcoin-core/gui#594, #617, #612, #627, #711, #658, #813, #825, #626, #831, #835 (qt backports: part 5) #6829
Conversation
|
…s confirmation dialog
…in `UnlockContext`
kwvg
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe diff adds two pure virtual methods to interfaces/node.h: getMempoolMaxUsage() and getNetLocalAddresses(). CConnman exposes getNetLocalAddresses(); NodeImpl implements both new Node methods. ClientModel gains getNetLocalAddresses() and extends mempoolSizeChanged to include max usage. RPCConsole now shows local addresses and accepts mempool max usage; UI updated in debugwindow.ui. Significant SendCoinsEntry refactor from QStackedWidget to QWidget with corresponding .ui and .cpp changes; CSS adjusted. macOS guards switched to Q_OS_MACOS across multiple Qt files; Wayland handling added in guiutil. Intro prune checkbox logic adjusted. OptionsDialog now accepts a ClientModel. TransactionView manages opened dialogs. WalletController centralizes removal. UnlockContext becomes non-movable. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks (2 warnings, 1 inconclusive)❌ Failed Checks (2 warnings, 1 inconclusive)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.✨ 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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/qt/res/css/general.css (1)
1730-1763: Replace QStackedWidget#SendCoinsEntry selectors
Seven instances ofQStackedWidget#SendCoinsEntryinsrc/qt/res/css/general.css(lines 1730–1760) remain stale; update each prefix to#SendCoinsEntryso the rules apply to the new QWidget base.src/qt/bitcoingui.cpp (1)
1504-1520: Potential nullptr deref on CoinJoin client; add null checks.GetClient(...) may return nullptr if disabled/unavailable; guard before isMixing().
Apply this diff:
-#ifdef Q_OS_MACOS +#ifdef Q_OS_MACOS // Disabling macOS App Nap on initial sync, disk, reindex operations and mixing. bool disableAppNap = !m_node.masternodeSync().isSynced() || sync_state != SynchronizationState::POST_INIT; #ifdef ENABLE_WALLET if (enableWallet) { for (const auto& wallet : m_node.walletLoader().getWallets()) { - disableAppNap |= m_node.coinJoinLoader()->GetClient(wallet->getWalletName())->isMixing(); + if (auto cj_loader = m_node.coinJoinLoader()) { + if (auto cj_client = cj_loader->GetClient(wallet->getWalletName())) { + disableAppNap |= cj_client->isMixing(); + } + } } } #endif // ENABLE_WALLET
🧹 Nitpick comments (18)
src/qt/intro.h (1)
67-67: Document the new state flag for future maintainers.Add a short comment explaining when
m_prune_checkbox_is_defaultflips to false (CLI-prunepresent, or user interaction) and what behavior it gates.- bool m_prune_checkbox_is_default{true}; + // Tracks whether prune checkbox is still in the auto-updating state (no user override, no CLI -prune override). + bool m_prune_checkbox_is_default{true};src/qt/intro.cpp (1)
156-165: Treat any user interaction (including prune size edits) as an override of auto mode.Today only checkbox toggles flip
m_prune_checkbox_is_defaultto false. Consider also flipping it when the user adjusts the prune size so future auto-updates don’t flip the checkbox back.- connect(ui->pruneGB, qOverload<int>(&QSpinBox::valueChanged), [this](int prune_GB) { + connect(ui->pruneGB, qOverload<int>(&QSpinBox::valueChanged), [this](int prune_GB) { + m_prune_checkbox_is_default = false; m_prune_target_gb = prune_GB; UpdatePruneLabels(ui->prune->isChecked()); UpdateFreeSpaceLabel(); });src/qt/test/addressbooktests.cpp (1)
166-176: Use a compatibility guard to support older Qt (optional).If your minimum Qt is not guaranteed to define Q_OS_MACOS, prefer a dual check.
Apply:
-#ifdef Q_OS_MACOS +#if defined(Q_OS_MACOS) || defined(Q_OS_MAC)Please confirm the minimum Qt version targeted by Dash GUI builds for this branch (Qt ≥ 5.9+ defines Q_OS_MACOS). If not guaranteed, adopt the dual guard.
src/qt/notificator.h (1)
72-74: Mirror combined Q_OS_MACOS/Q_OS_MAC guards in header and implementation
- In src/qt/notificator.h, change the
#ifdef Q_OS_MACOSaround thenotifyMacUserNotificationCenterdeclaration to:-#ifdef Q_OS_MACOS +#if defined(Q_OS_MACOS) || defined(Q_OS_MAC) void notifyMacUserNotificationCenter(const QString &title, const QString &text); -#endif +#endif- In src/qt/notificator.cpp, update both
#ifdef Q_OS_MACOSblocks (around thenotifyMacUserNotificationCenterdefinition at ~line 213 and thecase UserNotificationCenter:call at ~line 233) to use#if defined(Q_OS_MACOS) || defined(Q_OS_MAC).src/qt/test/wallettests.cpp (1)
281-292: Optional: dual guard for broader Qt compatibility.Match the test guard to support environments that only define Q_OS_MAC.
-#ifdef Q_OS_MACOS +#if defined(Q_OS_MACOS) || defined(Q_OS_MAC)Please confirm CI/build matrix Qt versions; adopt the dual guard if any job uses an older Qt lacking Q_OS_MACOS.
src/qt/test/apptests.cpp (1)
62-73: Optional: dual macOS guard for test skip logic.Same rationale as other tests; keeps behavior identical while widening compatibility.
-#ifdef Q_OS_MACOS +#if defined(Q_OS_MACOS) || defined(Q_OS_MAC)Confirm Qt minimum; adjust guard accordingly across all updated test files for consistency.
src/qt/notificator.cpp (1)
21-23: Switch to Q_OS_MACOS: confirm Qt baseline or add compat guard.If any supported build still uses a Qt where Q_OS_MACOS is undefined (older 5.x), these guards will exclude macOS code. Please verify CI’s minimum Qt, or add a central compat define.
Example compat (place in a shared Qt compat header, not per .cpp):
+#if defined(Q_OS_MAC) && !defined(Q_OS_MACOS) +#define Q_OS_MACOS 1 +#endifAlso applies to: 53-58, 213-219, 233-237
src/qt/optionsdialog.h (1)
12-12: Non-owning ClientModel pointer: prefer QPointer to avoid dangling.OptionsDialog doesn’t own ClientModel; using QPointer guards against use-after-free if the model is deleted elsewhere.
@@ -#include <QValidator> +#include <QValidator> +#include <QPointer> @@ - void setClientModel(ClientModel* client_model); + void setClientModel(ClientModel* client_model); @@ - ClientModel* m_client_model{nullptr}; + QPointer<ClientModel> m_client_model{nullptr};Also applies to: 55-56, 91-91
src/qt/walletmodel.h (1)
137-140: Make WalletModel pointer itself immutableFor full immutability of the guard state, consider making the pointer itself const:
- WalletModel *wallet; + WalletModel* const wallet;src/qt/forms/sendcoinsentry.ui (1)
219-226: Improve keyboard navigation: complete tab orderInclude the fee checkbox and “Use available balance” button in tabstops.
<tabstops> <tabstop>payTo</tabstop> <tabstop>addressBookButton</tabstop> <tabstop>pasteButton</tabstop> <tabstop>deleteButton</tabstop> <tabstop>addAsLabel</tabstop> <tabstop>payAmount</tabstop> + <tabstop>checkboxSubtractFeeFromAmount</tabstop> + <tabstop>useAvailableBalanceButton</tabstop> </tabstops>src/qt/overviewpage.cpp (1)
214-217: Rename ambiguous UI object name for maintainability
Rename the auto-generated QLabellabel_4insrc/qt/forms/overviewpage.uito a descriptive name (e.g.labelRecentTransactions), then update its uses insrc/qt/overviewpage.cpp:
- In the GUIUtil::setFont call (around line 152), replace
ui->label_4withui->labelRecentTransactions.- In the visibility toggle (line 216), replace
ui->label_4withui->labelRecentTransactions.
Afterwards, runrg -nC2 '\blabel_4\b' -g '*.cpp' -g '*.ui'to confirm no remaining references.
src/qt/bitcoingui.h (1)
345-347: Clarify slot naming/semantics.enableHistoryButton(bool privacy) takes a privacy flag but implies an “enable” boolean. Consider either renaming to onPrivacyToggled(bool privacy) or changing the parameter to enabled and wiring as setHistoryButtonEnabled(bool enabled). Ensure the implementation sets historyButton->setEnabled(!privacy).
Please confirm the implementation mirrors the intended logic.
src/net.cpp (1)
4452-4456: Name casing: prefer GetNetLocalAddresses for CConnmanCConnman methods use PascalCase (e.g., GetNodeStats, GetTotalBytesSent). Consider renaming for consistency.
Apply in this file:
-std::map<CNetAddr, LocalServiceInfo> CConnman::getNetLocalAddresses() const +std::map<CNetAddr, LocalServiceInfo> CConnman::GetNetLocalAddresses() constAlso update the declaration in net.h and all call sites (e.g., NodeImpl).
src/qt/clientmodel.cpp (1)
160-164: Returning a copy is fine; consider naming consistencyReturning a snapshot avoids locking issues; OK. If CConnman method is renamed to GetNetLocalAddresses, update this call accordingly.
- return m_node.getNetLocalAddresses(); + return m_node.getNetLocalAddresses(); // or GetNetLocalAddresses() if CConnman is renamedsrc/node/interfaces.cpp (1)
534-540: Follow CConnman naming and avoid duplicate styleIf CConnman is switched to GetNetLocalAddresses(), mirror it here. Otherwise, this looks good.
- std::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() override + std::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() override { - if (m_context->connman) - return m_context->connman->getNetLocalAddresses(); - else - return {}; + if (m_context->connman) return m_context->connman->getNetLocalAddresses(); // GetNetLocalAddresses() if renamed + return {}; }src/qt/walletview.cpp (1)
384-388: Null-safety is fine; consider also clearing sensitive textOptionally clear the sum text when disabling to avoid stale values lingering.
void WalletView::disableTransactionView(bool disable) { transactionView->setDisabled(disable); + if (disable && transactionSum) transactionSum->clear(); }src/qt/forms/debugwindow.ui (1)
300-333: Consider improving the tooltip text for Local Addresses.The tooltip text "Network addresses that your Dash node is currently using to communicate with other nodes" is good, but could be more specific about what "local addresses" means to avoid confusion with peer addresses shown elsewhere in the UI.
Consider updating the tooltip to be more explicit:
- <string>Network addresses that your Dash node is currently using to communicate with other nodes.</string> + <string>Local network addresses that your Dash node is advertising to other nodes for incoming connections.</string>src/qt/rpcconsole.cpp (1)
1110-1120: Signal/slot arity matches:ClientModel::mempoolSizeChangedis declared as(long, size_t, size_t)in clientmodel.h and emitted with three arguments in clientmodel.cpp.
Optional: useGUIUtil::formatBytesfor consistent byte-unit formatting across the UI.
📜 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 (34)
src/interfaces/node.h(1 hunks)src/net.cpp(1 hunks)src/net.h(1 hunks)src/node/interfaces.cpp(2 hunks)src/qt/bitcoingui.cpp(18 hunks)src/qt/bitcoingui.h(3 hunks)src/qt/clientmodel.cpp(2 hunks)src/qt/clientmodel.h(3 hunks)src/qt/forms/debugwindow.ui(12 hunks)src/qt/forms/sendcoinsentry.ui(3 hunks)src/qt/guiutil.cpp(12 hunks)src/qt/intro.cpp(3 hunks)src/qt/intro.h(1 hunks)src/qt/notificator.cpp(4 hunks)src/qt/notificator.h(1 hunks)src/qt/optionsdialog.cpp(8 hunks)src/qt/optionsdialog.h(3 hunks)src/qt/overviewpage.cpp(1 hunks)src/qt/res/css/general.css(1 hunks)src/qt/rpcconsole.cpp(3 hunks)src/qt/rpcconsole.h(1 hunks)src/qt/sendcoinsentry.cpp(3 hunks)src/qt/sendcoinsentry.h(2 hunks)src/qt/test/addressbooktests.cpp(1 hunks)src/qt/test/apptests.cpp(1 hunks)src/qt/test/wallettests.cpp(1 hunks)src/qt/transactionview.cpp(4 hunks)src/qt/transactionview.h(3 hunks)src/qt/walletcontroller.cpp(3 hunks)src/qt/walletcontroller.h(1 hunks)src/qt/walletmodel.cpp(0 hunks)src/qt/walletmodel.h(2 hunks)src/qt/walletview.cpp(2 hunks)src/qt/walletview.h(1 hunks)
💤 Files with no reviewable changes (1)
- src/qt/walletmodel.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.cppsrc/qt/test/wallettests.cpp
⏰ 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). (3)
- GitHub Check: linux64_tsan-test / Test source
- GitHub Check: linux64_ubsan-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
🔇 Additional comments (33)
src/qt/intro.cpp (1)
298-301: Good: auto-prune now respects user/CLI overrides.Gating the auto-check by
ui->prune->isEnabled()andm_prune_checkbox_is_defaultavoids surprising flips after user input or explicit-prune.This subtly changes behavior vs. earlier heuristics. Since this PR is a backport bundle, confirm this behavior change is intended for the backport set and not expected to be move-only.
src/qt/walletcontroller.h (1)
86-87: No changes needed: removeWallet() only starts the node‐level removal; removeAndDeleteWallet() is the single entry point for unregistering the model
removeAndDeleteWallet() locks m_mutex, erases from m_wallets, emits walletRemoved on the GUI thread (via Qt::QueuedConnection), and deletes the model—no duplication with removeWallet() and thread‐safety is ensured.Likely an incorrect or invalid review comment.
src/qt/sendcoinsentry.h (1)
10-11: Base-class change (QStackedWidget → QWidget): audit dependent CSS and code.The object is now a QWidget. Any selectors like “QStackedWidget#SendCoinsEntry …” will no longer match. See general.css comments—update to “#SendCoinsEntry …”.
Also applies to: 25-26
src/qt/walletmodel.h (1)
129-134: Ensure C++17+ enforcement in the autotools build and no residual UnlockContext moves
CMakeLists.txt sets C++20 for IDE use, but confirm configure.ac/Makefile.am (the automake chain) adds -std=c++17 or higher. Also verify there are no remainingstd::moveor assignment usages ofUnlockContextin callsites.src/qt/transactionview.cpp (1)
138-144: Mac guard update + scrollbar policy LGTMsrc/qt/walletcontroller.cpp (1)
80-86: Centralizing wallet removal via helper is a good cleanupsrc/qt/guiutil.cpp (1)
83-88: Confirm Q_OS_MACOS availability in Qt 5.11.3+ The project’s minimum Qt version has been raised to 5.11.3 and above, and Q_OS_MACOS is defined in all supported Qt 5.x releases (github.com, doc.qt.io); grep shows no remaining Q_OS_MAC in src/qt files (doc.qt.io).src/qt/bitcoingui.h (2)
27-29: macOS guard modernization LGTM.Switching Q_OS_MAC → Q_OS_MACOS is correct and consistent with Qt’s current platform macros.
196-199: macOS guard modernization LGTM (member section).Consistent with the header include change above.
src/qt/clientmodel.cpp (1)
55-62: Good: propagate mempool max usage to UIEmitting max usage alongside current usage is correct and cheap. No issues.
src/node/interfaces.cpp (1)
523-523: Integer overflow risk in mempool max computationGetIntArg returns int; multiplying by 1,000,000 can overflow for large -maxmempool (e.g., >2,147 MiB). Use wider type and clamp.
- size_t getMempoolMaxUsage() override { return gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000; } + size_t getMempoolMaxUsage() override { + const int64_t mib = gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE); + if (mib <= 0) return 0; + const uint64_t bytes = static_cast<uint64_t>(mib) * 1000000ULL; + // Clamp to size_t to avoid wrap on 32-bit + return bytes > std::numeric_limits<size_t>::max() ? std::numeric_limits<size_t>::max() : static_cast<size_t>(bytes); + }Likely an incorrect or invalid review comment.
src/interfaces/node.h (2)
236-237: LGTM! Well-placed API addition for mempool max usage.The new
getMempoolMaxUsage()method is appropriately positioned after the dynamic usage getter, maintaining logical grouping of mempool-related methods.
245-246: LGTM! Appropriate addition for local address exposure.The
getNetLocalAddresses()method correctly returns a map ofCNetAddrtoLocalServiceInfo, which aligns with the network information exposure pattern established by other methods in this interface.src/qt/clientmodel.h (3)
19-20: LGTM! Clean API addition with proper forward declaration.The addition of
netaddress.hinclude and forward declaration ofLocalServiceInfois appropriate for the newgetNetLocalAddresses()method. The forward declaration follows best practices to minimize header dependencies.Also applies to: 27-27
75-75: LGTM! Consistent API design.The new
getNetLocalAddresses()method follows the established naming convention and const-correctness pattern of the class.
137-137: LGTM! Signal signature properly extended.The
mempoolSizeChangedsignal now includesmempoolMaxSizeInBytesas the third parameter, which is a backward-compatible extension that provides more complete information to slots.src/qt/sendcoinsentry.cpp (3)
23-23: LGTM! Correct base class change to QWidget.The change from QStackedWidget to QWidget aligns with the UI simplification in sendcoinsentry.ui, removing the multi-state payment request UI complexity.
227-229: LGTM! Clean simplification of display unit updates.The method now only updates the main payAmount field's display unit, consistent with the removal of the multi-state payment UI.
234-234: LGTM! Correct base class method call.Changed to call
QWidget::changeEvent(e)instead ofQStackedWidget::changeEvent(e), properly reflecting the base class change.src/qt/optionsdialog.cpp (3)
57-57: LGTM! Consistent platform macro update.The change from
Q_OS_MACtoQ_OS_MACOSaligns with Qt's modern platform detection macros. This is the correct and more specific macro for macOS.Also applies to: 107-107, 323-323
592-594: LGTM! Improved proxy validation logic.The enhanced validation using
SplitHostPortproperly rejects proxy addresses that include a port number in the host field, preventing configuration errors.
220-223: Add null check for client_model parameter.The
setClientModelmethod should validate the input parameter to prevent potential null pointer issues.void OptionsDialog::setClientModel(ClientModel* client_model) { + if (!client_model) return; m_client_model = client_model; }Likely an incorrect or invalid review comment.
src/qt/rpcconsole.cpp (1)
974-978: Consistent macOS guard usage.Switch to Q_OS_MACOS looks correct and consistent with other files.
src/qt/bitcoingui.cpp (10)
29-31: macOS include guard update is fine.
76-85: DEFAULT_UIPLATFORM macOS guard looks correct.
231-234: AppNap inhibitor creation under macOS looks correct.
822-845: Tray vs Dock behavior split is appropriate.
909-915: Dock menu cleanup on model teardown under macOS is correct.
1085-1090: Non-macOS tray Show/Hide wiring looks good.
1122-1126: Quit action excluded on macOS (handled by Dock menu) is correct.
1735-1753: Minimize-to-tray handling skipped on macOS (as intended).
1768-1787: CloseEvent macOS handling is correct (use default behavior).
1430-1431: OptionsDialog::setClientModel is properly implemented and used: the setter storesclient_modelinm_client_modeland is later referenced (e.g. callingm_client_model->dataDir()) in the dialog logic. No further action needed.
| void AddPendingProbeConnections(const std::set<uint256>& proTxHashes); | ||
|
|
||
| size_t GetNodeCount(ConnectionDirection) const; | ||
| std::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() const; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align method name with project style (PascalCase) and add noexcept/[[nodiscard]].
CConnman methods use PascalCase (e.g., GetNodeCount). Rename for consistency and add noexcept/[[nodiscard]] since this is a const accessor returning a copy.
Apply this diff:
- std::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() const;
+ [[nodiscard]] std::map<CNetAddr, LocalServiceInfo> GetNetLocalAddresses() const noexcept;Then update definition and call sites. To locate them:
🏁 Script executed:
#!/bin/bash
# Update/verify implementation and uses
rg -n 'getNetLocalAddresses\s*\(' -g 'src/**'
rg -n 'GetNetLocalAddresses\s*\(' -g 'src/**'Length of output: 952
Rename getNetLocalAddresses to GetNetLocalAddresses and add [[nodiscard]] noexcept
Aligns with CConnman’s PascalCase convention and marks the accessor as non-throwing and its return value as used.
- Update declaration in src/net.h (l. 1492):
- std::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() const; + [[nodiscard]] std::map<CNetAddr, LocalServiceInfo> GetNetLocalAddresses() const noexcept;
- Update definition in src/net.cpp (l. 4452): rename to
CConnman::GetNetLocalAddresses()and addnoexcept. - Update override in src/node/interfaces.cpp (ll. 534–537) and virtual declaration in src/interfaces/node.h (l. 246).
- Update ClientModel in src/qt/clientmodel.h (l. 75) and src/qt/clientmodel.cpp (ll. 160–162).
- Update call site in src/qt/rpcconsole.cpp (l. 1050).
- Rename all remaining calls from
getNetLocalAddresses()toGetNetLocalAddresses().
📝 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::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() const; | |
| [[nodiscard]] std::map<CNetAddr, LocalServiceInfo> GetNetLocalAddresses() const noexcept; |
🤖 Prompt for AI Agents
In src/net.h around line 1492, rename the accessor declaration from
getNetLocalAddresses to GetNetLocalAddresses and annotate it with [[nodiscard]]
and noexcept; then update the corresponding definition in src/net.cpp around
line 4452 to CConnman::GetNetLocalAddresses() and add noexcept, update the
virtual declaration in src/interfaces/node.h at line 246 and the override in
src/node/interfaces.cpp (lines ~534–537) to match the new name and noexcept,
update ClientModel declaration/definition in src/qt/clientmodel.h (line ~75) and
src/qt/clientmodel.cpp (lines ~160–162), update the call site in
src/qt/rpcconsole.cpp (line ~1050), and perform a project-wide replacement of
all remaining calls from getNetLocalAddresses() to GetNetLocalAddresses();
ensure signatures (including const, return type) remain identical except for the
name and added [[nodiscard]] noexcept, and rebuild to catch any missed
references.
| connect(m_mask_values_action, &QAction::toggled, this, &BitcoinGUI::setPrivacy); | ||
| connect(m_mask_values_action, &QAction::toggled, this, &BitcoinGUI::enableHistoryButton); | ||
| } |
There was a problem hiding this comment.
Privacy mode can still navigate to History programmatically; guard navigation.
Signals like transactionClicked/coinsSent still call gotoHistoryPage(). Add a privacy check inside gotoHistoryPage() to prevent bypass via programmatic triggers.
Proposed addition (outside this hunk):
void BitcoinGUI::gotoHistoryPage()
{
if (isPrivacyModeActivated()) { gotoOverviewPage(); return; }
historyButton->setChecked(true);
if (walletFrame) walletFrame->gotoHistoryPage();
}🤖 Prompt for AI Agents
In src/qt/bitcoingui.cpp around lines 587 to 589, programmatic navigation
(signals like transactionClicked/coinsSent) can still reach the History page
despite the UI privacy toggle; add a guard at the start of
BitcoinGUI::gotoHistoryPage() that checks isPrivacyModeActivated(), and if true
calls gotoOverviewPage() and returns; otherwise continue with the existing
behavior (set historyButton checked and forward the call to
walletFrame->gotoHistoryPage() if walletFrame exists).
src/qt/bitcoingui.cpp
Outdated
| void BitcoinGUI::enableHistoryButton(bool privacy) | ||
| { | ||
| if (walletFrame->currentWalletModel()) { | ||
| historyButton->setEnabled(!privacy); | ||
| if (historyButton->isChecked()) gotoOverviewPage(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
History button enable/disable is good; also stop programmatic switches.
The UI state change is handled; pair it with the gotoHistoryPage() guard to fully enforce privacy mode.
🤖 Prompt for AI Agents
In src/qt/bitcoingui.cpp around lines 921-928, enableHistoryButton correctly
disables the historyButton in privacy mode but you must also prevent
programmatic navigation into the history page; add a guard at the start of
gotoHistoryPage() that checks the current wallet model's privacy state (or a
stored privacy flag) and returns immediately if privacy is enabled, and ensure
any code that programmatically checks historyButton does not call
gotoHistoryPage() when privacy is true (use the guard rather than relying on
button state changes).
| <property name="text"> | ||
| <string notr="true">N/A</string> | ||
| </property> |
There was a problem hiding this comment.
Remove the notr attribute from the N/A string.
The notr="true" attribute on line 321 prevents translation of "N/A", which is inconsistent with other N/A labels in this file that are translatable. This should be consistent across all similar labels.
- <string notr="true">N/A</string>
+ <string>N/A</string>📝 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.
| <property name="text"> | |
| <string notr="true">N/A</string> | |
| </property> | |
| <property name="text"> | |
| <string>N/A</string> | |
| </property> |
🤖 Prompt for AI Agents
In src/qt/forms/debugwindow.ui around lines 320 to 322, the <property
name="text"> element contains a <string notr="true">N/A</string> which prevents
translation; remove the notr="true" attribute so the string reads simply
<string>N/A</string> (i.e., make the "N/A" string translatable to match other
labels in the file).
| <widget class="QLabel" name="labellLabel"> | ||
| <property name="text"> | ||
| <string>&Label:</string> | ||
| </property> | ||
| <item row="0" column="0"> | ||
| <widget class="QLabel" name="payToLabel_s"> | ||
| <property name="text"> | ||
| <string>Pay To:</string> | ||
| </property> | ||
| <property name="alignment"> | ||
| <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set> | ||
| </property> | ||
| </widget> | ||
| </item> | ||
| <item row="0" column="1"> | ||
| <layout class="QHBoxLayout" name="payToLayout_s"> | ||
| <property name="spacing"> | ||
| <number>0</number> | ||
| </property> | ||
| <item> | ||
| <widget class="QLabel" name="payTo_s"> | ||
| <property name="textFormat"> | ||
| <enum>Qt::PlainText</enum> | ||
| </property> | ||
| </widget> | ||
| </item> | ||
| <item> | ||
| <widget class="QToolButton" name="deleteButton_s"> | ||
| <property name="alignment"> | ||
| <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set> | ||
| </property> | ||
| <property name="buddy"> | ||
| <cstring>addAsLabel</cstring> | ||
| </property> | ||
| </widget> | ||
| </item> |
There was a problem hiding this comment.
Fix widget name typo: labellLabel → labelLabel
The extra “l” will break any CSS or code targeting the old objectName.
- <widget class="QLabel" name="labellLabel">
+ <widget class="QLabel" name="labelLabel">📝 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.
| <widget class="QLabel" name="labellLabel"> | |
| <property name="text"> | |
| <string>&Label:</string> | |
| </property> | |
| <item row="0" column="0"> | |
| <widget class="QLabel" name="payToLabel_s"> | |
| <property name="text"> | |
| <string>Pay To:</string> | |
| </property> | |
| <property name="alignment"> | |
| <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set> | |
| </property> | |
| </widget> | |
| </item> | |
| <item row="0" column="1"> | |
| <layout class="QHBoxLayout" name="payToLayout_s"> | |
| <property name="spacing"> | |
| <number>0</number> | |
| </property> | |
| <item> | |
| <widget class="QLabel" name="payTo_s"> | |
| <property name="textFormat"> | |
| <enum>Qt::PlainText</enum> | |
| </property> | |
| </widget> | |
| </item> | |
| <item> | |
| <widget class="QToolButton" name="deleteButton_s"> | |
| <property name="alignment"> | |
| <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set> | |
| </property> | |
| <property name="buddy"> | |
| <cstring>addAsLabel</cstring> | |
| </property> | |
| </widget> | |
| </item> | |
| <widget class="QLabel" name="labelLabel"> | |
| <property name="text"> | |
| <string>&Label:</string> | |
| </property> | |
| <property name="alignment"> | |
| <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set> | |
| </property> | |
| <property name="buddy"> | |
| <cstring>addAsLabel</cstring> | |
| </property> | |
| </widget> | |
| </item> |
🤖 Prompt for AI Agents
In src/qt/forms/sendcoinsentry.ui around lines 114 to 125 the QLabel widget has
a typo in its name attribute ("labellLabel") which will break any code or CSS
targeting the object; change the widget name to "labelLabel" in the UI file and
then search the project for references to "labellLabel" (UI code, stylesheets,
tests) and update them to "labelLabel" to keep references consistent.
src/qt/transactionview.cpp
Outdated
| m_opened_dialogs.append(dlg); | ||
| connect(dlg, &QObject::destroyed, [this, dlg] { | ||
| m_opened_dialogs.removeOne(dlg); | ||
| }); | ||
| dlg->show(); |
There was a problem hiding this comment.
Fix container modification during iteration when closing dialogs
closeOpenedDialogs() iterates m_opened_dialogs while each dlg->close() triggers destroyed → removeOne(dlg), mutating the same QList during iteration. This risks skipped elements or UB. Iterate over a copy, then clear.
void TransactionView::closeOpenedDialogs()
{
- // close all dialogs opened from this view
- for (QDialog* dlg : m_opened_dialogs) {
- dlg->close();
- }
- m_opened_dialogs.clear();
+ // Close all dialogs opened from this view without modifying the list during iteration
+ const auto dialogs = m_opened_dialogs; // copy
+ for (QDialog* dlg : dialogs) {
+ if (dlg) dlg->close();
+ }
+ m_opened_dialogs.clear();
}Also applies to: 731-736, 746-754
src/qt/transactionview.h
Outdated
| class TransactionDescDialog; | ||
| class TransactionFilterProxy; |
There was a problem hiding this comment.
Missing include; consider safer storage for dialogs.
- This header uses QList in a data member; add the proper include to avoid ODR/order fragility.
- Optional: store QPointer to avoid dangling if dialogs self-destruct.
@@
-#include <QWidget>
+#include <QWidget>
+#include <QList>
+#include <QPointer>
@@
- QList<TransactionDescDialog*> m_opened_dialogs;
+ QList<QPointer<TransactionDescDialog>> m_opened_dialogs;Also applies to: 90-91, 126-127
🤖 Prompt for AI Agents
In src/qt/transactionview.h around lines 15-16 (and also apply at lines 90-91
and 126-127), the header currently uses QList in a data member without including
the QList header and stores raw pointers to TransactionDescDialog which can
dangle if dialogs self-destruct; add the proper #include for QList and #include
for QPointer (or the forward declaration for QPointer header), and change the
member type to QList<QPointer<TransactionDescDialog>> (or otherwise use
QPointer<TransactionDescDialog> elements) so the list holds guarded pointers
that become null if dialogs are deleted.
| QMutexLocker locker(&m_mutex); | ||
| for (WalletModel* wallet_model : m_wallets) { | ||
| wallet_model->wallet().remove(); | ||
| Q_EMIT walletRemoved(wallet_model); | ||
| delete wallet_model; | ||
| removeWallet(wallet_model); | ||
| } | ||
| m_wallets.clear(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid holding m_mutex while initiating removals
closeAllWallets() holds m_mutex and calls removeWallet(); unload is queued, but keeping the lock around external calls is fragile and can deadlock with future changes. Snapshot under lock, release, then remove.
- QMutexLocker locker(&m_mutex);
- for (WalletModel* wallet_model : m_wallets) {
- removeWallet(wallet_model);
- }
+ std::vector<WalletModel*> wallets_copy;
+ {
+ QMutexLocker locker(&m_mutex);
+ wallets_copy = m_wallets;
+ }
+ for (WalletModel* wallet_model : wallets_copy) {
+ removeWallet(wallet_model);
+ }📝 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.
| QMutexLocker locker(&m_mutex); | |
| for (WalletModel* wallet_model : m_wallets) { | |
| wallet_model->wallet().remove(); | |
| Q_EMIT walletRemoved(wallet_model); | |
| delete wallet_model; | |
| removeWallet(wallet_model); | |
| } | |
| m_wallets.clear(); | |
| } | |
| std::vector<WalletModel*> wallets_copy; | |
| { | |
| QMutexLocker locker(&m_mutex); | |
| wallets_copy = m_wallets; | |
| } | |
| for (WalletModel* wallet_model : wallets_copy) { | |
| removeWallet(wallet_model); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/qt/walletcontroller.cpp around lines 109 to 113, closeAllWallets()
currently holds m_mutex while calling removeWallet(), which can deadlock; change
it to take the mutex, copy m_wallets into a local list/vector (snapshot),
release the mutex (end the QMutexLocker scope), then iterate over the snapshot
and call removeWallet() for each entry so removals happen without holding
m_mutex.
| connect(this, &WalletView::setPrivacy, overviewPage, &OverviewPage::setPrivacy); | ||
| connect(this, &WalletView::setPrivacy, this, &WalletView::disableTransactionView); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extend privacy toggle to other widgets (export, sum)
Disabling only the transaction table still leaves “Export” and the “Selected amount” visible/usable. Consider disabling/hiding these in privacy mode too.
Proposed minimal approach (store a pointer to the Export button and toggle both it and the sum labels):
@@
- QPushButton *exportButton = new QPushButton(tr("&Export"), this);
+ m_exportButton = new QPushButton(tr("&Export"), this);
@@
- hbox_buttons->addWidget(exportButton);
+ hbox_buttons->addWidget(m_exportButton);And in class (walletview.h):
private:
QPushButton* m_exportButton{nullptr};
QLabel* transactionSumLabel{nullptr};
Then update disable slot:
void WalletView::disableTransactionView(bool disable)
{
transactionView->setDisabled(disable);
+ if (m_exportButton) m_exportButton->setDisabled(disable);
+ if (transactionSum) transactionSum->setVisible(!disable);
+ if (transactionSumLabel) transactionSumLabel->setVisible(!disable);
}Also wire transactionSumLabel as a member where it’s created.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/qt/walletview.cpp around lines 139-141, the privacy toggle currently only
disables the transaction table; extend it to also disable/hide the Export button
and the Selected amount label(s). Add member pointers in walletview.h (e.g.
QPushButton* m_exportButton and QLabel* transactionSumLabel), initialize and
assign them where the Export button and sum label(s) are created, and update the
disableTransactionView slot (and the connect for setPrivacy) to also
setVisible/setEnabled or hide/show those widgets when privacy is
enabled/disabled so Export and the sum are not usable/visible in privacy mode.
src/qt/walletview.h
Outdated
| private Q_SLOTS: | ||
| void disableTransactionView(bool disable); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against potential null dereference in slot implementation.
disableTransactionView(bool) likely calls transactionView->setDisabled(disable). If privacy toggles before the view is constructed, this will crash.
Implement the slot defensively in src/qt/walletview.cpp:
void WalletView::disableTransactionView(bool disable)
{
if (transactionView) {
transactionView->setDisabled(disable);
}
}🤖 Prompt for AI Agents
In src/qt/walletview.h around lines 126 to 128, the disableTransactionView(bool)
slot may be invoked before transactionView is constructed causing a null
dereference; update the implementation in src/qt/walletview.cpp to check
transactionView for null before calling setDisabled(disable) (i.e., only call
transactionView->setDisabled(disable) if transactionView is non-null).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/bitcoingui.cpp (1)
1038-1044: Keep Transactions button disabled under privacy after state flips.setWalletActionsEnabled() can re-enable the button; fold privacy into the enablement.
- historyButton->setEnabled(enabled); + historyButton->setEnabled(enabled && !isPrivacyModeActivated());
🧹 Nitpick comments (2)
src/qt/clientmodel.h (2)
15-21: Add missing include to match std::map usage.Prevents relying on transitive includes and avoids compile fragility.
#include <atomic> #include <memory> #include <uint256.h> +#include <map> + #include <netaddress.h>
75-75: New API makes sense; copy return is acceptable.Returning by value avoids exposing internal storage via references. If this becomes hot, consider returning a lightweight snapshot (e.g., small vector of pairs) but not required now.
📜 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 (18)
src/interfaces/node.h(1 hunks)src/net.cpp(1 hunks)src/net.h(1 hunks)src/node/interfaces.cpp(2 hunks)src/qt/bitcoingui.cpp(18 hunks)src/qt/clientmodel.cpp(2 hunks)src/qt/clientmodel.h(3 hunks)src/qt/forms/debugwindow.ui(12 hunks)src/qt/guiutil.cpp(12 hunks)src/qt/intro.cpp(3 hunks)src/qt/intro.h(1 hunks)src/qt/optionsdialog.cpp(8 hunks)src/qt/overviewpage.cpp(1 hunks)src/qt/rpcconsole.cpp(3 hunks)src/qt/rpcconsole.h(1 hunks)src/qt/transactionview.cpp(4 hunks)src/qt/walletcontroller.cpp(3 hunks)src/qt/walletcontroller.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- src/interfaces/node.h
- src/qt/rpcconsole.h
- src/net.h
- src/qt/forms/debugwindow.ui
- src/node/interfaces.cpp
- src/qt/walletcontroller.h
- src/qt/intro.h
- src/net.cpp
- src/qt/transactionview.cpp
- src/qt/walletcontroller.cpp
- src/qt/intro.cpp
- src/qt/guiutil.cpp
- src/qt/optionsdialog.cpp
- src/qt/rpcconsole.cpp
- src/qt/clientmodel.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/clientmodel.hsrc/qt/bitcoingui.cppsrc/qt/overviewpage.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/clientmodel.h (2)
src/net.cpp (2)
getNetLocalAddresses(4452-4456)getNetLocalAddresses(4452-4452)src/qt/clientmodel.cpp (2)
getNetLocalAddresses(160-163)getNetLocalAddresses(160-160)
src/qt/bitcoingui.cpp (3)
src/qt/bitcoingui.h (1)
m_mask_values_action(181-395)src/qt/walletview.cpp (2)
gotoOverviewPage(225-228)gotoOverviewPage(225-225)src/qt/walletframe.cpp (2)
gotoOverviewPage(180-191)gotoOverviewPage(180-180)
src/qt/overviewpage.cpp (1)
src/qt/overviewpage.h (1)
m_privacy(55-74)
⏰ 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-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
🔇 Additional comments (6)
src/qt/clientmodel.h (2)
27-27: Forward declaration is fine here.Using a forward-declared LocalServiceInfo in a return type is OK since the function is only declared in this header.
137-137: Resolved: AllmempoolSizeChangedconnects/slots updated
Verified thatClientModel::mempoolSizeChangedis only connected insrc/qt/rpcconsole.cpp(line 715) and the corresponding slotRPCConsole::setMempoolSize(long, size_t, size_t)at line 1110 matches the new signature. No other references found.src/qt/overviewpage.cpp (1)
214-218: Correct visibility logic for “out of sync” label.Gating labelTransactionsStatus on both privacy and labelWalletStatus fixes the prior re-show bug without fighting showOutOfSyncWarning().
src/qt/bitcoingui.cpp (3)
29-31: Q_OS_MACOS macro: confirm Qt version coverage or add compatibility.If older Qt (where Q_OS_MACOS is undefined) is still supported, these guards will exclude mac-specific code. Either:
- Confirm minimum Qt ensures Q_OS_MACOS is defined, or
- Add a compatibility shim (central header) mapping Q_OS_MAC → Q_OS_MACOS.
Also applies to: 76-85, 231-234, 265-268, 647-661, 664-671, 822-845, 908-915, 1084-1089, 1121-1126, 1504-1520, 1735-1753, 1769-1787
1430-1431: Passing ClientModel into OptionsDialog is a solid fix.Avoids null access and lets the dialog reflect current settings.
587-589: History navigation fully guarded
Global grep confirms no direct calls to gotoHistoryPage bypass BitcoinGUI’s guard—all history transitions respect the privacy-disabling check.
src/qt/bitcoingui.cpp
Outdated
| #ifdef ENABLE_WALLET | ||
| void BitcoinGUI::enableHistoryButton(bool privacy) | ||
| { | ||
| if (walletFrame->currentWalletModel()) { | ||
| historyButton->setEnabled(!privacy); | ||
| if (historyButton->isChecked()) gotoOverviewPage(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Block programmatic navigation to History when privacy is on.
Signals still call gotoHistoryPage(); add a guard to enforce privacy mode.
void BitcoinGUI::gotoHistoryPage()
{
+ if (isPrivacyModeActivated()) {
+ gotoOverviewPage();
+ return;
+ }
historyButton->setChecked(true);
if (walletFrame) walletFrame->gotoHistoryPage();
}Also applies to: 971-974, 1269-1273
🤖 Prompt for AI Agents
In src/qt/bitcoingui.cpp around lines 920-928 (and similarly at 971-974 and
1269-1273), the code allows programmatic navigation to the History page even
when privacy mode is enabled; add a guard that prevents entering History when
privacy is true. Modify the place(s) that call gotoHistoryPage() (and the
gotoHistoryPage() implementation itself if present) to first check the
walletFrame->currentWalletModel() and a privacy flag (or the same boolean
parameter used in enableHistoryButton) and return early without navigating if
privacy is enabled; also ensure historyButton clicks/signals respect this guard
and do not call gotoHistoryPage() when privacy is on so programmatic and
signal-driven navigation are both blocked.
|
I'd personal do skip bitcoin-core/gui#708 as DNM. This PR breaks UX imo. Hiding "recent transactions" - reasonable; but if user intentionally clicks to "transaction" it is same many other user cases when user can discover his funds: type "wallet balance in RPC console; create a new TX; clicks "inputs" when creating new tx, .... many other ways. And disabling this one doesn't make any sense. I believe "overview page" to be masked is a good enough for this feature. @UdjinM6 @PastaPastaPasta what do you think? |
|
I think I concur that 708 may go too far, and can be skipped, but I'm open to other options. |
Agree, switching discreet mode isn't an actual protection. Let's drop 708 and keep discreet mode just a minor feature for the Overview tab only. |
Currently the label moves downward when discreet mode is enabled, which is a bit visually jarring. We're better off hiding it wholesale. Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>
… user has touched it
kwvg
changed the title
kwvg
requested review from
PastaPastaPasta,
UdjinM6 and
knst
and removed request for
PastaPastaPasta and
knst
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/guiutil.cpp (1)
1475-1482: Non-mac Montserrat family may be invalid for Normal/Bold (trailing space)
When mapMontserratMapping returns "", the family becomes "Montserrat " (note trailing space), which can fail matching and cause fallback fonts. Set base family without suffix when empty.- font.setFamily(fontFamilyToString(FontFamily::Montserrat) + " " + mapMontserratMapping[qWeight]); - font.setWeight(qWeight); - font.setStyle(fItalic ? QFont::StyleItalic : QFont::StyleNormal); + const QString base_family = fontFamilyToString(FontFamily::Montserrat); + const QString suffix = mapMontserratMapping[qWeight]; + font.setFamily(suffix.isEmpty() ? base_family : (base_family + " " + suffix)); + font.setWeight(qWeight); + font.setStyle(fItalic ? QFont::StyleItalic : QFont::StyleNormal);Also applies to: 1501-1504
♻️ Duplicate comments (2)
src/net.h (1)
1492-1492: Rename to PascalCase and add [[nodiscard]] (avoid noexcept here).Align with CConnman’s accessor style (e.g., GetNodeCount). [[nodiscard]] is fine; noexcept is unsafe since returning a copied std::map may allocate and throw.
Apply this diff in this header:
- std::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() const; + [[nodiscard]] std::map<CNetAddr, LocalServiceInfo> GetNetLocalAddresses() const;Then rename the definition and call sites accordingly (examples from this PR: src/net.cpp, interfaces/node.h + src/node/interfaces.cpp, qt ClientModel/RPCConsole).
Run to locate remaining references:
#!/bin/bash rg -n 'getNetLocalAddresses\s*\(' -g 'src/**' rg -n 'GetNetLocalAddresses\s*\(' -g 'src/**'src/qt/guiutil.cpp (1)
622-628: Wayland bring-to-front: toggle hint bit instead of resetting all flagsResetting all window flags twice may recreate the native Wayland window and cause side effects/flicker. Toggle only the WindowStaysOnTopHint bit via setWindowFlag.
- if (QGuiApplication::platformName() == "wayland") { - auto flags = w->windowFlags(); - w->setWindowFlags(flags|Qt::WindowStaysOnTopHint); - w->show(); - w->setWindowFlags(flags); - w->show(); + if (QGuiApplication::platformName() == "wayland") { + const bool already_on_top = w->windowFlags().testFlag(Qt::WindowStaysOnTopHint); + if (!already_on_top) w->setWindowFlag(Qt::WindowStaysOnTopHint, true); + w->show(); + if (!already_on_top) w->setWindowFlag(Qt::WindowStaysOnTopHint, false); + w->show(); } else {Also applies to: 629-639
🧹 Nitpick comments (2)
src/qt/intro.h (1)
67-67: Add a brief comment to capture the flag’s lifecycle and intent.LGTM functionally. A one-liner here will prevent future regressions around auto-updating the prune checkbox after user interaction or explicit -prune usage.
- bool m_prune_checkbox_is_default{true}; + // UI gate: true while checkbox is still at its default; set false after user interaction or when -prune is provided to suppress auto-updates. + bool m_prune_checkbox_is_default{true};src/qt/guiutil.cpp (1)
144-148: Missing include (guarded block uses std::set)
This TU declares std::set but doesn’t include in the visible includes. If not transitively included elsewhere, some toolchains will fail.Add near other STL includes:
#include <set>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/interfaces/node.h(1 hunks)src/net.cpp(1 hunks)src/net.h(1 hunks)src/node/interfaces.cpp(2 hunks)src/qt/clientmodel.cpp(2 hunks)src/qt/clientmodel.h(3 hunks)src/qt/forms/debugwindow.ui(12 hunks)src/qt/guiutil.cpp(12 hunks)src/qt/intro.cpp(3 hunks)src/qt/intro.h(1 hunks)src/qt/optionsdialog.cpp(8 hunks)src/qt/overviewpage.cpp(1 hunks)src/qt/rpcconsole.cpp(3 hunks)src/qt/rpcconsole.h(1 hunks)src/qt/walletcontroller.cpp(3 hunks)src/qt/walletcontroller.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/interfaces/node.h
- src/qt/walletcontroller.h
- src/qt/walletcontroller.cpp
- src/qt/forms/debugwindow.ui
- src/qt/rpcconsole.cpp
- src/qt/clientmodel.h
- src/qt/clientmodel.cpp
- src/node/interfaces.cpp
- src/qt/rpcconsole.h
- src/qt/overviewpage.cpp
- src/net.cpp
- src/qt/optionsdialog.cpp
- src/qt/intro.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/intro.hsrc/net.hsrc/qt/guiutil.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/net.h (3)
src/netaddress.cpp (3)
CNetAddr(102-102)CNetAddr(292-297)CNetAddr(299-303)src/net.cpp (2)
getNetLocalAddresses(4452-4456)getNetLocalAddresses(4452-4452)src/qt/clientmodel.cpp (2)
getNetLocalAddresses(160-163)getNetLocalAddresses(160-160)
🔇 Additional comments (5)
src/qt/guiutil.cpp (5)
83-89: Macro switch to Q_OS_MACOS: confirm Qt baseline or add compatibility shim
Ensure all supported builders use a Qt version that defines Q_OS_MACOS (Qt 5.12+/Qt 6). If any target still uses older Qt where only Q_OS_MAC exists, consider a small compatibility shim.Add near includes (only if needed; optional):
// Compatibility shim if some builders still rely on Q_OS_MAC #ifndef Q_OS_MACOS # ifdef Q_OS_MAC # define Q_OS_MACOS 1 # endif #endifAlso applies to: 121-129, 144-148, 629-632, 666-668, 1318-1329, 1475-1482, 1486-1505, 1612-1620, 1624-1636, 1644-1648, 1856-1859
666-668: macOS config open fallback LGTM
QProcess::startDetached("/usr/bin/open", {"-t", ...}) behind Q_OS_MACOS is correct.
1318-1329: setApplicationFont macOS path LGTM
Logic aligns with upstream behavior and preserves user-adjusted normal weight.
1644-1648: Shortcut modifier on macOS: confirm intended UI convention
Using "Ctrl" on macOS is fine if the goal is Command (Qt may map standard shortcuts, but custom sequences from strings typically do not). Verify these appear as expected (Cmd+N) in menus/help.
1612-1620: macOS focus-rect and progress dialog tweaks LGTM
Guards and behaviors match known Qt/macOS quirks.Also applies to: 1624-1636, 1856-1859
9820d50 fix(qt): adjust css for SendCoinsEntry (UdjinM6) Pull request description: ## Issue being fixed or feature implemented d496df0 (#6829) changed `SendCoinsEntry` class from `QStackedWidget` to `QWidget` but we forgot to update `css` accordingly so GUI is a bit broken now. <img width="947" height="51" alt="Screenshot 2025-09-29 at 19 06 30" src="https://github.com/user-attachments/assets/ecb184db-82c2-4a0b-a37c-f4f00c4e9e33" /> ## What was done? Adjust css to match cpp ## How Has This Been Tested? Compile and check GUI, should look the same way it did before d496df0 <img width="928" height="49" alt="Screenshot 2025-09-29 at 19 07 25" src="https://github.com/user-attachments/assets/ad51c157-294f-4ef8-a3e4-97ab5f211226" /> ## Breaking Changes n/a ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg:  --- ACK 9820d50 Tree-SHA512: cd3db58be58fec533c6817a336cb6eddee93fbac9c57128c15f613260e2e318b0f2cd678f46f4ed74e6fc3b2394eb0292fe480c9c8742d5f54673dd4d90ca2d3
…en closing wallet" 826221b revert: Revert "merge bitcoin-core/gui#835: Fix crash when closing wallet" (UdjinM6) Pull request description: ## Issue being fixed or feature implemented This reverts commit 20763f1 from #6829. The issue that was meant to be fixed by that backport does not exist in our codebase yet, see bitcoin-core/gui#835 (review). Even worse, backporting it too early created an issue where wallets would not be removed from gui while being removed in background. ## What was done? revert it ## How Has This Been Tested? open multiple wallets, try closing one of them. develop: no visual changes, feels like "Close wallet" menu is not working but wallet is actually removed which can be confirmed by restarting qt this PR/previous behaviour: wallet is actually gone from ui asap ## Breaking Changes n/a ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK 826221b Tree-SHA512: ac50b9f3b7ceb0884a7e51aaf20f66753376569e42327331be8d6635d9b20d56034f6b2655406196e4882b1c4c3dcdbad6311b7b92786a7e51241c6255fdb785
4 participants
Additional Information
develop(e23a658)Breaking Changes
None expected.
Checklist