Refactor P2PKH filtering, via AddressBookSortFilterProxyModel #157

pull luke-jr wants to merge 4 commits into bitcoin-core:master from luke-jr:gui_filter_p2pkh changing 7 files +35 −32

luke-jr commented at 5:51 PM on December 18, 2020: member

This didn't accomplish what I hoped, but maybe worth including as a refactor anyway.

There's obviously further improvement possible, but this already seems like quite a bit for one refactoring step.
AddressTableModel: Keep track of whether addresses are P2PKH 6604abe6ab
GUI: Support filtering by P2PKH-only in AddressBookSortFilterProxyModel 86be9e4a35
GUI: Use AddressBookPage to filter for P2PKH-only in Sign Message addressbook c0308b4ab7
GUI: Drop dead filtering code from AddressTableModel 3fe03461da
hebasto commented at 8:43 PM on December 26, 2020: member

This didn't accomplish what I hoped...

What do you hope?
luke-jr commented at 10:27 PM on December 26, 2020: member

What do you hope?

I'd like the new "signmessage for p2pkh only" to only show if there are actually non-p2pkh addresses being hidden.
DrahtBot commented at 4:42 AM on January 11, 2021: contributor



🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
DrahtBot added the label Needs rebase on Jan 11, 2021

in src/qt/addresstablemodel.cpp:93 in 6604abe6ab outdated

  88 | @@ -88,7 +89,8 @@ class AddressTablePriv
  89 |                          QString::fromStdString(address.purpose), address.is_mine);
  90 |                  cachedAddressTable.append(AddressTableEntry(addressType,
  91 |                                    QString::fromStdString(address.name),
  92 | -                                  QString::fromStdString(EncodeDestination(address.dest))));
  93 | +                                  QString::fromStdString(EncodeDestination(address.dest)),
  94 | +                                  (address.dest.type() == typeid(PKHash))));

promag commented at 9:08 PM on January 27, 2021:

6604abe6ab25925b379b3f16630906de3aaf51fb

I think you could do std::get_if<PKHash>(address.dest) != nullptr. Same below, and drop <typeinfo> header.

nit, commit prefix should be qt?

Talkless commented at 1:20 PM on March 13, 2021:

There's std::holds_alternative for that.

in src/qt/addressbookpage.cpp:153 in 86be9e4a35 outdated
```
 149 | @@ -145,14 +150,14 @@ AddressBookPage::~AddressBookPage()
 150 |      delete ui;
 151 |  }
 152 |  
 153 | -void AddressBookPage::setModel(AddressTableModel *_model)
 154 | +void AddressBookPage::setModel(AddressTableModel *_model, const bool p2pkh_only)
```
promag commented at 9:20 PM on January 27, 2021:

86be9e4a353f8c2ab35c3c1177e8ad63b8e50eab

nit, this could be renamed AddressBookPage::setupModel(AddressTableModel* source_model, const bool p2pkh_only). Alternatively these could be passed in AddressBookPage constructor and this method would be private.

jarolrod commented at 4:17 AM on July 6, 2021:

This suggestion looks reasonable to me, @luke-jr will you get back to this?
hebasto commented at 4:08 AM on March 24, 2021: member

Concept ACK. Rebasing?
hebasto added the label Refactoring on Apr 19, 2021
hebasto added the label Wallet on Apr 19, 2021
hebasto added the label Waiting for author on Apr 20, 2021
hebasto commented at 11:15 AM on September 29, 2021: member

Closing due to PR author's inactivity. Labeled "Up for grabs".
hebasto closed this on Sep 29, 2021
hebasto added the label Up for grabs on Sep 29, 2021
hebasto removed the label Waiting for author on Sep 29, 2021
bitcoin-core locked this on Sep 29, 2022

Contributors

Labels

Refactoring Needs rebase Up for grabs Wallet