qt: Hide non PKHash-Addresses in signing address book #17918

pull emilengler wants to merge 1 commits into bitcoin:master from emilengler:2020-01-hide-non-pkhash-addresses changing 6 files +16 −5

emilengler commented at 7:01 PM on January 13, 2020: contributor

Video Demo

This PR hides all non PKHash addresses in the signing GUI in the Address Book when it is opened through the signing dialog, as non PKHash addresses are useless there.
ysangkok commented at 8:30 PM on January 13, 2020: none

PKHashOnly is a variable but is not snake-case as it should be according to dev notes.
DrahtBot added the label GUI on Jan 13, 2020
theStack commented at 12:50 PM on January 14, 2020: member

Is it really good UI design to just hide addresses? From a user perspective I'd rather prefer to see a message that explicitely tells me that bech32 address signing is not supported yet in Bitcoin Core, rather than being confused on why the addresses don't even show up in the Sign/Verify dialog in the first place.
emilengler commented at 5:26 PM on January 14, 2020: contributor

Is it really good UI design to just hide addresses?

Yes, it just shows what really matters
emilengler force-pushed on Jan 14, 2020
emilengler commented at 5:32 PM on January 14, 2020: contributor

@ysangkok Fixed
MarcoFalke commented at 6:02 PM on January 14, 2020: member

It should explicitly say in the window that non-pkh addresses are hidden?
emilengler commented at 2:58 PM on January 15, 2020: contributor

It should explicitly say in the window that non-pkh addresses are hidden?

Good idea. I just meant that the current approach isn't that good. Maybe I understood theStack wrong.
emilengler force-pushed on Jan 15, 2020

in src/qt/addresstablemodel.cpp:85 in 9cd486f807 outdated

  80 |      {
  81 |          cachedAddressTable.clear();
  82 |          {
  83 |              for (const auto& address : wallet.getAddresses())
  84 |              {
  85 | +                if (pk_hash_only && address.dest.type() != typeid(PKHash))

Empact commented at 11:24 PM on January 17, 2020:

#include <typeinfo> https://en.cppreference.com/w/cpp/language/typeid

in src/qt/signverifymessagedialog.cpp:92 in 9cd486f807 outdated
```
  88 | @@ -89,6 +89,7 @@ void SignVerifyMessageDialog::on_addressBookButton_SM_clicked()
  89 |  {
  90 |      if (model && model->getAddressTableModel())
  91 |      {
  92 | +        model->refresh(true);
```
Empact commented at 11:25 PM on January 17, 2020:

model->refresh(/* pk_hash_only */ true);

emilengler commented at 8:01 AM on January 18, 2020:

Implemented with your other suggestion in c4ea501e96363e937200bc97b8e2d78162bdb699
Empact commented at 11:26 PM on January 17, 2020: member

I'm inclined to agree with @theStack that showing disabled addresses would be more clear than a message at the top, but I could go either way.

Concept ACK
emilengler commented at 7:57 AM on January 18, 2020: contributor

I'm inclined to agree with @theStack that showing disabled addresses would be more clear than a message at the top, but I could go either way.

There is a message for it, see addressbookpage.cpp:109
qt: Hide non PKHash-Addresses in signing address book c4ea501e96
emilengler force-pushed on Jan 18, 2020

in src/qt/addressbookpage.cpp:109 in c4ea501e96

 105 | @@ -106,7 +106,7 @@ AddressBookPage::AddressBookPage(const PlatformStyle *platformStyle, Mode _mode,
 106 |          ui->newAddress->setVisible(true);
 107 |          break;
 108 |      case ReceivingTab:
 109 | -        ui->labelExplanation->setText(tr("These are your Bitcoin addresses for receiving payments. Use the 'Create new receiving address' button in the receive tab to create new addresses."));
 110 | +        ui->labelExplanation->setText(tr("These are your Bitcoin addresses for receiving payments. Use the 'Create new receiving address' button in the receive tab to create new addresses.\nSigning is only possible with addresses of the type 'legacy'."));

luke-jr commented at 7:30 PM on January 26, 2020:

Suggest breaking this into two tr calls so we 1) don't lose existing translations of the first part, and 2) can remove or change the second part without resetting translations every time.

emilengler commented at 8:04 PM on January 26, 2020:

ACK

in src/qt/walletmodel.cpp:569 in c4ea501e96
```
 562 | @@ -563,3 +563,8 @@ bool WalletModel::isMultiwallet()
 563 |  {
 564 |      return m_node.getWallets().size() > 1;
 565 |  }
 566 | +
 567 | +void WalletModel::refresh(bool pk_hash_only)
 568 | +{
 569 | +    addressTableModel = new AddressTableModel(this, pk_hash_only);
```
luke-jr commented at 7:33 PM on January 26, 2020:

I'm not sure it makes sense to use the same cache with context-dependent factors like this.

Probably would be best to keep the same AddressTableModel with all addresses, and then a FilteredAddressTableModel(AddressTableModel*) to hide them...
DrahtBot commented at 6:26 AM on May 17, 2020: 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:
- #18992 (qt: add sign and verify message support to AddressBookPage by 10xcryptodev)
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.
jonasschnelli commented at 8:22 AM on May 29, 2020: contributor

Code Review ACK c4ea501e96363e937200bc97b8e2d78162bdb699
jonasschnelli merged this on May 29, 2020
jonasschnelli closed this on May 29, 2020
sidhujag referenced this in commit 55bfc2737b on May 31, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

qt: Hide non PKHash-Addresses in signing address book #17918

Conflicts