feat(qt): add shift-click range selection to coin control dialog#7260
feat(qt): add shift-click range selection to coin control dialog#7260PastaPastaPasta wants to merge 3 commits intodashpay:developfrom
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 👍 / 👎.
src/qt/coincontroltreewidget.cpp
Outdated
| 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 👍 / 👎.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
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)
✏️ 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: 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
📒 Files selected for processing (4)
src/qt/coincontroldialog.cppsrc/qt/coincontroldialog.hsrc/qt/coincontroltreewidget.cppsrc/qt/coincontroltreewidget.h
src/qt/coincontroltreewidget.cpp
Outdated
| // 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.
PastaPastaPasta
force-pushed
the
feat/coin-control-shift-select
branch
from
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
📒 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
Override mouse events in CoinControlTreeWidget to support shift-click for selecting/deselecting a contiguous range of UTXOs. Tracks an anchor item from the last normal click, and on shift-click sets all leaf items between anchor and target to the anchor's check state. Uses the existing setEnabled(false) bulk-update pattern to suppress per-item label recalculation.
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
PastaPastaPasta
force-pushed
the
feat/coin-control-shift-select
branch
from
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".
src/qt/coincontroltreewidget.cpp
Outdated
| 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 👍 / 👎.
src/qt/coincontroltreewidget.cpp
Outdated
| 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
📒 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
…enabled items Only record the shift-click anchor on left-button clicks to enabled (non-locked) leaf items, matching standard UI behavior. Also guard the shift-click path so it falls back to normal click handling if either endpoint has become disabled since being recorded.
Review GateCommit:
|
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
📒 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: 912f2ad
🟡 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)`.
2 participants
Issue being fixed or feature implemented
Users with many UTXOs must click each coin individually in the coin control dialog. This adds standard shift-click range selection, allowing a contiguous block of coins to be selected or deselected in one action.
What was done?
Overrode
mousePressEventandmouseReleaseEventinCoinControlTreeWidgetto support shift-click range selection:setEnabled(false)bulk-update pattern to suppress per-itemupdateLabels()calls, with a singlerefreshLabels()call after the batchresetAnchor()whenupdateView()rebuilds the tree to prevent dangling pointersHow Has This Been Tested?
Manual testing:
Breaking Changes
None
Checklist: