Skip to content
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.

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

Multiselect in coincontrol treewidget and display selected count #11750

Closed
wants to merge 1 commit into from

Conversation

@Elbandi
Copy link
Contributor

@Elbandi Elbandi commented Nov 22, 2017

This patch allow multiple out selection in coincontrol dialog.
Only for changing the checked state, locking need single select.

usecase: someone gets lost of payments from zpool/miningrigrentals/nicehash/etc, its easy to select inputs for sending (no need to click many times in intems or space-down-space-down-space-down... by keyboard)

Copy link
Member

@promag promag left a comment

Care to upload a screenshot with the diff?

src/qt/coincontroltreewidget.cpp Outdated
@@ -8,17 +8,18 @@
CoinControlTreeWidget::CoinControlTreeWidget(QWidget *parent) :
QTreeWidget(parent)
{

setSelectionMode( QAbstractItemView::ExtendedSelection);

This comment has been minimized.

@promag

promag Nov 22, 2017
Member

Remove space after (.

src/qt/coincontroldialog.cpp Outdated
void CoinControlDialog::updateLabelSelected()
{
QList<QTreeWidgetItem *> selected = ui->treeWidget->selectedItems();
if (selected.size() > 0)

This comment has been minimized.

@promag

promag Nov 22, 2017
Member

How about:

int count = selected.size();
ui->labelSelected->setVisible(count > 0);
ui->labelSelected->setText(tr("(%1 selected)").arg(count));

This comment has been minimized.

@Elbandi

Elbandi Nov 22, 2017
Author Contributor

It hides the "(X selected)" label if no selection. like locked count

This comment has been minimized.

@promag

promag Nov 22, 2017
Member

ui->labelSelected->setVisible(count > 0); should hide when there's no selection.

<item>
<widget class="QLabel" name="labelSelected">
<property name="text">
<string notr="true">(1 selected)</string>

This comment has been minimized.

@promag

promag Nov 22, 2017
Member

Incorrect?

This comment has been minimized.

@Elbandi

Elbandi Nov 22, 2017
Author Contributor

???,
placeholder string, as in labelLocked

This comment has been minimized.

@promag

promag Nov 22, 2017
Member

Don't know why that is there too, and I think there is no special reason. I guess it's ok since it is consistent with others.

src/qt/coincontroltreewidget.cpp Outdated
}

void CoinControlTreeWidget::keyPressEvent(QKeyEvent *event)
{
if (event->key() == Qt::Key_Space) // press spacebar -> select checkbox
{
event->ignore();
if (this->currentItem()) {
for (int i = 0; i < this->selectedItems().count(); i++) {

This comment has been minimized.

@promag

promag Nov 22, 2017
Member

for (QTreeWidgetItem* item : selectedItems()) {
src/qt/coincontroltreewidget.cpp Outdated
@@ -8,17 +8,18 @@
CoinControlTreeWidget::CoinControlTreeWidget(QWidget *parent) :
QTreeWidget(parent)
{

setSelectionMode( QAbstractItemView::ExtendedSelection);
}

void CoinControlTreeWidget::keyPressEvent(QKeyEvent *event)
{
if (event->key() == Qt::Key_Space) // press spacebar -> select checkbox

This comment has been minimized.

@promag

promag Nov 22, 2017
Member

Comment should be "press spacebar -> toggle checkbox"?

@Elbandi Elbandi force-pushed the Elbandi:ccmultiselect branch to 7cec76f Nov 23, 2017
@Elbandi
Copy link
Contributor Author

@Elbandi Elbandi commented Nov 23, 2017

new commit pushed

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Nov 29, 2017

Tested a bit.
The multiselection works well.

I got confused by the selection count indicator label.
It's confusing because you have the first button "(un)select all" where the new "(1) selected" has a different context for "selected".

Maybe remove the label (just add the multiselection possibility) or use a different help text for the new label... though I don't think it's necessary to have a such label (rarely saw this in multiselection use-cases).

@Elbandi
Copy link
Contributor Author

@Elbandi Elbandi commented Nov 30, 2017

Rename the button to "(un)check all", eventually thats the right function.

@laanwj
Copy link
Member

@laanwj laanwj commented Feb 12, 2018

It's confusing because you have the first button "(un)select all" where the new "(1) selected" has a different context for "selected".

I have the same opinion here; the coin selection is already inherently multi-select, as multiple outputs can be checked. So adding another layer of multi-selection on top is confusing. It's no longer clear whether selection refers to "toggled checkbox" or "selected row".

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Feb 26, 2018

I agree this is confusing, yet also see the validity of the use case.

Maybe @jonasschnelli 's idea + renaming the button to "(Un)check all" would be sufficient.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Apr 10, 2018

I think we should remove the new label. Just allow the spacebar toggling. Seems much more clear to me then confusing with "selection" (multiple contexts of selection)

@laanwj laanwj added the Up for grabs label May 14, 2018
@laanwj
Copy link
Member

@laanwj laanwj commented May 14, 2018

Last post by the author was some time last year. Going to add "Up for grabs" label and closing.

@laanwj laanwj closed this May 14, 2018
@Elbandi
Copy link
Contributor Author

@Elbandi Elbandi commented May 17, 2018

Sorry, i didnt know, what is the latest consensus about the text?
i can change the code, just someone say "lets be this and that"

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Apr 4, 2019

@Elbandi I guess do what @jonasschnelli suggested. When you're ready, ping someone to reopen the PR before you push the new version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.