Repository navigation
feat(qt): add shift-click range selection to coin control dialog - #7260
PastaPastaPasta merged 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 025cc4871f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std::vector<QTreeWidgetItem*> leafItems; | ||
| int anchorIdx = -1; | ||
| int targetIdx = -1; | ||
| QTreeWidgetItemIterator it(this); |
There was a problem hiding this comment.
The range-building logic starts from QTreeWidgetItemIterator it(this), which walks the full tree structure in pre-order rather than the currently visible rows, so in tree mode it will include children of collapsed address groups. A shift-click from one visible leaf to another can therefore silently toggle hidden UTXOs in collapsed groups, causing unintended coin selection and potentially spending coins the user never saw selected.
Useful? React with 👍 / 👎.
| setEnabled(false); | ||
| for (int i = anchorIdx; i <= targetIdx; ++i) { | ||
| QTreeWidgetItem* item = leafItems[i]; | ||
| if (!item->isDisabled() && item->checkState(COLUMN_CHECKBOX) != m_lastClickedState) { |
There was a problem hiding this comment.
Range application uses the cached m_lastClickedState, but that cache is only refreshed on normal leaf clicks; if the anchor item's checkbox changes through other flows (for example Select All / parent tristate propagation / lock-related state changes), shift-click will apply an outdated state that disagrees with what the anchor currently shows. This makes range selection invert unexpectedly after bulk actions.
Useful? React with 👍 / 👎.
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Shift+click range selection to CoinControlTreeWidget and a dialog-level label refresh entrypoint. The tree widget tracks an anchor item (m_lastClickedItem), overrides mouseReleaseEvent to detect Shift+Left releases on leaf (UTXO) rows, builds the visible ordered leaf list (skipping children of collapsed parents), applies a batched checkbox state to the inclusive range while temporarily disabling updates, and calls CoinControlDialog::refreshLabels() once. CoinControlDialog gains refreshLabels() which delegates to the existing static updateLabels(...). A resetAnchor() method clears the stored anchor when repopulating the view. Sequence DiagramsequenceDiagram
actor User
participant TreeWidget as CoinControlTreeWidget
participant Dialog as CoinControlDialog
participant Qt as Qt Framework
User->>TreeWidget: Mouse press
TreeWidget->>Qt: QTreeWidget handles press
User->>TreeWidget: Mouse release (Shift+Left)
TreeWidget->>TreeWidget: mouseReleaseEvent() — detect Shift+Left and anchor
alt Shift-click range
TreeWidget->>TreeWidget: Build visible ordered leaf list\n(skip collapsed parents)
TreeWidget->>TreeWidget: Compute anchor and target indices
TreeWidget->>TreeWidget: setUpdatesEnabled(false)
loop Apply checkbox state across range
TreeWidget->>TreeWidget: setCheckState(item)
end
TreeWidget->>TreeWidget: setUpdatesEnabled(true)
TreeWidget->>Dialog: refreshLabels()
Dialog->>Dialog: updateLabels(m_coin_control, model, this)
else Single click
TreeWidget->>Qt: Default QTreeWidget behavior
TreeWidget->>TreeWidget: Update m_lastClickedItem
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/coincontroltreewidget.cpp`:
- Around line 76-90: The anchor capture is too broad and the cached
m_lastClickedState can be stale; update the mouseReleaseEvent logic so the
non-shift branch only arms/records m_lastClickedItem and m_lastClickedState for
enabled left-button clicks (i.e., check event->button() == Qt::LeftButton and
that the clicked leaf item is enabled/unlocked via isLeafItem(...) and
item->isEnabled()/not locked), and remove/avoid recording on right-clicks or
disabled items; when handling the shift-click range (isShiftClick) do not rely
on the cached m_lastClickedState—read the current checkState(COLUMN_CHECKBOX)
from m_lastClickedItem at the moment of applying the range (instead of using
m_lastClickedState) so external itemChanged handlers (e.g., in
CoinControlDialog) can't cause stale toggles. Ensure the isShiftClick condition
includes the enabled-state check for both clickedItem and m_lastClickedItem.
- Around line 96-107: The current collection of leaf items uses
QTreeWidgetItemIterator which iterates the full tree (including collapsed
children); replace that traversal with a visual-order walk using
itemBelow()/itemAbove() so only visible items are considered. Specifically,
instead of QTreeWidgetItemIterator, start from the first visible item (e.g.
topmost visible returned by itemAt(0,0) or the first top-level visible) and
iterate with itemBelow(current) until null, calling isLeafItem(...) and pushing
into leafItems while setting anchorIdx (for m_lastClickedItem) and targetIdx
(for clickedItem) the same way; this ensures shift-click range selection toggles
only visible leaves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5494330b-68e2-4b6a-a05d-f4fa4c289445
📥 Commits
Reviewing files that changed from the base of the PR and between ac94680 and 025cc4871f75e00b08683d8ad490f3803aeb799b.
📒 Files selected for processing (4)
src/qt/coincontroldialog.cppsrc/qt/coincontroldialog.hsrc/qt/coincontroltreewidget.cppsrc/qt/coincontroltreewidget.h
| // Collect all leaf items in display order | ||
| std::vector<QTreeWidgetItem*> leafItems; | ||
| int anchorIdx = -1; | ||
| int targetIdx = -1; | ||
| QTreeWidgetItemIterator it(this); | ||
| while (*it) { | ||
| if (isLeafItem(*it)) { | ||
| if (*it == m_lastClickedItem) anchorIdx = leafItems.size(); | ||
| if (*it == clickedItem) targetIdx = leafItems.size(); | ||
| leafItems.push_back(*it); | ||
| } | ||
| ++it; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/qt/coincontroltreewidget.cpp | head -120Repository: dashpay/dash
Length of output: 4804
🌐 Web query:
QTreeWidgetItemIterator Qt 5 collapsed items behavior
💡 Result:
In Qt 5, QTreeWidgetItemIterator iterates the item structure (the underlying tree), not what’s currently visible in the widget. So collapsed branches are still visited: collapsing only affects whether a parent’s children are shown in the view, not whether they “exist” for the iterator (you can see the same “continues past my subtree” behavior discussed on Stack Overflow). [1][2]
QTreeWidgetItemIterator::IteratorFlags let you filter by things like Hidden/NotHidden, Selected, Checked, etc., but there is no “Collapsed” flag—so collapsing does not automatically filter items out. [3]
If you need “only visible items”, you must filter manually, e.g.:
- exclude
item->isHidden(); - exclude items whose any ancestor is not expanded (use
item->parent()chain +parent->isExpanded()/item->isExpanded()semantics). [2]
Sources:
[1] Stack Overflow: QTreeWidgetItemIterator with a "subtree" (iterator continues after given item)
[2] Qt 5.15 docs mirror (QTreeWidgetItem: setExpanded(), isExpanded())
[3] Qt docs (QTreeWidgetItemIterator flags: Hidden/NotHidden/etc., no collapsed flag)
🌐 Web query:
Qt 5 itemBelow itemAbove visual order documentation
💡 Result:
In Qt 5, itemAbove() / itemBelow() are not APIs for visual stacking order of QGraphicsItem (those names exist for e.g. QTreeWidgetItem, not for Graphics View items).
For Graphics View visual order (stacking / paint order), Qt defines it like this:
- Items are drawn parent first, then children in ascending stacking order.
- Among siblings, stacking is determined by:
zValue()/setZValue()(higher Z is on top), and- if Z is equal, insertion order (added later is on top).
You can also change insertion order among same-Z siblings withstackBefore(). [1][2]
How to get “item above/below” in visual order:
- For siblings: use
parentItem()->childItems()(then interpret the list using the stacking rules above), or keep your own ordered list and update it when you callsetZValue()/stackBefore(). [1][2] - For “global” scene order:
QGraphicsScene::items(Qt::DescendingOrder)returns items sorted by stacking order, with the topmost first. [3]
Sources:
[1] Qt 5.15 QGraphicsItem stacking order / sorting rules (Z-value, insertion order, stackBefore) (doc.qt.io)
[2] Qt 5.15 QGraphicsItem::setZValue() and stackBefore() details (doc.qt.io)
[3] Qt 5.15 QGraphicsScene::items() sorting (topmost first) (doc.qt.io)
Citations:
- 1: https://doc.qt.io/qt-5/qgraphicsitem.html
- 2: https://doc.qt.io/qt-5/qgraphicsitem.html
- 3: https://doc.qt.io/qt-5/qgraphicsscene.html
🌐 Web query:
QTreeWidget itemBelow itemAbove Qt 5 collapsed items
💡 Result:
In Qt 5, QTreeWidget::itemBelow(item) / itemAbove(item) walk the tree in visual (on-screen) order, not the underlying full hierarchy. In other words, they only follow what the view would show if you pressed the Up/Down arrow keys:
itemBelow()returns the item visually below the given item. If the item is collapsed, its children are not visible, soitemBelow()will skip the whole collapsed subtree and return the next visible item after it.itemAbove()similarly returns the item above in visual order (and will step “out of” collapsed subtrees because their children aren’t visible). [1]
Because collapsing an item hides its children from the view, “below/above” navigation naturally ignores them. [2]
If you need “next/previous” regardless of expanded/collapsed state, don’t use itemBelow/itemAbove; iterate the actual item tree instead (e.g., recursive child(i) traversal, or QTreeWidgetItemIterator over all items).
Sources:
[1] Qt docs: QTreeWidget::itemBelow() (“visually below”) and itemAbove() (“item above”) https://doc.qt.io/qt-6/qtreewidget.html
[2] Explanation of collapse hiding children (tree view expected behavior) https://stackoverflow.com/questions/75558608/qtreewidget-qtreeview-custom-folding-to-always-show-leaf
🌐 Web query:
Qt 5 QTreeWidget itemBelow itemAbove API reference
💡 Result:
In Qt 5 (Qt Widgets), QTreeWidget provides these API functions:
QTreeWidgetItem *QTreeWidget::itemAbove(const QTreeWidgetItem *item) const— returns the item above the given item. Introduced in Qt 4.3. [1]QTreeWidgetItem *QTreeWidget::itemBelow(const QTreeWidgetItem *item) const— returns the item visually below the given item. Introduced in Qt 4.3. [1]
Qt 5.15 reference page (archived): https://doc.qt.io/archives/qt-5.15/qtreewidget.html [1]
Sources:
[1] Qt 5.15.19 (archives) — QTreeWidget Class (member function docs for itemAbove/itemBelow). (doc.qt.io)
Citations:
Shift-click range selection includes items in collapsed groups.
QTreeWidgetItemIterator walks the full tree structure and includes collapsed items. When shift-clicking between two visible leaves, the current code toggles all leaves in the range, including those hidden inside collapsed groups in between. itemBelow() / itemAbove() are safer here because they navigate in visual order and skip collapsed subtrees.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/coincontroltreewidget.cpp` around lines 96 - 107, The current
collection of leaf items uses QTreeWidgetItemIterator which iterates the full
tree (including collapsed children); replace that traversal with a visual-order
walk using itemBelow()/itemAbove() so only visible items are considered.
Specifically, instead of QTreeWidgetItemIterator, start from the first visible
item (e.g. topmost visible returned by itemAt(0,0) or the first top-level
visible) and iterate with itemBelow(current) until null, calling isLeafItem(...)
and pushing into leafItems while setting anchorIdx (for m_lastClickedItem) and
targetIdx (for clickedItem) the same way; this ensures shift-click range
selection toggles only visible leaves.
025cc48 to
6c812f5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/qt/coincontroltreewidget.cpp (1)
82-90: Anchor recording captures right-clicks and disabled items.The anchor is recorded for any click type on any leaf item. This could lead to unexpected behavior:
- Right-click (context menu) shouldn't establish an anchor for shift-click ranges
- Clicking a locked/disabled coin records it as anchor with
Uncheckedstate, causing subsequent shift-click to uncheck the rangeConsider restricting anchor recording to left-button clicks on enabled items:
♻️ Suggested refinement
if (!isShiftClick) { // Normal click — let Qt handle the checkbox toggle on release QTreeWidget::mouseReleaseEvent(event); // Record anchor after toggle so we capture the post-toggle state - if (clickedItem && isLeafItem(clickedItem)) { + if (event->button() == Qt::LeftButton && clickedItem && isLeafItem(clickedItem) && !clickedItem->isDisabled()) { m_lastClickedItem = clickedItem; } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/coincontroltreewidget.cpp` around lines 82 - 90, The anchor (m_lastClickedItem) is being set for any mouse release on leaf items, which incorrectly records right-clicks and disabled/locked items; update the mouseReleaseEvent handling so you only assign m_lastClickedItem when the release is from the left mouse button (check event->button() == Qt::LeftButton) and the clicked item is enabled (use isLeafItem(clickedItem) && (clickedItem->flags() & Qt::ItemIsEnabled) or an equivalent enabled check) — keep the existing early return for non-shift clicks and only record the anchor under these left-button + enabled conditions to avoid anchors from right-clicks or disabled coins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/qt/coincontroltreewidget.cpp`:
- Around line 82-90: The anchor (m_lastClickedItem) is being set for any mouse
release on leaf items, which incorrectly records right-clicks and
disabled/locked items; update the mouseReleaseEvent handling so you only assign
m_lastClickedItem when the release is from the left mouse button (check
event->button() == Qt::LeftButton) and the clicked item is enabled (use
isLeafItem(clickedItem) && (clickedItem->flags() & Qt::ItemIsEnabled) or an
equivalent enabled check) — keep the existing early return for non-shift clicks
and only record the anchor under these left-button + enabled conditions to avoid
anchors from right-clicks or disabled coins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec79567c-6a9c-4740-8b71-cf78fb7291d4
📥 Commits
Reviewing files that changed from the base of the PR and between 025cc4871f75e00b08683d8ad490f3803aeb799b and 6c812f54b8cdd12476ac00d190aafcbe490a2546.
📒 Files selected for processing (4)
src/qt/coincontroldialog.cppsrc/qt/coincontroldialog.hsrc/qt/coincontroltreewidget.cppsrc/qt/coincontroltreewidget.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/qt/coincontroltreewidget.h
- src/qt/coincontroldialog.cpp
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
6c812f5 to
bb8504f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb8504fa31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (clickedItem && isLeafItem(clickedItem)) { | ||
| m_lastClickedItem = clickedItem; |
There was a problem hiding this comment.
In mouseReleaseEvent, the anchor is updated for any non-shift leaf click, including right-clicks used only to open the context menu. That means a context-menu click can silently move the shift-range anchor to an item whose check state never changed, so the next shift-click applies an unexpected state/range. Restrict anchor updates to primary-button checkbox interactions so range selection tracks intentional selection clicks.
Useful? React with 👍 / 👎.
| bool isShiftClick = (event->button() == Qt::LeftButton) | ||
| && (event->modifiers() & Qt::ShiftModifier) | ||
| && m_lastClickedItem | ||
| && clickedItem | ||
| && clickedItem != m_lastClickedItem | ||
| && isLeafItem(clickedItem); |
There was a problem hiding this comment.
The shift-click branch is entered whenever the clicked item is a leaf, but it does not check whether that leaf is disabled. In this dialog, locked coins are disabled; shift-clicking a locked row can still trigger a range update on other enabled rows between the anchor and that locked row, even though the clicked row itself is non-interactive. Require enabled endpoints (and ideally an enabled anchor) before applying range changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/qt/coincontroltreewidget.cpp (1)
56-69:⚠️ Potential issue | 🟠 MajorDon't arm the range anchor from right-clicks or locked coins.
Line 67 currently records
m_lastClickedItemafter any leaf release, so a context-menu click or a click on a disabled coin can silently become the next range anchor. Lines 56-61 should also reject disabled endpoints, otherwise shift-clicking from or onto a locked row still applies the anchor state across the rest of the range.🩹 Suggested guard changes
- bool isShiftClick = (event->button() == Qt::LeftButton) - && (event->modifiers() & Qt::ShiftModifier) - && m_lastClickedItem - && clickedItem - && clickedItem != m_lastClickedItem - && isLeafItem(clickedItem); + const bool isShiftClick = (event->button() == Qt::LeftButton) + && (event->modifiers() & Qt::ShiftModifier) + && m_lastClickedItem + && !m_lastClickedItem->isDisabled() + && clickedItem + && !clickedItem->isDisabled() + && clickedItem != m_lastClickedItem + && isLeafItem(clickedItem); if (!isShiftClick) { // Normal click — let Qt handle the checkbox toggle on release QTreeWidget::mouseReleaseEvent(event); // Record anchor after toggle so we capture the post-toggle state - if (clickedItem && isLeafItem(clickedItem)) { + if (event->button() == Qt::LeftButton && clickedItem && isLeafItem(clickedItem) && !clickedItem->isDisabled()) { m_lastClickedItem = clickedItem; } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/coincontroltreewidget.cpp` around lines 56 - 69, The current logic arms m_lastClickedItem for any leaf on mouse release, which allows right-clicks or locked/disabled rows to become range anchors; update the guards so both the isShiftClick computation and the post-toggle assignment reject endpoints that are not enabled/selectable. Concretely, augment the isShiftClick condition (the variable around mouseReleaseEvent handling) to require that both m_lastClickedItem and clickedItem are enabled/selectable (e.g., check item->flags() & Qt::ItemIsEnabled or a project helper like isLockedCoin/isSelectableLeaf) and keep the same check when assigning m_lastClickedItem after the release so disabled/locked or non-left-button clicks never become the anchor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/qt/coincontroltreewidget.cpp`:
- Around line 56-69: The current logic arms m_lastClickedItem for any leaf on
mouse release, which allows right-clicks or locked/disabled rows to become range
anchors; update the guards so both the isShiftClick computation and the
post-toggle assignment reject endpoints that are not enabled/selectable.
Concretely, augment the isShiftClick condition (the variable around
mouseReleaseEvent handling) to require that both m_lastClickedItem and
clickedItem are enabled/selectable (e.g., check item->flags() &
Qt::ItemIsEnabled or a project helper like isLockedCoin/isSelectableLeaf) and
keep the same check when assigning m_lastClickedItem after the release so
disabled/locked or non-left-button clicks never become the anchor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 242fd09d-9362-4a0b-993c-f6bf615eab1e
📥 Commits
Reviewing files that changed from the base of the PR and between 6c812f54b8cdd12476ac00d190aafcbe490a2546 and bb8504fa31e3e321cb8093210c6886f4bbd57a78.
📒 Files selected for processing (4)
src/qt/coincontroldialog.cppsrc/qt/coincontroldialog.hsrc/qt/coincontroltreewidget.cppsrc/qt/coincontroltreewidget.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/qt/coincontroldialog.cpp
- src/qt/coincontroldialog.h
- src/qt/coincontroltreewidget.h
|
✅ Final review complete — no blockers (commit 3034e98) · triage: normal |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/qt/coincontroltreewidget.cpp (1)
81-97: Consider usingitemBelow()for guaranteed visual-order iteration.The current approach with
QTreeWidgetItemIteratoris fragile when the tree is sorted by column. According to Qt 5 documentation,QTreeWidgetItemIteratordoes not respectsortByColumn()order—it iterates based on internal tree structure using pre-order traversal, not visual display order. This means if the tree is sorted, the collectedleafItemsvector may not reflect the visual order the user sees.
QTreeWidget::itemBelow()(available since Qt 4.3) is the idiomatic way to walk visible items in display order:
- Automatically skips collapsed subtrees at any depth.
- Respects sort order applied to the tree.
- Matches visual on-screen order (arrow-key navigation).
- More resilient to future tree structure changes.
The current manual filtering of
parent->isExpanded()only checks one level up, which is insufficient if grandparent nodes are collapsed or if the tree is sorted.♻️ Proposed refactor using itemBelow()
- // Collect visible leaf items in display order, skipping children of - // collapsed parent nodes so we don't toggle coins the user can't see std::vector<QTreeWidgetItem*> leafItems; int anchorIdx = -1; int targetIdx = -1; - QTreeWidgetItemIterator it(this); - while (*it) { - if (isLeafItem(*it)) { - QTreeWidgetItem* parent = (*it)->parent(); - if (!parent || parent->isExpanded()) { - if (*it == m_lastClickedItem) anchorIdx = leafItems.size(); - if (*it == clickedItem) targetIdx = leafItems.size(); - leafItems.push_back(*it); - } + + // Start from the topmost visible item and walk down in visual order + QTreeWidgetItem* current = topLevelItem(0); + while (current) { + if (isLeafItem(current)) { + if (current == m_lastClickedItem) anchorIdx = leafItems.size(); + if (current == clickedItem) targetIdx = leafItems.size(); + leafItems.push_back(current); } - ++it; + current = itemBelow(current); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/coincontroltreewidget.cpp` around lines 81 - 97, The current QTreeWidgetItemIterator loop in coincontroltreewidget.cpp can produce non-visual order and misses collapsed ancestors; replace it with a visual-order traversal using QTreeWidget::itemBelow by starting at topLevelItem(0) (or the first visible item) and repeatedly calling itemBelow to iterate visible items in display order, skipping non-leaf items via isLeafItem, setting anchorIdx when the iterated item equals m_lastClickedItem and targetIdx when it equals clickedItem, and pushing matching items into leafItems; this ensures sorted order and correctly skips any collapsed subtrees at any depth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/qt/coincontroltreewidget.cpp`:
- Around line 81-97: The current QTreeWidgetItemIterator loop in
coincontroltreewidget.cpp can produce non-visual order and misses collapsed
ancestors; replace it with a visual-order traversal using QTreeWidget::itemBelow
by starting at topLevelItem(0) (or the first visible item) and repeatedly
calling itemBelow to iterate visible items in display order, skipping non-leaf
items via isLeafItem, setting anchorIdx when the iterated item equals
m_lastClickedItem and targetIdx when it equals clickedItem, and pushing matching
items into leafItems; this ensures sorted order and correctly skips any
collapsed subtrees at any depth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e6838cf-a8f8-434a-b4ab-b8b1a986addd
📥 Commits
Reviewing files that changed from the base of the PR and between bb8504fa31e3e321cb8093210c6886f4bbd57a78 and 912f2ada8676d7287be2a8e4e6d02d9e0b5c216c.
📒 Files selected for processing (1)
src/qt/coincontroltreewidget.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 912f2ada86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (event->button() == Qt::LeftButton && clickedItem | ||
| && isLeafItem(clickedItem) && !clickedItem->isDisabled()) { | ||
| m_lastClickedItem = clickedItem; |
There was a problem hiding this comment.
The non-shift path records m_lastClickedItem for any left-clicked leaf row, even when that click did not change the checkbox state. In the coin control tree, users can left-click rows for navigation/context without toggling selection, which silently moves the shift-range anchor; a later shift-click then applies an unexpected state across a range of UTXOs. Restrict anchor updates to actual checkbox toggles (or checkbox-indicator clicks) so range selection follows intentional selection actions.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean range-selection implementation with proper anchor tracking, dangling pointer protection, and efficient batch updates. The core design is sound — anchor/target/range with skip-locked-coins logic. However, both the anchor-setting and the shift-range application trigger on any left-click anywhere on a leaf row, not just on the checkbox indicator. This means clicking an amount or address cell (which normally just selects the row) now silently moves the range anchor and can batch-toggle coins on shift-click.
Reviewed commit: 912f2ada
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/coincontroltreewidget.cpp`:
- [SUGGESTION] lines 54-75: Range selection and anchor tracking fire from any column click, not just checkbox interactions
Both the shift-range path (line 63) and the anchor-update path (line 71-74) trigger on any left-click on a leaf row, regardless of which column was clicked. In the existing dialog, clicking a leaf's text columns (amount, address, date, etc.) only changes focus/selection — the checkbox state is only toggled when clicking column 0's checkbox indicator.
After this PR:
1. Clicking the "Amount" column on row A silently sets A as the anchor
2. Shift-clicking the "Address" column on row D batch-toggles all coins A through D
Neither click was on a checkbox, but coins are now selected/deselected. This is a behavioral regression from Qt's normal checkbox handling where state changes are tied to actual checkbox activation.
The fix should gate both anchor updates and range application on clicks in the checkbox column. At minimum, check `visualItemRect(item)` against the checkbox indicator area, or verify that `event->pos().x()` falls within column 0's geometry via `header()->sectionPosition(0)` + `header()->sectionSize(0)`.
| QTreeWidgetItem* clickedItem = itemAt(event->pos()); | ||
|
|
||
| bool isShiftClick = (event->button() == Qt::LeftButton) | ||
| && (event->modifiers() & Qt::ShiftModifier) | ||
| && m_lastClickedItem | ||
| && clickedItem | ||
| && clickedItem != m_lastClickedItem | ||
| && isLeafItem(clickedItem) | ||
| && !clickedItem->isDisabled() | ||
| && !m_lastClickedItem->isDisabled(); | ||
|
|
||
| if (!isShiftClick) { | ||
| // Normal click — let Qt handle the checkbox toggle on release | ||
| QTreeWidget::mouseReleaseEvent(event); | ||
| // Record anchor after toggle so we capture the post-toggle state | ||
| if (event->button() == Qt::LeftButton && clickedItem | ||
| && isLeafItem(clickedItem) && !clickedItem->isDisabled()) { | ||
| m_lastClickedItem = clickedItem; | ||
| } | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: Range selection and anchor tracking fire from any column click, not just checkbox interactions
Both the shift-range path (line 63) and the anchor-update path (line 71-74) trigger on any left-click on a leaf row, regardless of which column was clicked. In the existing dialog, clicking a leaf's text columns (amount, address, date, etc.) only changes focus/selection — the checkbox state is only toggled when clicking column 0's checkbox indicator.
After this PR:
- Clicking the "Amount" column on row A silently sets A as the anchor
- Shift-clicking the "Address" column on row D batch-toggles all coins A through D
Neither click was on a checkbox, but coins are now selected/deselected. This is a behavioral regression from Qt's normal checkbox handling where state changes are tied to actual checkbox activation.
The fix should gate both anchor updates and range application on clicks in the checkbox column. At minimum, check visualItemRect(item) against the checkbox indicator area, or verify that event->pos().x() falls within column 0's geometry via header()->sectionPosition(0) + header()->sectionSize(0).
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/coincontroltreewidget.cpp`:
- [SUGGESTION] lines 54-75: Range selection and anchor tracking fire from any column click, not just checkbox interactions
Both the shift-range path (line 63) and the anchor-update path (line 71-74) trigger on any left-click on a leaf row, regardless of which column was clicked. In the existing dialog, clicking a leaf's text columns (amount, address, date, etc.) only changes focus/selection — the checkbox state is only toggled when clicking column 0's checkbox indicator.
After this PR:
1. Clicking the "Amount" column on row A silently sets A as the anchor
2. Shift-clicking the "Address" column on row D batch-toggles all coins A through D
Neither click was on a checkbox, but coins are now selected/deselected. This is a behavioral regression from Qt's normal checkbox handling where state changes are tied to actual checkbox activation.
The fix should gate both anchor updates and range application on clicks in the checkbox column. At minimum, check `visualItemRect(item)` against the checkbox indicator area, or verify that `event->pos().x()` falls within column 0's geometry via `header()->sectionPosition(0)` + `header()->sectionSize(0)`.
c2c1c56 to
6b5a555
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Targeted Qt UI feature adding shift-click range selection to coin control. Implementation is defensive (anchor reset on view rebuild, locked-coin skip, gated to checkbox-area left-clicks). No blockers found. Minor nitpicks around magic-number duplication and fragile leaf detection; both agents flagged the lack of automated test coverage for the new interaction.
🟡 1 suggestion(s) | 💬 3 nitpick(s)
4 additional finding(s)
suggestion: No automated coverage for new shift-click selection logic
src/qt/coincontroltreewidget.cpp (line 72)
The new mouse-release handler, anchor tracking, hidden-row filtering, and refreshLabels() fast path ship without any tests under src/qt/test/. Qt test infrastructure already exists in this repo (e.g. wallettests.cpp, addressbooktests.cpp) and supports QTest-driven event simulation, so the deterministic pieces are testable: anchor recording on a click, range application across visible leaves, skipping of locked/disabled items, collapsed-parent exclusion, and anchor reset on updateView(). Because the shift-click path bypasses the default QTreeWidget::mouseReleaseEvent, a small future refactor could silently regress selection semantics with no CI signal. A regression test would meaningfully de-risk this code.
nitpick: Use named constants from CoinControlDialog instead of local literals
src/qt/coincontroltreewidget.cpp (line 46)
isLeafItem and isCheckboxClick redefine local integers COLUMN_ADDRESS = 3 and COLUMN_CHECKBOX = 0 and reference Qt::UserRole directly. The rest of the dialog uses CoinControlDialog::COLUMN_ADDRESS, CoinControlDialog::COLUMN_CHECKBOX, and CoinControlDialog::TxHashRole (defined in coincontroldialog.h:76-88, used throughout coincontroldialog.cpp e.g. lines 47-51, 129, 420). If the column enum is ever reordered or TxHashRole is moved, these literals would silently drift from viewItemChanged's leaf detection — shift-click would then misclassify rows with no compile error. Since qt/coincontroldialog.h is already included here, the named constants would keep the two leaf-detection sites in lockstep.
nitpick: Leaf detection by hex-string length duplicates a fragile assumption
src/qt/coincontroltreewidget.cpp (line 49)
isLeafItem decides whether a row is a UTXO by reading data(COLUMN_ADDRESS, Qt::UserRole) and checking toString().length() == 64. This mirrors CoinControlDialog::viewItemChanged (coincontroldialog.cpp around line 420), but the assumption — that parent (wallet-address) rows never store a 64-char string under TxHashRole — is now duplicated in two places. Parent rows today only call setText(COLUMN_ADDRESS, sWalletAddress), but a future change that stashes any 64-char string under that role on a parent row would misclassify it as a leaf here. A item->parent() != nullptr check in tree mode, or a dedicated IsLeafRole flag, would be more robust. Not blocking — just worth knowing the trap now exists in two locations.
nitpick: Shift-click branch skips QTreeWidget::mouseReleaseEvent — current-item state will not update
src/qt/coincontroltreewidget.cpp (line 116)
When the shift-click branch executes, QTreeWidget::mouseReleaseEvent(event) is never invoked, so Qt's default current-item / focus-row handling for the click doesn't fire. Checkbox states across the range are updated correctly, but the visually 'current' row (focus rectangle, currentItem()) won't follow the shift-click target. This may be intentional — to prevent Qt's built-in shift-range-selection from fighting with the new behavior — but worth confirming the UX matches expectations. If the visual current-row update is desired, calling setCurrentItem(clickedItem) after the batch would handle it without re-enabling Qt's default range selection.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] In `src/qt/coincontroltreewidget.cpp`:72-149: No automated coverage for new shift-click selection logic
The new mouse-release handler, anchor tracking, hidden-row filtering, and `refreshLabels()` fast path ship without any tests under `src/qt/test/`. Qt test infrastructure already exists in this repo (e.g. `wallettests.cpp`, `addressbooktests.cpp`) and supports QTest-driven event simulation, so the deterministic pieces are testable: anchor recording on a click, range application across visible leaves, skipping of locked/disabled items, collapsed-parent exclusion, and anchor reset on `updateView()`. Because the shift-click path bypasses the default `QTreeWidget::mouseReleaseEvent`, a small future refactor could silently regress selection semantics with no CI signal. A regression test would meaningfully de-risk this code.
- [NITPICK] In `src/qt/coincontroltreewidget.cpp`:46-51: Use named constants from CoinControlDialog instead of local literals
`isLeafItem` and `isCheckboxClick` redefine local integers `COLUMN_ADDRESS = 3` and `COLUMN_CHECKBOX = 0` and reference `Qt::UserRole` directly. The rest of the dialog uses `CoinControlDialog::COLUMN_ADDRESS`, `CoinControlDialog::COLUMN_CHECKBOX`, and `CoinControlDialog::TxHashRole` (defined in `coincontroldialog.h:76-88`, used throughout `coincontroldialog.cpp` e.g. lines 47-51, 129, 420). If the column enum is ever reordered or `TxHashRole` is moved, these literals would silently drift from `viewItemChanged`'s leaf detection — shift-click would then misclassify rows with no compile error. Since `qt/coincontroldialog.h` is already included here, the named constants would keep the two leaf-detection sites in lockstep.
- [NITPICK] In `src/qt/coincontroltreewidget.cpp`:49-50: Leaf detection by hex-string length duplicates a fragile assumption
`isLeafItem` decides whether a row is a UTXO by reading `data(COLUMN_ADDRESS, Qt::UserRole)` and checking `toString().length() == 64`. This mirrors `CoinControlDialog::viewItemChanged` (coincontroldialog.cpp around line 420), but the assumption — that parent (wallet-address) rows never store a 64-char string under `TxHashRole` — is now duplicated in two places. Parent rows today only call `setText(COLUMN_ADDRESS, sWalletAddress)`, but a future change that stashes any 64-char string under that role on a parent row would misclassify it as a leaf here. A `item->parent() != nullptr` check in tree mode, or a dedicated `IsLeafRole` flag, would be more robust. Not blocking — just worth knowing the trap now exists in two locations.
- [NITPICK] In `src/qt/coincontroltreewidget.cpp`:116-124: Shift-click branch skips QTreeWidget::mouseReleaseEvent — current-item state will not update
When the shift-click branch executes, `QTreeWidget::mouseReleaseEvent(event)` is never invoked, so Qt's default current-item / focus-row handling for the click doesn't fire. Checkbox states across the range are updated correctly, but the visually 'current' row (focus rectangle, `currentItem()`) won't follow the shift-click target. This may be intentional — to prevent Qt's built-in shift-range-selection from fighting with the new behavior — but worth confirming the UX matches expectations. If the visual current-row update is desired, calling `setCurrentItem(clickedItem)` after the batch would handle it without re-enabling Qt's default range selection.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
6b5a555 to
7086905
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7086905197
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (anchorIdx < 0 || targetIdx < 0) { | ||
| QTreeWidget::mouseReleaseEvent(event); |
There was a problem hiding this comment.
If a user checks a leaf, collapses its parent, and then Shift-clicks another visible checkbox, the saved anchor is omitted from leafItems, so this fallback performs only the ordinary target toggle. Because it also bypasses the normal path that records m_lastClickedItem, the hidden anchor remains active and subsequent Shift-clicks continue failing until the user performs another normal checkbox click; reset or replace the anchor in this fallback.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The range-selection path correctly gates interactions to enabled leaf checkboxes, reads the anchor state live, skips disabled coins, and excludes children of collapsed groups. One interaction bug remains: collapsing the anchor's parent leaves a stale hidden anchor that prevents subsequent Shift-clicks from selecting ranges, and the event-driven behavior has no automated regression coverage. Source: reviewer backend gpt-5.6-sol (general and dash-core-commit-history) plus CodeRabbit automated review (backend model not disclosed); final verifier backend gpt-5.6-sol. Orchestration only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/coincontroltreewidget.cpp`:
- [BLOCKING] src/qt/coincontroltreewidget.cpp:129-131: Replace an anchor that is no longer visible
When the anchor's parent is collapsed, the anchor is deliberately omitted from `leafItems`, leaving `anchorIdx` negative. The fallback delegates the Shift-click to Qt, which toggles the visible target normally, but it neither clears the hidden anchor nor records the newly toggled target. Every subsequent Shift-click therefore repeats this fallback instead of applying a range until the user performs an unmodified checkbox click. After normal handling, replace the unusable anchor with the target if its checkbox changed; otherwise clear the anchor.
- [SUGGESTION] src/qt/coincontroltreewidget.cpp:74-149: No automated coverage for new shift-click selection logic
No Qt test covers the new mouse-release handler. Deterministic cases include recording an anchor only after a real checkbox toggle, applying ranges in list and tree order, skipping disabled coins, excluding collapsed groups, recovering from a hidden anchor, and resetting the anchor when `updateView()` rebuilds the tree. These behaviors depend on Qt event sequencing and the range path bypasses the default release handler, so manual testing alone leaves the feature vulnerable to silent interaction regressions.
In `<commit:099bf3b>`:
- [SUGGESTION] <commit:099bf3b>:1: Squash the two corrective commits into the feature commit
Commits 099bf3b3051a6d9f6683b2a084803685154fc2aa and 708690519704cb57c957830b6242a9823b723447 exclusively correct the shift-selection implementation introduced by 10a977425743a36d3f9beec0ceb2cda479219316. Because Dash preserves PR commits in permanent history, retaining these review-fix seams adds avoidable blame and bisect noise. Fold both corrections into the feature commit so the range-selection feature lands as one coherent change.
| if (anchorIdx < 0 || targetIdx < 0) { | ||
| QTreeWidget::mouseReleaseEvent(event); | ||
| return; |
There was a problem hiding this comment.
✅ Resolved at
3034e981; see the replies below.
🔴 Blocking: Replace an anchor that is no longer visible
When the anchor's parent is collapsed, the anchor is deliberately omitted from leafItems, leaving anchorIdx negative. The fallback delegates the Shift-click to Qt, which toggles the visible target normally, but it neither clears the hidden anchor nor records the newly toggled target. Every subsequent Shift-click therefore repeats this fallback instead of applying a range until the user performs an unmodified checkbox click. After normal handling, replace the unusable anchor with the target if its checkbox changed; otherwise clear the anchor.
| if (anchorIdx < 0 || targetIdx < 0) { | |
| QTreeWidget::mouseReleaseEvent(event); | |
| return; | |
| if (anchorIdx < 0 || targetIdx < 0) { | |
| const Qt::CheckState previousState = clickedItem->checkState(COLUMN_CHECKBOX); | |
| QTreeWidget::mouseReleaseEvent(event); | |
| if (clickedItem->checkState(COLUMN_CHECKBOX) != previousState) { | |
| m_lastClickedItem = clickedItem; | |
| } else { | |
| resetAnchor(); | |
| } | |
| return; | |
| } |
source: ['codex']
There was a problem hiding this comment.
Fixed in 7581401 as suggested. If the anchor is no longer visible, the Shift-click toggles the target normally. The target becomes the new anchor if its checkbox changed, and the anchor is cleared otherwise. The hiddenAnchorReplaced test covers this and fails without the fix.
🤖 Posted autonomously by Codex on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 3034e981): Confirmed that the fallback in 7581401 records the clicked target when Qt changes its checkbox and clears the anchor otherwise. The current hiddenAnchorReplaced test checks that the next Shift-click selects a range from the replacement anchor; I verified the code and assertions statically, without rerunning the mutation check.
| void CoinControlTreeWidget::mouseReleaseEvent(QMouseEvent *event) | ||
| { | ||
| int COLUMN_CHECKBOX = 0; | ||
|
|
||
| QTreeWidgetItem* clickedItem = itemAt(event->pos()); | ||
| const bool isCheckboxInteraction = isCheckboxClick(clickedItem, event->pos()); | ||
|
|
||
| bool isShiftClick = (event->button() == Qt::LeftButton) | ||
| && (event->modifiers() & Qt::ShiftModifier) | ||
| && m_lastClickedItem | ||
| && clickedItem | ||
| && clickedItem != m_lastClickedItem | ||
| && isCheckboxInteraction | ||
| && isLeafItem(clickedItem) | ||
| && !clickedItem->isDisabled() | ||
| && !m_lastClickedItem->isDisabled(); | ||
|
|
||
| if (!isShiftClick) { | ||
| // Normal click — let Qt handle the checkbox toggle on release | ||
| const Qt::CheckState previousState = clickedItem ? clickedItem->checkState(COLUMN_CHECKBOX) : Qt::Unchecked; | ||
| QTreeWidget::mouseReleaseEvent(event); | ||
| // Record anchor after toggle so we capture the post-toggle state | ||
| if (event->button() == Qt::LeftButton && clickedItem | ||
| && isCheckboxInteraction | ||
| && isLeafItem(clickedItem) | ||
| && !clickedItem->isDisabled() | ||
| && clickedItem->checkState(COLUMN_CHECKBOX) != previousState) { | ||
| m_lastClickedItem = clickedItem; | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Shift+click: select/deselect the range between anchor and target | ||
| // Read the anchor's current check state live (not cached) so it stays | ||
| // correct after bulk operations like Select All or parent tristate changes | ||
| Qt::CheckState stateToApply = m_lastClickedItem->checkState(COLUMN_CHECKBOX); | ||
|
|
||
| // Collect visible leaf items in display order, skipping children of | ||
| // collapsed parent nodes so we don't toggle coins the user can't see | ||
| std::vector<QTreeWidgetItem*> leafItems; | ||
| int anchorIdx = -1; | ||
| int targetIdx = -1; | ||
| QTreeWidgetItemIterator it(this); | ||
| while (*it) { | ||
| if (isLeafItem(*it)) { | ||
| QTreeWidgetItem* parent = (*it)->parent(); | ||
| if (!parent || parent->isExpanded()) { | ||
| if (*it == m_lastClickedItem) anchorIdx = leafItems.size(); | ||
| if (*it == clickedItem) targetIdx = leafItems.size(); | ||
| leafItems.push_back(*it); | ||
| } | ||
| } | ||
| ++it; | ||
| } | ||
|
|
||
| if (anchorIdx < 0 || targetIdx < 0) { | ||
| QTreeWidget::mouseReleaseEvent(event); | ||
| return; | ||
| } | ||
|
|
||
| if (anchorIdx > targetIdx) std::swap(anchorIdx, targetIdx); | ||
|
|
||
| // Batch update: disable widget to suppress per-item updateLabels calls | ||
| setEnabled(false); | ||
| for (int i = anchorIdx; i <= targetIdx; ++i) { | ||
| QTreeWidgetItem* item = leafItems[i]; | ||
| if (!item->isDisabled() && item->checkState(COLUMN_CHECKBOX) != stateToApply) { | ||
| item->setCheckState(COLUMN_CHECKBOX, stateToApply); | ||
| } | ||
| } | ||
| setEnabled(true); | ||
|
|
||
| // Single label update for the whole batch | ||
| CoinControlDialog* coinControlDialog = qobject_cast<CoinControlDialog*>(this->parentWidget()); | ||
| if (coinControlDialog) coinControlDialog->refreshLabels(); | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
3034e981; see the replies below.
🟡 Suggestion: No automated coverage for new shift-click selection logic
No Qt test covers the new mouse-release handler. Deterministic cases include recording an anchor only after a real checkbox toggle, applying ranges in list and tree order, skipping disabled coins, excluding collapsed groups, recovering from a hidden anchor, and resetting the anchor when updateView() rebuilds the tree. These behaviors depend on Qt event sequencing and the range path bypasses the default release handler, so manual testing alone leaves the feature vulnerable to silent interaction regressions.
source: ['codex']
There was a problem hiding this comment.
Added CoinControlTreeWidgetTests in 1a77d5c. They use real mouse clicks on the checkbox indicator to cover anchor recording, ranges in both directions, clicks outside the checkbox, locked coins, collapsed groups, the hidden anchor, resetAnchor(), and focus after a range. Writing them exposed one more bug: applying a range left keyboard focus off the tree, because the tree is briefly disabled. That is fixed in the feature commit, and rangeKeepsKeyboardFocus covers it. The two review-fix commits are squashed into the feature commit.
🤖 Posted autonomously by Codex on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 3034e981): Confirmed that the final test commit, 3034e98, registers CoinControlTreeWidgetTests in the Qt runner and build configuration, with real checkbox clicks covering ranges, outside-checkbox interactions, disabled coins, collapsed groups, hidden-anchor recovery, resetAnchor(), and focus restoration. The feature commit also restores tree focus after the batch update, matching the regression addressed by rangeKeepsKeyboardFocus.
Selecting many coins in the coin control dialog, such as a set of CoinJoin denominations, takes one click per coin. Support the usual Shift-click range selection in CoinControlTreeWidget: a click on a coin's checkbox that toggles it records that coin as the anchor, and a Shift-click on another coin's checkbox applies the anchor's current check state to every coin between the two in display order. - Only left-button clicks on the checkbox of an enabled coin set the anchor or apply a range, so clicks on other columns, context-menu clicks and locked coins keep their previous behavior. - The anchor's state is read when the range is applied, so it is correct after Select All or a group checkbox changed it. - Locked (disabled) coins inside the range and coins in collapsed address groups are left alone. - If the anchor is hidden in a collapsed group, the Shift-click toggles only its target, which becomes the new anchor. - The range is applied with the tree disabled, the existing pattern that defers the label update to a single refreshLabels() call. Focus is given back to the tree afterwards so Space keeps toggling coins. - updateView() resets the anchor when it rebuilds the tree. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
7086905 to
1a77d5c
Compare
Drive CoinControlTreeWidget with real mouse clicks on the checkbox indicator: checking and unchecking a range in both directions, clicks outside the checkbox, locked coins, collapsed address groups, an anchor hidden by collapsing its group, resetAnchor(), and keyboard focus after a range was applied. Like the other widget tests, the suite skips itself on macOS with the minimal platform, which crashes when widgets are shown (QTBUG-49686); run it with QT_QPA_PLATFORM=cocoa there. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1a77d5c to
3034e98
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete diff at head 3034e98, including dialog lifecycle, wallet-selection callbacks, range handling, and Qt test integration; no actionable in-scope defects were found. Both prior findings are fixed in the current source. Verification was static only: the supplied CI snapshot shows successful formatting and amd64 container checks, while the main validation job and several other checks remain queued.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change adds moderately involved but well-contained Qt checkbox range-selection behavior and regression tests, without the large or intricate changes to transaction coin-selection logic required for critical. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
thepastaclaw
added
the
pastaclaw:approved
thepastaclaw
removed
the
pastaclaw:approved
…lang-analyzer null dereference) 92365cc fix(qt): reserve the coin vector in the coin control test helper (Konstantin Akimov) 9c6ccd0 fix(qt): clear ItemIsEnabled through QFlags::setFlag in coin control test (Konstantin Akimov) 7daa67d fix(test): assert the chain tip before dereferencing it in reward reallocation tests (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented `develop` CI is red since the merge of #7260 (aec4994). Both `linux64_asan-build` and `linux64_multiprocess-build` pass on the merge of #7790 (2ca083a) and on the head of #7584 (fed2f49), and both are red on the head of #7260 (3034e98) and on its merge commit. #7260 was merged with those two jobs failing. 1. `linux64_asan` fails in `test_dash-qt`: `CoinControlTreeWidgetTests::disabledCoinsSkipped()` triggers an UBSan implicit-integer-sign-change inside `QFlags::operator&`. The test, added by #7260 in 3034e98, clears a flag with `flags() & ~Qt::ItemIsEnabled`. `~Qt::ItemIsEnabled` is evaluated as a plain `int` (-33) and QFlags converts it to its unsigned storage type. 2. `linux64_multiprocess` reports a clang static analyzer diagnostic in `test/block_reward_reallocation_tests.cpp:209`: `tip->nBits` dereferences a pointer the analyzer considers possibly null, because `GetMnRewardEraAfter()` (via `DeploymentActiveAfter()`) accepts a null `pindexPrev`. The pattern dates back to #7339 (7f2058e) and the file was last touched by #7536 in August, so this diagnostic is not caused by #7260's diff. The analyzer runs in that job because the tidy step operates on the `make distdir` copy of the tree, which does not include `src/.clang-tidy`, so clang-tidy falls back to its default `clang-analyzer-*` checks. I could not read the job log (needs credentials) to confirm that this diagnostic alone fails the step, so this part is fixed on the evidence of the pasted output. ## What was done? - `src/qt/test/coincontroltreewidgettests.cpp`: clear the flag with `flags().setFlag(Qt::ItemIsEnabled, false)`, which stays inside the `QFlags` type. - `src/test/block_reward_reallocation_tests.cpp`: wrap every chain-tip lookup that feeds `GetMnRewardEraAfter()` in `Assert()` and include `util/check.h`. The chain is never empty at those points; the assertion documents it for the analyzer and the reader. This is test code, so a fatal check is acceptable. ## How Has This Been Tested? see CI ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: cd7d524e35ec6039f5b72ae05cfd498c83c00d842a617bf412de87a5759eb7fbd74f5b96698e9a3aab27174d8ac8942f5624bbe49a1028c4d9d0fbc3cfb3dd33
fbe6526 Merge #7807: feat(consensus): hold EvoNode shares and multiple payouts behind a new evo_shares deployment (pasta) 57fd2d9 Merge #7776: fix: keep a pending ProUpServTx across a registrar update that keeps the operator key (pasta) ca69801 Merge #7803: fix: return non-zero amount of accounts after sethdseed (pasta) 57a8f07 Merge #7796: backport: bitcoin#26720 (remaining), partial bitcoin#29523: weight-check the knapsack exact-match selection (pasta) 4eb82f6 Merge #7786: fix(net): keep a dedicated onion listener when -bind is given and correct the bind release note (pasta) 70626a0 Merge #7793: backport: partial bitcoin#28868 (wallet: reload wallet if migration exited early, keep mixed watchonly txs) (pasta) 250a3cd Merge #7790: fix: guard cached governance object flags with the object lock (pasta) 700c195 Merge #7787: fix: only sign EHF signals inside the deployment's start/timeout window (pasta) a3b0f3c Merge #7785: fix: restore the txindex requirement when governance validation is enabled (pasta) 2d1735d Merge #7794: fix(wallet): floor the denomination gap threshold so rebalancing cannot oscillate (pasta) d9f6592 Merge #7783: fix: stop fish completion from re-running the typed dash-cli command (pasta) 89d44c8 Merge #7795: backport: bitcoin#34272, partial bitcoin#31650 (pass PSBT by const reference in PSBTInputSignedAndVerified) (pasta) a1d7411 Merge #7779: fix!: report legacy evonode Platform addresses when listdiff changes its address (pasta) 73f6ea2 Merge #7789: fix(qt): report malformed lang/font settings instead of aborting at startup (pasta) 40de493 Merge #7781: fix(rpc): use a consistent tip height in quorum dkgstatus (pasta) 7d1a90d Merge #7791: fix(net): request object votes from nPeersPerHashMax peers (pasta) 51b145c Merge #7778: test: use single-member in functional tests: feature_mnehf, feature_notifications, p2p_instantsend (pasta) Pull request description: ## Issue being fixed or feature implemented Backports for v24.0.0-rc.3. `v24.0.x` was cut at the v24.0.0-rc.2 tag (1e239d4), and develop has since moved to 24.1.0 (#7775), so rc.3 is built from this branch instead of being tagged on develop like rc.1 and rc.2. ## What was done? Cherry-picked (`git cherry-pick -m1 -x`) every develop merge since rc.2 that carries `backport-candidate-24.0.x`, plus #7778, which #7807's test needs. They are in develop merge order: - #7778 test: use single-member in functional tests: feature_mnehf, feature_notifications, p2p_instantsend (test-only. Needed because #7807's `feature_evo_shares_activation.py` depends on its `mine_quorum_single_member()` fix and fails without it.) - #7791 fix(net): request object votes from nPeersPerHashMax peers - #7781 fix(rpc): use a consistent tip height in quorum dkgstatus - #7789 fix(qt): report malformed lang/font settings instead of aborting at startup - #7779 fix!: report legacy evonode Platform addresses when listdiff changes its address - #7795 backport: bitcoin#34272, partial bitcoin#31650 (pass PSBT by const reference in PSBTInputSignedAndVerified) - #7783 fix: stop fish completion from re-running the typed dash-cli command - #7794 fix(wallet): floor the denomination gap threshold so rebalancing cannot oscillate - #7785 fix: restore the txindex requirement when governance validation is enabled - #7787 fix: only sign EHF signals inside the deployment's start/timeout window - #7790 fix: guard cached governance object flags with the object lock - #7793 backport: partial bitcoin#28868 (wallet: reload wallet if migration exited early, keep mixed watchonly txs) - #7786 fix(net): keep a dedicated onion listener when -bind is given and correct the bind release note. Includes partial bitcoin#36170 (5f40d56). Upstream's dedicated-onion-bind changes to `feature_torcontrol.py` and `p2p_private_broadcast.py` are not included, because those tests depend on upstream commits (5693833, e74d54e) that Dash does not have. - #7796 backport: bitcoin#26720 (remaining), partial bitcoin#29523: weight-check the knapsack exact-match selection - #7803 fix: return non-zero amount of accounts after sethdseed - #7776 fix: keep a pending ProUpServTx across a registrar update that keeps the operator key - #7807 feat(consensus): hold EvoNode shares and multiple payouts behind a new evo_shares deployment One pick needed conflict resolution, in a test only, described in its commit message: #7793, `test/functional/wallet_migration.py`. `test_conflict_txs()` comes from bitcoin#28542 (#7762), which is not on this branch, so it is left out. The C++ part of #7793 is identical to develop. Merged since rc.2 but intentionally not included: #7775 (24.1 version bump), #7260 and #7804 (new Qt feature and its CI fix), #7584 (refactor), routine upstream backports #7257, #7759, #7762, #7768, #7788, #7801, and test/CI-only changes #7777 and #7797. No version change is needed. `configure.ac` is already 24.0.0 with `CLIENT_VERSION_IS_RELEASE` false, so the build takes its version string from the `v24.0.0-rc.3` tag, as it did for rc.2. ## How Has This Been Tested? Built on macOS arm64 against depends with `--enable-debug --enable-werror` (dashd, dash-qt, tests). - `make check`: all unit test suites pass, including `test_dash-qt`. - Functional tests, all passing: `feature_evo_shares_activation.py`, `feature_masternode_shares.py`, `feature_mnehf.py`, `feature_governance.py --descriptors`, `feature_governance_objects.py`, `feature_governance_txindex_devnet.py`, `feature_proxy.py`, `feature_notifications.py`, `feature_llmq_connections.py`, `feature_llmq_chainlocks_automatic.py`, `feature_llmq_singlenode.py`, `p2p_instantsend.py`, `rpc_blockchain.py` (v1 and v2 transport), `rpc_netinfo.py`, `rpc_quorum.py`, `rpc_verifychainlock.py`, `wallet_hd.py` (legacy and descriptors), `wallet_migration.py`. - `feature_bind_extra.py` (from #7786) only runs on Linux and was skipped locally, so CI covers it. Every touched file was diffed against develop. The remaining differences come only from develop PRs that are not part of this backport. ## Breaking Changes Carried over from the picked PRs: - #7807: consensus change behind the new `evo_shares` deployment (bit 14). Mainnet and testnet are unaffected until it activates. Devnets with `v24` already active and an EvoNode carrying several owner payouts need to check or reset. New RPC `protx shared_register_prepare_evo`. - #7785: outside regtest, a node with `-txindex=0` and without `-disablegovernance` refuses to start again, as before v23.1.0. - #7786: carries a partial bitcoin#36170. A node started with `-bind` but no specific `-bind=<addr:port>=onion` now refuses to start while `-listenonion` is enabled. That is the default when listening, even without Tor configured. A wildcard onion bind is also refused. Such nodes need `-bind=127.0.0.1:<port>=onion` or `-listenonion=0`. Nodes without `-bind`, including those using only `-whitebind`, keep the default `127.0.0.1:9996` (`19996` testnet) onion target. See `doc/release-notes-7300.md`. - #7779: `protx listdiff` now includes `platform_p2p` and `platform_https` for a legacy-address evonode whose core P2P address changes. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 8cb01bdb0234f31b5dd2c105b69673de148922991a7523627395c0e0ccfa22b94be5ffbecc842398d443bc90fd9baf2eee6a9cf6fcdbbd1626eb00b93726a55a
2 participants
Issue being fixed or feature implemented
Selecting many coins in the coin control dialog (for example a set of CoinJoin denominations) takes one click per coin. This adds the usual Shift-click range selection, so a contiguous block of coins can be checked or unchecked in one action.
What was done?
CoinControlTreeWidgetoverridesmouseReleaseEvent:updateView()pattern that avoids recalculating labels for every item. OneCoinControlDialog::refreshLabels()call follows, and focus goes back to the tree so Space keeps toggling coins.updateView()callsresetAnchor()when it rebuilds the tree, so no stale item pointer survives.Works in both tree mode and list mode.
How Has This Been Tested?
New
CoinControlTreeWidgetTestsintest_dash-qtdrive the widget with real mouse clicks on the checkbox indicator. They cover checking and unchecking a range in both directions, clicks outside the checkbox, locked coins, collapsed groups, an anchor hidden by collapsing its group,resetAnchor(), and keyboard focus after a range. Mutation check: with the previous head's widget code,hiddenAnchorReplacedandrangeKeepsKeyboardFocusfail.On macOS arm64 (
--enable-debug --enable-werror),QT_QPA_PLATFORM=cocoa src/qt/test/test_dash-qtpasses the new suite.SharedMnWalkthroughTests::walkthroughtimes out under cocoa on this headless machine; that is unrelated and happens on develop too. On macOS the suite skips itself with theminimalplatform (QTBUG-49686), like the other widget tests. Linux CI runs it underminimal.Breaking Changes
None
Checklist:
🤖 Generated with Claude Code