refactor(qt): UI refresh (1/n, runtime show/hide of "Governance" and "Masternode" tabs, split out proposal model, additional bounds checking, better QTextEdit styling) #7112
Conversation
Also `s/coinJoinEnabledChanged/showCoinJoinChanged/g` to establish a naming convention that will be used soon.
This should fix the misalignment of the `subFeeFromAmount` `QCheckBox` compared to the rest of the `QCheckBox`es Review with `git log -p -n1 --ignore-space-change --color-moved=dimmed_zebra`.
Review with `git log -p -n1 --color-moved=dimmed_zebra`.
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a Qt governance proposal model and UI wiring: introduces Proposal and ProposalModel (ProposalList) to parse and present CGovernanceObject data as a QAbstractTableModel and integrates them into the build. Changes tab visibility handling: OptionsModel now stores showMasternodes/showGovernance flags and emits showMasternodesChanged/showGovernanceChanged/showCoinJoinChanged; BitcoinGUI and WalletView create mastenode/governance pages unconditionally and use new updateGovernanceVisibility/updateMasternodesVisibility slots to toggle visibility. Font utilities gain getScaledFont/getFontBold and per-QTextEdit HTML registration. Renames coinJoinEnabledChanged → showCoinJoinChanged and updates related connections. Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/guiutil_font.h (1)
1-178: Pipeline failure: clang-format differences detected.The CI reports formatting issues in this file. Run
clang-formator the project's diff tool to fix formatting before merging.
🤖 Fix all issues with AI agents
In `@src/qt/governancelist.cpp`:
- Around line 186-188: The context menu action currently captures a raw
Proposal* (proposal) which can dangle if reconcile() modifies m_data before the
user clicks; change the lambda to capture a stable identifier (e.g.,
proposal->getHash() or id) instead of the pointer and, inside the action, look
up the Proposal from m_data (or the container that owns proposals) and only call
openUrl() if the lookup returns a valid pointer; alternatively, ensure
proposalContextMenu is disabled/cleared during reconcile() updates to avoid the
race. Reference: proposalContextMenu, proposal_url, Proposal, reconcile(),
m_data.
In `@src/qt/guiutil_font.cpp`:
- Around line 619-625: getScaledFont currently calls
g_font_registry.GetScaledFontSize(...) before calling getFont, causing the size
to be scaled twice; change getScaledFont to pass the unscaled size (baseSize *
multiplier) into getFont and let getFont (which calls
g_font_registry.GetScaledFontSize internally) perform the actual scaling. Locate
getScaledFont and replace the g_font_registry.GetScaledFontSize(...) argument
with the raw baseSize * multiplier while keeping the weight selection logic the
same.
🧹 Nitpick comments (5)
src/qt/proposalmodel.h (2)
20-48: Good design for the Proposal wrapper class.The class cleanly encapsulates governance object data with cached QString fields. A few observations:
The
clientModelraw pointer is acceptable here sinceProposaldoesn't own it, but methods likeisActive(),GetAbsoluteYesCount(), andvotingStatus()dereference it without null checks. If aProposalis ever used afterclientModelbecomes invalid, this could cause a crash.Consider adding a brief comment documenting the lifetime requirement (e.g.,
clientModelmust outlive theProposal).
89-91: Minor: Redundant empty check inisValidRow.The
!m_data.empty()check is redundant sincerow >= 0 && row < rowCount()already handles the empty case (whenm_datais empty,rowCount()returns 0, so anyrow >= 0will fail therow < rowCount()check).Simplified version
- bool isValidRow(int row) const { return !m_data.empty() && row >= 0 && row < rowCount(); } + bool isValidRow(int row) const { return row >= 0 && row < rowCount(); }src/qt/proposalmodel.cpp (3)
28-32: Early return leaves Proposal in partially initialized state.If
prop_data.read()fails, the constructor returns early withm_hashset but all other fields (m_title,m_startDate,m_endDate,m_paymentAmount,m_url) at their default values. This could result in proposals being displayed with a hash but empty title and dates.Consider either:
- Throwing an exception or using a factory that returns
std::optional<Proposal>- Adding an
isValid()method to check if parsing succeeded- Documenting this behavior is intentional for malformed governance objects
95-103: Unused parameter warnings.The
indexparameter is unused inrowCount()andcolumnCount(), which is expected for flat (non-hierarchical) models. Consider usingQ_UNUSEDto suppress compiler warnings.Suppress unused parameter warnings
int ProposalModel::rowCount(const QModelIndex& index) const { + Q_UNUSED(index) return static_cast<int>(m_data.size()); } int ProposalModel::columnCount(const QModelIndex& index) const { + Q_UNUSED(index) return Column::_COUNT; }
136-164: Unreachable code:breakstatements after return.Lines 138 and 161 contain
breakstatements that are unreachable because the inner switch cases either return or fall through todefaultwhich returns. The outer switch at line 163 also has unreachable code paths.Remove unreachable breaks
default: return {}; - }; - break; + } } case Qt::EditRole: { // Edit role is used for sorting, so return the raw values where possible switch (index.column()) { ... default: return {}; - }; - break; + } } - }; + } return {};
| proposalContextMenu->clear(); | ||
| proposalContextMenu->addAction(proposal_url, proposal, &Proposal::openUrl); | ||
| proposalContextMenu->addAction(proposal_url, [proposal]() { proposal->openUrl(); }); | ||
|
|
There was a problem hiding this comment.
Potential dangling pointer in context menu lambda.
The lambda [proposal]() { proposal->openUrl(); } captures a raw pointer to a Proposal owned by m_data. If reconcile() is called between when the context menu is displayed and when the user clicks the action (removing this proposal), the captured pointer would dangle.
This is a low-probability race condition since:
- The update timer interval is typically several seconds
- The proposal would need to be removed in that exact window
Consider capturing by hash and looking up the proposal when the action is triggered, or disabling the menu during updates.
Safer alternative using hash lookup
// right click menu with option to open proposal url
- QString proposal_url = proposal->url();
+ const QString proposal_url = proposal->url();
+ const QString proposal_hash = proposal->hash();
proposal_url.replace(QChar('&'), QString("&&"));
proposalContextMenu->clear();
- proposalContextMenu->addAction(proposal_url, [proposal]() { proposal->openUrl(); });
+ proposalContextMenu->addAction(proposal_url, [this, proposal_hash]() {
+ // Re-lookup proposal to avoid dangling pointer if reconcile() ran
+ for (int row = 0; row < proposalModel->rowCount(); ++row) {
+ if (auto* p = proposalModel->getProposalAt(proposalModel->index(row, 0));
+ p && p->hash() == proposal_hash) {
+ p->openUrl();
+ return;
+ }
+ }
+ });🤖 Prompt for AI Agents
In `@src/qt/governancelist.cpp` around lines 186 - 188, The context menu action
currently captures a raw Proposal* (proposal) which can dangle if reconcile()
modifies m_data before the user clicks; change the lambda to capture a stable
identifier (e.g., proposal->getHash() or id) instead of the pointer and, inside
the action, look up the Proposal from m_data (or the container that owns
proposals) and only call openUrl() if the lookup returns a valid pointer;
alternatively, ensure proposalContextMenu is disabled/cleared during reconcile()
updates to avoid the race. Reference: proposalContextMenu, proposal_url,
Proposal, reconcile(), m_data.
Review with `git log -p -n1 --color-moved=dimmed_zebra`.
Inheriting the whole structure is overkill just to use `tr` and `hash()` is used in hot loops so we should cache the value in the ctor. Also remove dead variable.
1 participant
Additional Information
develop(cc50446)Breaking Changes
None expected.
Checklist