feat(qt): UI refresh (2/n, update "Governance" Tab layout, lifecycle icons, description dialogs, resumable proposal creation) #7110
Conversation
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe PR replaces the single ProposalWizard with two dialogs: ProposalCreate (build/submit proposals) and ProposalResume (broadcast pending wallet proposals). It adds Proposal and ProposalModel with richer fields/status/voting counts, extends GovernanceList to support Active vs Local sources and wallet-backed proposals, and updates node interfaces with governance query methods (yes/no/abstain counts, existence, funded height). Several Qt UI files, headers, moc files, wallet sources, and CSS were added/removed to wire the new flows. Sequence Diagram(s)sequenceDiagram
participant User
participant ProposalCreate
participant WalletModel
participant Node
participant Governance
User->>ProposalCreate: Open dialog / enter details
ProposalCreate->>Node: Load governance parameters
Node-->>ProposalCreate: Return superblock/heights and relay confs
ProposalCreate->>ProposalCreate: buildJsonAndHex() / validateFields()
User->>ProposalCreate: Confirm create
ProposalCreate->>WalletModel: Unlock / prepare
WalletModel-->>ProposalCreate: Wallet ready / tx prepared
ProposalCreate->>Governance: Submit proposal payload
Governance-->>ProposalCreate: Return proposal hash / status
ProposalCreate->>User: Show success / close
sequenceDiagram
participant User
participant GovernanceList
participant ProposalResume
participant WalletModel
participant Node
participant Governance
User->>GovernanceList: Open list / select "Local"
GovernanceList->>ProposalResume: Open resume dialog with proposals
ProposalResume->>Node: Query required confirmations
Node-->>ProposalResume: Return conf threshold
ProposalResume->>WalletModel: Poll collateral confirmations
WalletModel-->>ProposalResume: Return confirmation counts
alt confirmations met
ProposalResume->>User: Enable Broadcast button
User->>ProposalResume: Click Broadcast
ProposalResume->>Governance: Broadcast proposal
Governance-->>ProposalResume: Success/Error
ProposalResume->>GovernanceList: Remove/refresh entry
end
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
🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/qt/guiutil_font.cpp (1)
17-25: Run clang-format on this file (CI is failing).
The Clang Diff Format Check reports formatting differences in this file; please reformat before merge.src/interfaces/node.h (1)
20-26: Add missing<optional>include forstd::optional.
The file usesstd::optionalin the function signatures at lines 156–157 but doesn't include the required<optional>header, which will cause compilation failures.🛠️ Suggested fix
`#include` <functional> `#include` <memory> +#include <optional> `#include` <stddef.h> `#include` <stdint.h> `#include` <string>Also applies to: 156–157
src/qt/governancelist.cpp (1)
1-1: Address clang-format pipeline failure.The pipeline indicates clang-format differences in this file. Please run
clang-formatto fix formatting before merging.
🤖 Fix all issues with AI agents
In `@src/Makefile.qt.include`:
- Around line 34-36: The non-backported list is missing the new C++ proposal
sources: add entries for src/qt/proposaldesc.*, src/qt/proposaldescdialog.*, and
src/qt/proposalmodel.* to test/util/data/non-backported.txt (do not add any .ui
form files); note that src/qt/proposalwizard.* is already registered so no
change needed for it.
In `@src/node/interfaces.cpp`:
- Around line 310-316: The call to context().chainman->ActiveChain().Height()
inside getProposalFundedHeight is not protected by the cs_main lock; wrap the
ActiveChain().Height() access in LOCK(::cs_main) (as done in getNumBlocks() /
getLastBlockTime()) to prevent a race. Add a scoped LOCK(::cs_main) before
reading nTipHeight so the loop over context()->govman triggers uses a
thread-safe snapshot of the active chain height.
In `@src/qt/guiutil_font.h`:
- Around line 84-86: Run clang-format on src/qt/guiutil_font.h to fix CI
formatting failures; reformat the declarations/Getters (GetScaleSteps,
GetScaledFontSize, GetFont) and surrounding spacing/line breaks so they match
the project's clang-format rules, then stage the updated header (ensuring member
names m_scale_steps, m_font_scale, m_font remain unchanged) and push the
formatted file.
In `@src/qt/proposalmodel.cpp`:
- Around line 354-361: In ProposalModel::setVotingParams the dataChanged call
uses createIndex(rowCount(), Column::VOTING_STATUS) which can create an
out-of-range QModelIndex; fix by guarding for empty model and emitting
dataChanged only when rowCount() > 0, using createIndex(0,
Column::VOTING_STATUS) as the first index and createIndex(rowCount() - 1,
Column::VOTING_STATUS) as the last index so the range is valid; update the
conditional in setVotingParams to check rowCount() before calling Q_EMIT
dataChanged.
- Around line 364-366: The getProposalAt method can dereference m_data with an
invalid QModelIndex and crash; update ProposalModel::getProposalAt to validate
the index before accessing m_data by checking index.isValid() and that
index.row() is within the range 0 .. m_data.size()-1 (and optionally that
index.model() == this), and return nullptr for any invalid or out-of-range index
instead of indexing m_data directly.
- Around line 239-242: Replace the hard-coded English words with translatable
strings: use tr("Voting") and tr("Voted") for the status (from
proposal->status() / ProposalStatus::Voting) and use Qt's plural-aware tr() for
the block text by switching the message to a pluralized pattern such as tr("%1,
%n block(s) till superblock", "", blocks) (where blocks is from
proposal->blocksUntilSuperblock()) so translators can provide correct
singular/plural forms.
🧹 Nitpick comments (9)
src/qt/guiutil_font.cpp (1)
562-567: Preserve per‑widget base font size for registeredQTextEdits.
updateFonts()re-applies styling withDEFAULT_FONT_SIZEfor all registered text edits, which can override custom base sizes. If anyQTextEdituses a non-default base size, consider storing the base size at registration time and reusing it here.♻️ Possible approach
-std::map<QPointer<QTextEdit>, QString> mapTextEditStyleUpdates; +struct TextEditStyleUpdate { + QString html; + double base_point_size{0}; +}; +std::map<QPointer<QTextEdit>, TextEditStyleUpdate> mapTextEditStyleUpdates;- for (const auto& [widget, html] : mapTextEditStyleUpdates) { - setFontBodyHTML(widget, html, - getScaledFont(FontRegistry::DEFAULT_FONT_SIZE, /*bold=*/false), - getScaledFont(FontRegistry::DEFAULT_FONT_SIZE, /*bold=*/true)); + for (const auto& [widget, style] : mapTextEditStyleUpdates) { + const double base_size = style.base_point_size > 0 ? style.base_point_size + : FontRegistry::DEFAULT_FONT_SIZE; + setFontBodyHTML(widget, style.html, + getScaledFont(base_size, /*bold=*/false), + getScaledFont(base_size, /*bold=*/true)); }- mapTextEditStyleUpdates[widget] = html; + mapTextEditStyleUpdates[widget] = {html, widget->font().pointSizeF()};src/qt/proposaldescdialog.cpp (2)
5-13: Consider reordering includes for consistency.The include order could be improved for consistency with typical project conventions (own header first, then related headers, then Qt headers):
♻️ Suggested reordering
-#include <qt/forms/ui_proposaldescdialog.h> - -#include <qt/proposaldesc.h> `#include` <qt/proposaldescdialog.h> + +#include <qt/forms/ui_proposaldescdialog.h> +#include <qt/proposaldesc.h> `#include` <qt/proposalmodel.h> -#include <qt/guiutil_font.h> - `#include` <qt/guiutil.h> +#include <qt/guiutil_font.h>Also,
guiutil_font.happears unused since no direct font functions from that header are called in this file.
15-24: Consider adding a null check for the proposal pointer.The constructor dereferences
proposalwithout validating it. While callers may guarantee non-null, defensive programming would prevent potential crashes.🛡️ Suggested defensive check
ProposalDescDialog::ProposalDescDialog(const Proposal* proposal, BitcoinUnit unit, QWidget* parent) : QDialog(parent, GUIUtil::dialog_flags), ui(new Ui::ProposalDescDialog) { + assert(proposal != nullptr); ui->setupUi(this); setWindowTitle(tr("Details for %1").arg(proposal->title())); GUIUtil::registerWidget(ui->detailText, ProposalDesc::toHTML(proposal, unit)); GUIUtil::updateFonts(); GUIUtil::handleCloseWindowShortcut(this); }Based on learnings, fail-fast asserts are preferred for programmer errors that should be caught during development.
src/qt/proposaldesc.cpp (2)
14-34: Consider HTML-escaping hash values for consistency.While hash values are typically safe hex strings, applying
HtmlEscapetohash(),parentHash(), andcollateralHash()would provide defense-in-depth and maintain consistency with other fields.♻️ Suggested change
- strHTML += "<b>" + tr("Object Hash") + ":</b> " + proposal->hash() + "<br>"; - strHTML += "<b>" + tr("Parent Hash") + ":</b> " + proposal->parentHash() + "<br>"; + strHTML += "<b>" + tr("Object Hash") + ":</b> " + GUIUtil::HtmlEscape(proposal->hash()) + "<br>"; + strHTML += "<b>" + tr("Parent Hash") + ":</b> " + GUIUtil::HtmlEscape(proposal->parentHash()) + "<br>"; strHTML += "<b>" + tr("Collateral Date") + ":</b> " + GUIUtil::dateTimeStr(proposal->collateralDate()) + "<br>"; - strHTML += "<b>" + tr("Collateral Hash") + ":</b> " + proposal->collateralHash() + "<br>"; + strHTML += "<b>" + tr("Collateral Hash") + ":</b> " + GUIUtil::HtmlEscape(proposal->collateralHash()) + "<br>";
18-18: Minor: HTML structure could be more complete.The HTML starts with
<html>but lacks<body>tags. While Qt's QTextEdit typically handles incomplete HTML gracefully, a proper structure would be more robust.src/qt/forms/governancelist.ui (1)
47-67: Add a buddy to the filter label for keyboard focus.This improves keyboard navigation (Alt+key focuses the filter input).
♿ Suggested UI tweak
<widget class="QLabel" name="label_filter_2"> <property name="text"> <string>Filter List:</string> </property> + <property name="buddy"> + <cstring>filterLineEdit</cstring> + </property> </widget>src/qt/governancelist.h (1)
33-36: Include<cstdint>foruint8_t.Avoid relying on transitive includes for fixed-width integer types.
♻️ Proposed include
`#include` <QWidget> +#include <cstdint> + `#include` <map> `#include` <memory>src/qt/bitcoingui.cpp (1)
1470-1500: Hardcoded toolbar action indices are fragile.Lines 1478 and 1494 use
appToolBar->actions()[5]andappToolBar->actions()[6]respectively. This mirrors the existing pattern at line 1460 for CoinJoin (actions()[4]), but hardcoded indices are error-prone — if buttons are reordered or added, these will silently break.Consider storing action references when buttons are added, or adding a comment documenting the expected order. This is a maintenance concern rather than a blocking issue since it follows the existing pattern.
src/qt/proposalmodel.h (1)
27-31: Q_OBJECT macro placement is unconventional.The
Q_OBJECTmacro is placed in aprivate:section. While this compiles, the Qt convention is to place it at the very beginning of the class body (before any access specifier). This is a minor style nit.Suggested adjustment
class Proposal : public QObject { -private: Q_OBJECT +private: ClientModel* clientModel;
| double GetScaleSteps() const { return m_scale_steps; } | ||
| double GetScaledFontSize(int size) const { return std::round(size * (1 + (m_font_scale * m_scale_steps)) * 4) / 4.0; } | ||
| double GetScaledFontSize(double size) const { return std::round(size * (1 + (m_font_scale * m_scale_steps)) * 4) / 4.0; } | ||
| QString GetFont() const { return m_font; } |
There was a problem hiding this comment.
Run clang-format on this header (CI is failing).
The Clang Diff Format Check reports formatting differences here; please reformat before merge.
🤖 Prompt for AI Agents
In `@src/qt/guiutil_font.h` around lines 84 - 86, Run clang-format on
src/qt/guiutil_font.h to fix CI formatting failures; reformat the
declarations/Getters (GetScaleSteps, GetScaledFontSize, GetFont) and surrounding
spacing/line breaks so they match the project's clang-format rules, then stage
the updated header (ensuring member names m_scale_steps, m_font_scale, m_font
remain unchanged) and push the formatted file.
kwvg
force-pushed
the
gov_refresh
branch
3 times, most recently
from
fc8a95a to
4490d40
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/qt/proposalmodel.cpp`:
- Around line 30-51: The Proposal constructor leaves m_paymentAmount
uninitialized if JSON parsing fails or "payment_amount" is absent; initialize
m_paymentAmount to a safe default (e.g., 0.0) either in the Proposal member
initializer list or immediately before parsing in the
Proposal::Proposal(ClientModel*, const CGovernanceObject&, QObject*) constructor
so that m_paymentAmount always has a defined value for display/sorting; update
the constructor to set m_paymentAmount = 0.0 (or another appropriate default)
and keep the existing assignment from prop_data when payment_amount exists.
In `@src/qt/proposalmodel.h`:
- Around line 71-83: Add a destructor for the ProposalModel class to free owned
Proposal pointers in m_data: implement ProposalModel::~ProposalModel() to
iterate m_data, delete each Proposal*, and clear the list; this mirrors
ownership semantics already used in remove() and reconcile() and prevents leaks
by releasing remaining proposals when the model is destroyed.
In `@src/qt/proposalresume.cpp`:
- Around line 135-141: The broadcast button lambdas capture a stale numeric
index (size_t index = m_entries.size()) causing wrong or out-of-bounds lookups
when entries are removed; change the lambda to find the clicked button's current
position by searching m_entries for the sender button pointer
(entry.broadcast_btn) at runtime and call onBroadcast with that computed index,
and add `#include` <algorithm> at the top for std::find support; update the
connect call to use the button pointer via the lambda capture (e.g., capture the
QPushButton* or use sender()) and compute index =
std::distance(m_entries.begin(), std::find(...)) before calling onBroadcast.
♻️ Duplicate comments (5)
src/node/interfaces.cpp (2)
317-331: GuardActiveChain().Height()access withcs_mainlock.As previously noted,
context().chainman->ActiveChain().Height()requires thecs_mainlock for thread safety. This is consistent with other methods in this file that access the active chain (e.g.,getNumBlocks(),getLastBlockTime()).🔒 Suggested fix
std::optional<int32_t> getProposalFundedHeight(const uint256& proposal_hash) override { if (context().govman != nullptr && context().chainman != nullptr && context().dmnman != nullptr) { - const int32_t nTipHeight = context().chainman->ActiveChain().Height(); + int32_t nTipHeight; + { + LOCK(::cs_main); + nTipHeight = context().chainman->ActiveChain().Height(); + } for (const auto& trigger : context().govman->GetActiveTriggers()) {
301-302: GuardActiveHeight()access withcs_mainlock.
ctx.chainman->ActiveHeight()reads chain state that can change concurrently. This should be protected bycs_mainfor thread safety, consistent with other chain height accesses in this file (e.g.,getNumBlocks(),getLastBlockTime()).Note: Lines 309-314 have a similar pre-existing issue where
GetListAtChainTip()andActiveChain()are accessed outside a lock. Consider wrapping the entire block that accesses chain state.🔒 Suggested fix
if (ctx.chainman) { + LOCK(::cs_main); CSuperblock::GetNearestSuperblocksHeights(ctx.chainman->ActiveHeight(), info.lastsuperblock, info.nextsuperblock); }Or consolidate locking for all chain-dependent reads in this method.
src/qt/proposalmodel.cpp (2)
356-362: GuarddataChangedwhen the model is empty.
rowCount() - 1can be-1, producing an invalid QModelIndex range.🐛 Proposed fix
- Q_EMIT dataChanged(createIndex(0, Column::VOTING_STATUS), createIndex(rowCount() - 1, Column::VOTING_STATUS)); + if (rowCount() > 0) { + Q_EMIT dataChanged(createIndex(0, Column::VOTING_STATUS), + createIndex(rowCount() - 1, Column::VOTING_STATUS)); + }
366-370: Bounds check ingetProposalAtis off by one.
std::clamp(..., 0, m_data.count())allowsrow == count, which can access past the end.🐛 Proposed fix
- if (index.isValid() && index.row() == std::clamp(index.row(), 0, m_data.count())) { - return m_data[index.row()]; - } - return nullptr; + if (!index.isValid() || index.row() < 0 || index.row() >= m_data.count()) return nullptr; + return m_data[index.row()];src/Makefile.qt.include (1)
164-167: Verify non-backported.txt includes new proposal C++ files.Per previous review feedback, the following new C++ files should be added to
test/util/data/non-backported.txt:
src/qt/descriptiondialog.cppandsrc/qt/descriptiondialog.hsrc/qt/proposaldesc.cppandsrc/qt/proposaldesc.hsrc/qt/proposalmodel.cppandsrc/qt/proposalmodel.hsrc/qt/proposalresume.cppandsrc/qt/proposalresume.hNote:
src/qt/proposalwizard.*is already registered, and.uifiles should not be added per project guidelines.#!/bin/bash # Verify non-backported.txt includes the new proposal C++ files echo "=== Current entries in non-backported.txt ===" cat test/util/data/non-backported.txt | grep -E "(descriptiondialog|proposaldesc|proposalmodel|proposalresume)" || echo "No matching entries found" echo "" echo "=== Expected new entries ===" echo "src/qt/descriptiondialog.cpp" echo "src/qt/descriptiondialog.h" echo "src/qt/proposaldesc.cpp" echo "src/qt/proposaldesc.h" echo "src/qt/proposalmodel.cpp" echo "src/qt/proposalmodel.h" echo "src/qt/proposalresume.cpp" echo "src/qt/proposalresume.h"Also applies to: 268-279
🧹 Nitpick comments (6)
src/qt/proposalmodel.h (1)
42-49: Assert non‑null ClientModel inProposalconstruction.Most methods dereference
clientModel, so a fail‑fast assert makes the invariant explicit.♻️ Suggested assert
+#include <cassert> ... Proposal::Proposal(ClientModel* _clientModel, const CGovernanceObject& _govObj, QObject* parent) : QObject(parent), clientModel(_clientModel), govObj(_govObj) { + assert(clientModel); ... }Based on learnings, prefer a fail‑fast assert to enforce this invariant.
src/qt/descriptiondialog.cpp (1)
6-11: Consider reordering includes for consistency.The typical convention is to include the corresponding header file first (descriptiondialog.h), followed by other project headers, then third-party headers. Currently, the UI header is included first.
♻️ Suggested include order
-#include <qt/forms/ui_descriptiondialog.h> - `#include` <qt/descriptiondialog.h> + +#include <qt/forms/ui_descriptiondialog.h> `#include` <qt/guiutil_font.h> - `#include` <qt/guiutil.h>src/qt/optionsmodel.h (1)
163-164: Consider adding default initializers for safety.The new member variables
m_enable_masternodesandm_enable_governancelack default initializers. WhileInit()sets these values from QSettings before they're used, adding default initializers (e.g.,{false}) provides defense-in-depth against potential future code paths that might access these before initialization.Suggested change
- bool m_enable_masternodes; - bool m_enable_governance; + bool m_enable_masternodes{false}; + bool m_enable_governance{false};src/qt/bitcoingui.cpp (1)
1470-1499: Refactor to eliminate hard-coded toolbar action indices.The visibility methods use hard-coded indices to access toolbar actions:
- Line 1460:
appToolBar->actions()[4]for CoinJoin- Line 1478:
appToolBar->actions()[5]for Masternodes- Line 1494:
appToolBar->actions()[6]for GovernanceThis pattern is fragile. Store action pointers when buttons are added to the toolbar, or use a lookup method (e.g.,
QToolBar::widgetForAction()) instead of relying on index positions. If the toolbar layout changes, these indices will silently reference wrong actions.src/qt/guiutil_font.cpp (1)
212-243: Character-by-character iteration may be slow for large HTML documents.The
setFontBodyHTMLfunction iterates one character at a time through the entire document to apply formatting. For large documents, this could cause noticeable UI lag.Consider using
QTextBlockiteration combined withQTextBlock::iteratorfor fragment-level processing, which would be more efficient:QTextBlock block = widget->document()->begin(); while (block.isValid()) { for (auto it = block.begin(); !it.atEnd(); ++it) { QTextFragment fragment = it.fragment(); // Process fragment... } block = block.next(); }That said, for typical proposal descriptions this is likely acceptable.
src/qt/proposalwizard.cpp (1)
206-212: Consider more specific error handling for creation failures.The error handling shows a generic error message from
prepareProposal. It might be helpful to distinguish between different failure modes (e.g., insufficient funds, network issues, validation failures) for better user guidance.However, this is a minor UX improvement and the current implementation is functional.
kwvg
changed the title
QTextEdit styling, governance layout refresh)
kwvg
force-pushed
the
gov_refresh
branch
6 times, most recently
from
b135de6 to
3c01a6f
Compare
|
This pull request has conflicts, please rebase. |
…ernance" and "Masternode" tabs, split out proposal model, additional bounds checking, better QTextEdit styling) f9f7c6e refactor: repurpose `transactiondescdialog` as a generic desc. container (Kittywhiskers Van Gogh) 5f4a6c0 refactor: track "Masternodes" tab show/hide in `OptionsModel` (Kittywhiskers Van Gogh) 1994f8c qt: use show/hide for "Masternodes" tab to avoid client restart (Kittywhiskers Van Gogh) 6a7e6b0 refactor: track "Governance" tab show/hide in `OptionsModel` (Kittywhiskers Van Gogh) 64a2db6 qt: use show/hide for "Governance" tab to avoid client restart (Kittywhiskers Van Gogh) 762e5d3 qt: add styling support for QTextEdit `<b>` and `<h{1-3}>` elements (Kittywhiskers Van Gogh) 7a2f09d refactor: drop `QObject` inheritance, precalculate hash string (Kittywhiskers Van Gogh) fdd3193 fix: use `unique_ptr` and more bounds checking in `qt/proposalmodel` (Kittywhiskers Van Gogh) c9e0d94 move-only: split proposal model out to `src/qt/proposalmodel.{cpp,h}` (Kittywhiskers Van Gogh) 42f2345 refactor: add `pruneStaleEntities` helper to deduplicate pruning (Kittywhiskers Van Gogh) 72c4099 refactor: add `getScaledFont` helper, keep `getFont` internal (Kittywhiskers Van Gogh) 6d9b90b chore: enforce alphabetical sorting in `src/Makefile.qt.include` (Kittywhiskers Van Gogh) 83b9271 move-only: move `subFeeFromAmount` into `verticalLayout_2` (Kittywhiskers Van Gogh) bcfc0e3 refactor: improve CoinJoin tab nullptr checks, bail-out condition (Kittywhiskers Van Gogh) 7648792 refactor: drop `emitCoinJoinEnabledChanged()`, use `setOption()` instead (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #7110 * Dependency for #7116 | `develop` (cc50446) | This PR (18d6ce6) | | ----------------------- | ------------------- | |  |  | |  |  | |  |  | ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK f9f7c6e Tree-SHA512: 6c219c8bc5b90c52bb030a6f05bcf9168ec51be0d72c23b38b5aba852666419949ebe4ee07c2c8ba70241655a542de24e2e31b6b0241517e183b7bf289373a37
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Review with `git log -p -n1 --ignore-space-change --color-moved=dimmed_zebra`.
This is closer to how we report details for transactions and more translation-friendly and easier to copy.
Visiting URLs could be dangerous, add a warning before we open a link in someone's browser.
Dash Governance Tools' budget generator is a much more visually friendly design and we can model our wizard after it.
The JSON and payload hex means little for the vast majority of users, what matters is the error messages, so we responsively validate contents and drop an extra panel in the process.
The visual language should match other coin-spending activities, also warn that that you don't get the fee back.
The proposal creation process can be interrupted for reasons beyond our control, since submitting the actual proposal is different from spending the collateral, it should have its own page. Right now this removes the old code but doesn't create a new layout, we will do that in the next commit.
This replaces the remaining pages of the proposal wizard that we removed in the last commit.
You cannot create a proposal without an adequate balance, it's a bad idea to allow any kind of proposal manipulation without being synced.
We are no longer using the wizard format so the name is a misnomer, the
logic is also relatively simplified (as now half of it is in
`qt/proposalresume.{cpp,h}`)
kwvg
requested review from
PastaPastaPasta,
UdjinM6 and
knst
and removed request for
UdjinM6
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/qt/governancelist.cpp`:
- Around line 326-344: In GovernanceList::showAdditionalInfo, guard the call to
clientModel->getOptionsModel()->getDisplayUnit() by checking
clientModel->getOptionsModel() for nullptr (use the same null-check/ternary
pattern used in updateProposalButtons) before passing the display unit to
proposal->toHtml; if getOptionsModel() is null, pass a safe default (e.g.,
DEFAULT_UNIT or 0) so DescriptionDialog construction uses a valid display unit.
Ensure you update the reference in this function only (around the
DescriptionDialog call) and keep all other behavior the same.
🧹 Nitpick comments (3)
src/qt/proposalcreate.cpp (1)
30-33: Redundantstaticon function in anonymous namespace.Functions inside an anonymous namespace already have internal linkage; the
statickeyword is redundant.Suggested fix
namespace { -// Minimal helper to encode plain ASCII JSON to hex for RPC -static QString toHex(const QByteArray& bytes) { return QString(bytes.toHex()); } +// Minimal helper to encode plain ASCII JSON to hex for RPC +QString toHex(const QByteArray& bytes) { return QString(bytes.toHex()); } } // namespacesrc/qt/governancelist.cpp (1)
137-181:getWalletProposalsfilter logic is correct but could benefit from a comment.Line 175:
pending != (is_broadcast || is_stale)is a compact boolean expression that acts as a bidirectional filter —pending=trueselects unbroadcast/non-stale proposals (for Resume), whilepending=falseselects broadcast or stale proposals (for My Proposals). The logic is correct but the expression reads as a double negative. Consider a clarifying comment.Optional readability improvement
+ // When pending=true, select proposals not yet broadcast and not stale (resumable). + // When pending=false, select proposals already broadcast or stale (for "My Proposals"). if (pending != (is_broadcast || is_stale)) { result.push_back(obj); }src/qt/proposalmodel.cpp (1)
108-113:votingProgressdivision by zero ifsuperblockcycleis 0.If
params.superblockcyclewere ever 0, Line 112 would produce a division-by-zero (or inf/NaN). This is practically impossible with valid consensus parameters, but the result flows intostd::clamp<int>(proposal->votingProgress() * 6, 0, 5)at Line 304, where NaN behavior inclampis undefined in C++. A one-line guard would make this robust.Optional defensive guard
double Proposal::votingProgress() const { const auto params = clientModel->node().gov().getGovernanceInfo(); const int blocksRemaining = params.nextsuperblock - clientModel->getNumBlocks(); + if (params.superblockcycle <= 0) return 0.0; return 1.0 - (static_cast<double>(blocksRemaining) / static_cast<double>(params.superblockcycle)); }
| void GovernanceList::showAdditionalInfo(const QModelIndex& index) | ||
| { | ||
| if (!index.isValid()) { | ||
| if (!index.isValid() || !clientModel) { | ||
| return; | ||
| } | ||
|
|
||
| const auto proposal = proposalModel->getProposalAt(proposalModelProxy->mapToSource(index)); | ||
| if (proposal == nullptr) { | ||
| if (!proposal) { | ||
| return; | ||
| } | ||
|
|
||
| const auto windowTitle = tr("Proposal Info: %1").arg(proposal->title()); | ||
| const auto json = proposal->toJson(); | ||
|
|
||
| QMessageBox::information(this, windowTitle, json); | ||
| DescriptionDialog* dialog = new DescriptionDialog( | ||
| tr("Details for %1").arg(proposal->title()), | ||
| proposal->toHtml(clientModel->getOptionsModel()->getDisplayUnit()), | ||
| /*parent=*/this); | ||
| dialog->resize(800, 380); | ||
| dialog->setAttribute(Qt::WA_DeleteOnClose); | ||
| dialog->show(); | ||
| } |
There was a problem hiding this comment.
Missing null check on getOptionsModel() in showAdditionalInfo.
Line 339 calls clientModel->getOptionsModel()->getDisplayUnit() without guarding getOptionsModel(). The same pattern was flagged and fixed in updateProposalButtons (Lines 397-398) with a null-check ternary. Apply the same guard here for consistency.
Suggested fix
- DescriptionDialog* dialog = new DescriptionDialog(
- tr("Details for %1").arg(proposal->title()),
- proposal->toHtml(clientModel->getOptionsModel()->getDisplayUnit()),
- /*parent=*/this);
+ const auto unit = clientModel->getOptionsModel() ? clientModel->getOptionsModel()->getDisplayUnit()
+ : BitcoinUnit::DASH;
+ DescriptionDialog* dialog = new DescriptionDialog(
+ tr("Details for %1").arg(proposal->title()),
+ proposal->toHtml(unit),
+ /*parent=*/this);🤖 Prompt for AI Agents
In `@src/qt/governancelist.cpp` around lines 326 - 344, In
GovernanceList::showAdditionalInfo, guard the call to
clientModel->getOptionsModel()->getDisplayUnit() by checking
clientModel->getOptionsModel() for nullptr (use the same null-check/ternary
pattern used in updateProposalButtons) before passing the display unit to
proposal->toHtml; if getOptionsModel() is null, pass a safe default (e.g.,
DEFAULT_UNIT or 0) so DescriptionDialog construction uses a valid display unit.
Ensure you update the reference in this function only (around the
DescriptionDialog call) and keep all other behavior the same.
1 participant
Additional Information
Proposal Creation
Breaking Changes
None expected.
Checklist