Skip to content

feat(qt): add shift-click range selection to coin control dialog #7260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 2 commits into from
Oct 5, 2026
Merged

feat(qt): add shift-click range selection to coin control dialog #7260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7581401
feat(qt): add shift-click range selection to coin control dialog
PastaPastaPasta Oct 5, 2026
3034e98
test(qt): cover coin control shift-click range selection
PastaPastaPasta Oct 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Makefile.qttest.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ TEST_QT_MOC_CPP = \
if ENABLE_WALLET
TEST_QT_MOC_CPP += \
qt/test/moc_addressbooktests.cpp \
qt/test/moc_coincontroltreewidgettests.cpp \
qt/test/moc_mnsharesessiontests.cpp \
qt/test/moc_sharedmnwalkthroughtests.cpp \
qt/test/moc_providertransactiontests.cpp \
Expand All @@ -30,6 +31,7 @@ endif # ENABLE_WALLET
TEST_QT_H = \
qt/test/addressbooktests.h \
qt/test/apptests.h \
qt/test/coincontroltreewidgettests.h \
qt/test/mnsharesessiontests.h \
qt/test/masternodemaintenancetests.h \
qt/test/masternodetestutil.h \
Expand Down Expand Up @@ -63,6 +65,7 @@ qt_test_test_dash_qt_SOURCES = \
if ENABLE_WALLET
qt_test_test_dash_qt_SOURCES += \
qt/test/addressbooktests.cpp \
qt/test/coincontroltreewidgettests.cpp \
qt/test/masternodetestutil.cpp \
qt/test/mnsharesessiontests.cpp \
qt/test/providertransactiontests.cpp \
Expand Down
6 changes: 6 additions & 0 deletions src/qt/coincontroldialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,11 @@ CoinControlDialog::~CoinControlDialog()
delete ui;
}

void CoinControlDialog::refreshLabels()
{
CoinControlDialog::updateLabels(m_coin_control, model, this);
}

// ok button
void CoinControlDialog::buttonBoxClicked(QAbstractButton* button)
{
Expand Down Expand Up @@ -644,6 +649,7 @@ void CoinControlDialog::updateView()

bool treeMode = ui->radioTreeMode->isChecked();
ui->treeWidget->clear();
ui->treeWidget->resetAnchor();
ui->treeWidget->setEnabled(false); // performance, otherwise updateLabels would be called for every checked checkbox
ui->treeWidget->setAlternatingRowColors(!treeMode);
QFlags<Qt::ItemFlag> flgCheckbox = Qt::ItemIsSelectable | Qt::ItemIsEnabled | Qt::ItemIsUserCheckable;
Expand Down
2 changes: 2 additions & 0 deletions src/qt/coincontroldialog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ class CoinControlDialog : public QDialog
// static because also called from sendcoinsdialog
static void updateLabels(wallet::CCoinControl& m_coin_control, WalletModel*, QDialog*);

void refreshLabels();

static QList<CAmount> payAmounts;
static bool fSubtractFeeFromAmount;

Expand Down
124 changes: 124 additions & 0 deletions src/qt/coincontroltreewidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,21 @@
#include <qt/coincontroltreewidget.h>
#include <qt/coincontroldialog.h>

#include <QStyle>
#include <QStyleOptionViewItem>
#include <QTreeWidgetItemIterator>

CoinControlTreeWidget::CoinControlTreeWidget(QWidget *parent) :
QTreeWidget(parent)
{

}

void CoinControlTreeWidget::resetAnchor()
{
m_lastClickedItem = nullptr;
}

void CoinControlTreeWidget::keyPressEvent(QKeyEvent *event)
{
if (event->key() == Qt::Key_Space) // press spacebar -> select checkbox
Expand All @@ -32,3 +41,118 @@ void CoinControlTreeWidget::keyPressEvent(QKeyEvent *event)
this->QTreeWidget::keyPressEvent(event);
}
}

// Helper: check if an item is a leaf node (UTXO) by its 64-char tx hash
static bool isLeafItem(QTreeWidgetItem* item)
{
int COLUMN_ADDRESS = 3;
return item && item->data(COLUMN_ADDRESS, Qt::UserRole).toString().length() == 64;
}

bool CoinControlTreeWidget::isCheckboxClick(QTreeWidgetItem* item, const QPoint& pos) const
{
int COLUMN_CHECKBOX = 0;

if (!item || columnAt(pos.x()) != COLUMN_CHECKBOX) {
return false;
}

const QModelIndex index = indexFromItem(item, COLUMN_CHECKBOX);
if (!index.isValid()) {
return false;
}

QStyleOptionViewItem option;
option.initFrom(this);
option.rect = visualRect(index);
option.features = QStyleOptionViewItem::HasCheckIndicator;
option.checkState = item->checkState(COLUMN_CHECKBOX);

return style()->subElementRect(QStyle::SE_ItemViewItemCheckIndicator, &option, this).contains(pos);
}

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;
Comment on lines +96 to +101

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Update range anchor only on checkbox interactions

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 👍 / 👎.

}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return;
}

Comment on lines +78 to +105

@thepastaclaw thepastaclaw Mar 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  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).

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)`.

// 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);

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restrict shift-range updates to visible rows

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 👍 / 👎.

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) {
// The anchor is hidden in a collapsed group: toggle the target normally and make it the new anchor
const Qt::CheckState previousState = clickedItem->checkState(COLUMN_CHECKBOX);
QTreeWidget::mouseReleaseEvent(event);
Comment on lines +129 to +132

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Re-anchor when the previous item is no longer visible

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 👍 / 👎.

if (clickedItem->checkState(COLUMN_CHECKBOX) != previousState) {
m_lastClickedItem = clickedItem;
} else {
resetAnchor();
}
return;
Comment on lines +129 to +138

@thepastaclaw thepastaclaw Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

Suggested change
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']

@PastaPastaPasta PastaPastaPasta Oct 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thepastaclaw thepastaclaw Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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);
// Disabling the tree moved keyboard focus away from it, give it back to keep Space toggling coins
setFocus(Qt::MouseFocusReason);

// Single label update for the whole batch
CoinControlDialog* coinControlDialog = qobject_cast<CoinControlDialog*>(this->parentWidget());
if (coinControlDialog) coinControlDialog->refreshLabels();
}
Comment on lines +74 to +158

@thepastaclaw thepastaclaw Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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']

@PastaPastaPasta PastaPastaPasta Oct 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thepastaclaw thepastaclaw Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

10 changes: 9 additions & 1 deletion src/qt/coincontroltreewidget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define BITCOIN_QT_COINCONTROLTREEWIDGET_H

#include <QKeyEvent>
#include <QMouseEvent>
#include <QTreeWidget>

class CoinControlTreeWidget : public QTreeWidget
Expand All @@ -14,9 +15,16 @@ class CoinControlTreeWidget : public QTreeWidget

public:
explicit CoinControlTreeWidget(QWidget *parent = nullptr);
void resetAnchor();

protected:
virtual void keyPressEvent(QKeyEvent *event) override;
void keyPressEvent(QKeyEvent *event) override;
void mouseReleaseEvent(QMouseEvent *event) override;

private:
bool isCheckboxClick(QTreeWidgetItem* item, const QPoint& pos) const;

QTreeWidgetItem* m_lastClickedItem{nullptr};
};

#endif // BITCOIN_QT_COINCONTROLTREEWIDGET_H
Loading
Loading