qt: Remove hidden columns in coin control dialog #14828

pull promag wants to merge 1 commits into bitcoin:master from promag:2018-11-coincontroldialog changing 3 files +17 −14

promag commented at 12:26 PM on November 28, 2018: member

Instead of having hidden columns, store the data in specific roles.

Overlaps with #14817, fixes #11811.
promag commented at 12:32 PM on November 28, 2018: member

Ah @laanwj already suggested this approach in #11811 (comment)
fanquake added the label GUI on Nov 28, 2018
DrahtBot commented at 12:37 PM on November 28, 2018: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #14817 (qt: Remove unnecessary columns in Coin Selection window (#11811) by hebasto)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
hebasto commented at 2:30 PM on November 28, 2018: member

Concept ACK.

in src/qt/coincontroldialog.cpp:269 in 6d406be79c outdated

 262 | @@ -265,17 +263,17 @@ void CoinControlDialog::lockCoin()
 263 |      if (contextMenuItem->checkState(COLUMN_CHECKBOX) == Qt::Checked)
 264 |          contextMenuItem->setCheckState(COLUMN_CHECKBOX, Qt::Unchecked);
 265 |  
 266 | -    COutPoint outpt(uint256S(contextMenuItem->text(COLUMN_TXHASH).toStdString()), contextMenuItem->text(COLUMN_VOUT_INDEX).toUInt());
 267 | +    COutPoint outpt(uint256S(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());
 268 |      model->wallet().lockCoin(outpt);
 269 |      contextMenuItem->setDisabled(true);
 270 | -    contextMenuItem->setIcon(COLUMN_CHECKBOX, platformStyle->SingleColorIcon(":/icons/lock_closed"));
 271 | +    contextMenuItem->setIcon(COLUMN_CHECKBOX, platformStyle->SingleColorIcon(":/i cons/lock_closed"));

hebasto commented at 2:34 PM on November 28, 2018:

What does this extra space mean?

promag commented at 2:40 PM on November 28, 2018:

Ops, ty.

promag force-pushed on Nov 28, 2018
hebasto commented at 7:20 PM on November 28, 2018: member
There are some concerns about 4cd9a3a772807df825c27c410410c3ea96b59371 commit.

QTreeView::uniformRowHeights:

This property should only be set to true if it is guaranteed that all items in the view has the same height. This enables the view to do some optimizations.
1. IMO, it is not a good idea to mix a bugfix commit with an optimization commit in one PR (see Pull Request Philosophy).
2. There is no obvious need for such optimization.
3. How can 'the same height' condition be guaranteed now and then?
4. How can I test this optimization and measure the effect of it?
Could this optimization commit be moved out of this PR's scope?
promag force-pushed on Nov 28, 2018
promag commented at 8:32 PM on November 28, 2018: member

@hebasto removed commit.

in src/qt/coincontroldialog.cpp:697 in 06cc0412da outdated

 690 | @@ -693,10 +691,10 @@ void CoinControlDialog::updateView()
 691 |              itemOutput->setData(COLUMN_CONFIRMATIONS, Qt::UserRole, QVariant((qlonglong)out.depth_in_main_chain));
 692 |  
 693 |              // transaction hash
 694 | -            itemOutput->setText(COLUMN_TXHASH, QString::fromStdString(output.hash.GetHex()));
 695 | +            itemOutput->setData(COLUMN_ADDRESS, TxHashRole, QString::fromStdString(output.hash.GetHex()));
 696 |  
 697 |              // vout index
 698 | -            itemOutput->setText(COLUMN_VOUT_INDEX, QString::number(output.n));
 699 | +            itemOutput->setData(COLUMN_ADDRESS, VOutRole, uint(output.n));

hebasto commented at 7:24 AM on November 29, 2018:

Nit. uint() conversion is redundant: https://github.com/bitcoin/bitcoin/blob/bc60c615a529728986ffdb890faec6e241b61536/src/primitives/transaction.h#L22

hebasto changes_requested
hebasto commented at 7:34 AM on November 29, 2018: member

tACK 06cc0412da8fc3d031e804e7df0ae5b9686663b0 after nit
qt: Remove hidden columns in coin control dialog 1c28feb7d0
promag force-pushed on Nov 29, 2018
laanwj commented at 4:26 PM on November 29, 2018: member

Ah @laanwj already suggested this approach in #11811 (comment)

Yes, using column metadata is definitely the more idiomatic approach, thanks for doing this.

utACK 1c28feb7d026bc18304149d96f81888059d52670
hebasto commented at 5:17 PM on November 29, 2018: member

re- tACK 1c28feb7d026bc18304149d96f81888059d52670
jonasschnelli commented at 8:19 AM on December 4, 2018: contributor

Tested ACK 1c28feb7d026bc18304149d96f81888059d52670
jonasschnelli merged this on Dec 4, 2018
jonasschnelli closed this on Dec 4, 2018
jonasschnelli referenced this in commit 64fc7c0c1b on Dec 4, 2018
UdjinM6 referenced this in commit 9f02792ee5 on Sep 13, 2020
xdustinface referenced this in commit b7bbbd1335 on Sep 16, 2020
xdustinface referenced this in commit c76a737730 on Sep 16, 2020
UdjinM6 referenced this in commit b10dc1f547 on Sep 23, 2020
PastaPastaPasta referenced this in commit f4e3bf95d0 on Sep 24, 2020
jasonbcox referenced this in commit 75362b2bb4 on Oct 8, 2020
ckti referenced this in commit 3ed47f402c on Mar 28, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#11811 Bitcoin-Qt Coin Selection window has unnecessary columns "9" and "10"#14817 qt: Remove unnecessary columns in Coin Selection window (#11811)

qt: Remove hidden columns in coin control dialog #14828

Conflicts