Add new “address type” column to the “receiving tab” address book page #753

1. 
   pablomartin4btc commented at 5:32 am on September 10, 2023: contributor

   This PR fixes #646 and introduces some enhancements to the Address Book functionality.

   A new “Address Type” column has been added to the address book table page, only visible for the “Receiving address” tab. Users can employ an also new added combo box at the bottom of the page to filter address by their type, this filtering can be combined with the current search line text box. When the export feature is used, this new field will be included.

   

   As highlighted with the performed manual testing shown in the image above:

   • address type combo box has been reused from receive coins dialog, but each widget container can specify different tool-tips for their items;
   • “Sending address” tab doesn’t display the new column, combo box or its label, and the search text specify only the fields to be filtered out;
   • “Receiving address” tab display the new added widgets, filter works fine in combination of either text, combo item selection or combination of both, tool-tip texts differs from the ones on the receive coins dialog where the address can be created specifying their types.

   Tested in mainnet and all test networks.

   For code reviewers, please check the commit messages for a detailed breakdown.

2. 
   DrahtBot commented at 5:32 am on September 10, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	hernanmarino, BrandonOdiwuor
   Stale ACK	MarnixCroes

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #824 (Migrate legacy wallets that are not loaded by achow101)

   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.

3. DrahtBot added the label CI failed on Sep 10, 2023
4. pablomartin4btc force-pushed on Sep 10, 2023
5. pablomartin4btc force-pushed on Sep 11, 2023
6. pablomartin4btc force-pushed on Sep 11, 2023
7. DrahtBot removed the label CI failed on Sep 12, 2023
8. 
   MarnixCroes commented at 8:59 am on September 12, 2023: contributor

   cACK, nice it jumps weird when having it sorted on address type and then changing the address type.

   Screencast from 12-09-2023 10:55:01.webm

9. 
   pablomartin4btc commented at 7:36 pm on September 12, 2023: contributor

   it jumps weird when having it sorted on address type and then changing the address type.

   Yeah, it seems that that’s due to the resize mode configuration, it was something that was already happening:

   

   I’ll check if it can be fixed.

10. pablomartin4btc force-pushed on Sep 12, 2023
11. 
    pablomartin4btc commented at 0:32 am on September 13, 2023: contributor

    Fixed the issue reported by @MarnixCroes above where the columns were resizing incorrectly.

    A follow up could be to set a default width value for each column or most of them and have the feature of storing the user preferable width as in the peers table in the “Node window”.

    

12. 
    MarnixCroes commented at 12:41 pm on September 13, 2023: contributor

    The Export button works different. It doesn’t open an in app dialog

    master

    PR

13. 
    pablomartin4btc commented at 3:26 pm on September 13, 2023: contributor

    Thanks for checking that also @MarnixCroes.

    That looks weird, the way the export button works or its functionality behind it hasn’t been even touched. For me, on master and with any network passed at the startup works the same, I always get the 2nd dialog you posted.

    I’ll try it later on a different OS. Did the export actually worked for you? Another thing I could think of is that perhaps the new filter model has some particular behavior but I can imaging that would be used for the persistence, not for the file dialog itself, could you please try testing it just up to the 3rd. commit (that’s where the new column is added but not the filter & combo-box)? On the folder you have this PR’s code you can do it by running git checkout 95d5b3c65cc6be67eb270e5ddd5e8abf068fd0f3.

14. 
    MarnixCroes commented at 2:24 pm on September 14, 2023: contributor

    @pablomartin4btc Please ignore my previous comment, I cannot repro it on master or other PR commits, sorry my bad.

    Did the export actually worked for you?

    yes it does

15. MarnixCroes approved
16. 
    MarnixCroes commented at 2:26 pm on September 14, 2023: contributor

    tACK 4b00cdc6fd9109a6b1094b936e175e9909155857
17. 
    pablomartin4btc commented at 2:41 pm on September 15, 2023: contributor

    @pablomartin4btc Please ignore my previous comment, I cannot repro it on master or other PR commits, sorry my bad.

    Don’t worry, it was a good spot. This is the part of the code that opens the “Save file…” (app) dialog and as you can see the title that you were seeing in your first screenshot for master, “Export Address List” is being set there: https://github.com/bitcoin-core/gui/blob/4b00cdc6fd9109a6b1094b936e175e9909155857/src/qt/addressbookpage.cpp#L320-L324 That code hasn’t been touched by this PR.

    I couldn’t reproduced it in either master or this branch in both Ubuntu 20.04 or 22.04 but I crossed compiled it in WSL and run bitcoin-qt in Windows 11 Pro and you can see the “Export Address List” dialog in both master and this PR (you can note that master doesn’t have the new column address type in the receiving window):

    master:

    this PR:

    So this PR hasn’t interfered with the way that on the export feature the save file dialog is getting open or displayed.

18. DrahtBot added the label Needs rebase on Oct 4, 2023
19. hernanmarino approved
20. 
    hernanmarino commented at 9:30 pm on October 6, 2023: contributor

    Concept ACK, it looks like a nice functionality. Will test after rebase
21. DrahtBot requested review from hernanmarino on Oct 6, 2023
22. gui: Refactor AddressType from ReceiveCoinsDialog

    Extract and refactor ui->addressType combo content filling into GUIUtil,
    having the translated descriptions in one place and allowing other Qt widget
    containers to easily create similar combo boxes with customizable tooltips
    per item and default selected item, both specified by the caller.

    bc0e33f604
23. wallet: Correct output type for Segwit address

    Current OutputTypeFromDestination at outputtype, returns OUTPUT_TYPE_STRING_LEGACY
    for Segwit addresses, as p2sh-segwit requires extra info from the wallet to figure
    that out, this change adds a correct way to obtain the desired result from the
    wallet interface. So far this will be needed from the UI to identify such type,
    as currently a user could select this output type to create an address (receivecoinsdialog)
    but no display of it exists.

    90284c7e35
24. gui: Add Address Type Column to AddressBookPage

    Add new address type column to the addresstablemodel, use the getOutputType
    function from the wallet interface to display the correct address type in the
    new column content. Update the address book page to set the new column resize mode
    and to display the new column only when the receiving tab is enabled
    so it must be hidden on the sending tab.
    
    Update AddressBookFilterProxyModel::filterAcceptsRow so the filter works also
    on the new addressType column content. Also the searLineEdit greyed text
    reflects that the new field/ column addressType will be included in the search/
    filter by but only when the receiving tab is enabled.
    
    Add the new column to the export feature.

    7be6b792ea
25. gui: Extend address book filter for nested filtering

    Extend AddressBookFilterProxyModel to allow using nested filters to be applied
    on top of it. If future needs arise for similar filters outside the address book page,
    the code could be easily refactored and moved in a new subclass of QSortFilterProxyModel,
    perhaps with limits on nested levels.
    
    For safety and performance reasons, the code of the filter proxy model class declaration
    (in addressbookpage.h) and its instance creation were updated by aligning it with
    TransactionFilterProxy in overviewpage.h as this addresses situations of unexpected crashes,
    such as segfault on closing the app, double free or corruption, or stack smashing detection.

    5d597c0b3d
26. pablomartin4btc force-pushed on Oct 7, 2023
27. 
    pablomartin4btc commented at 0:44 am on October 7, 2023: contributor

    Rebased.
28. pablomartin4btc force-pushed on Oct 7, 2023
29. DrahtBot added the label CI failed on Oct 7, 2023
30. pablomartin4btc force-pushed on Oct 7, 2023
31. gui: Add a combo widget to filter by address type

    Introduce a label and a combobox UI widgets for address type filtering on the address book page.
    These 2 widgets are been displayed only on the Receiving tab.
    To ensure that the combo box selection updates the filter model correctly, an intermediary signal
    (addressTypeChanged) and a slot (handleAddressTypeChanged) were necessary due to the incompatibility
    between QComboBox::currentIndexChanged and QSortFilterProxyModel::setFilterFixedString.
    
    Update filter model to use nested filtering so when selected item changes on address type combo
    it will update the filter pattern on top of what the parent filter has already, which is lead by the
    searchLineEdit widget and all references of the current proxyModel (eg mapToSource, mapFromSource)
    to the nested and final filter model.
    
    Use GUIUtil::AddItemsToAddressTypeCombo to populate the combo with the default output types descriptions
    passing a defined local map for the output types tooltips that will be displayed for each item,
    similarly to the address types combobox in receivecoinsdialog.

    dffc37f15c
32. pablomartin4btc force-pushed on Oct 7, 2023
33. DrahtBot removed the label CI failed on Oct 7, 2023
34. DrahtBot removed the label Needs rebase on Oct 8, 2023
35. hernanmarino approved
36. 
    hernanmarino commented at 8:47 pm on October 10, 2023: contributor

    cr ACK and tested ACK
37. DrahtBot added the label CI failed on Jan 14, 2024
38. 
    pablomartin4btc commented at 9:14 pm on March 27, 2024: contributor

    @luke-jr would you be interested on looking at this if you have some time?
39. 
    luke-jr commented at 0:50 am on March 28, 2024: member

    I don’t know how useful this is. Why would users care to see only a certain type, or to see what type addresses are like this?
40. 
    pablomartin4btc commented at 9:32 pm on April 7, 2024: contributor

    I don’t know how useful this is. Why would users care to see only a certain type, or to see what type addresses are like this?

    Well, since QT offers to the user the feature to create certain types, it’d make sense to list them properly identifying the original intention.

    

    It’d have been good to discuss it on the issue #646 itself, which has been moved from the core repo, perhaps I should’ve asked before working on it. However, given that it’s already developed, I don’t think it would hurt to merge it once it gets approved accordingly, of course.

41. 
    BrandonOdiwuor commented at 1:17 pm on April 12, 2024: contributor

    Concept ACK

    Nice solution adding the AddressType column and the filter by AddressType

42. in src/qt/addressbookpage.cpp:175 in dffc37f15c

    174+    ui->tableView->setModel(proxyModel->nestedProxyModel());
    175     ui->tableView->sortByColumn(0, Qt::AscendingOrder);
    176 
    177     // Set column widths
    178-    ui->tableView->horizontalHeader()->setSectionResizeMode(AddressTableModel::Label, QHeaderView::Stretch);
    179+    ui->tableView->horizontalHeader()->setSectionResizeMode(AddressTableModel::Label, QHeaderView::ResizeToContents);
    

    ---

    ---

    

    BrandonOdiwuor commented at 1:18 pm on April 12, 2024:

    Why this change?

    from this

    to this

    ---

    

    pablomartin4btc commented at 7:15 pm on April 12, 2024:

    Good point, this is something that could be subject for discussion, I’m open to any suggestions. Originally, as for this change, I decided to change the resize mode from Stretch to ResizeToContents as it looked tighter to me (resize mode enum - qt doc ref). I was thinking to make a bit of a mix of both of them, Stretch for some columns and ResizeToContents to others (e.g. check here), but I see there are other devs that also calculate the size of certain columns (in the latest link they also mentioned the property setStretchLastSection, which extends the size of the last column to cover the rest of the table so there’s no gap as you see in your 2nd screenshot, and I played a bit with but I finally discarded it cos there was an unexpected behaviour that can’t remember now). We could also allow users resize each column as their preference and save them in the settings (I think this is done on the “Peers” table).

    Please let me know your opinion on this or if you have any suggestions. Thanks!

43. DrahtBot added the label Needs rebase on Aug 14, 2024
44. 
    DrahtBot commented at 2:45 pm on August 14, 2024: contributor

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

45. 
    DrahtBot commented at 1:22 am on November 11, 2024: contributor

    ⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

Contributors
pablomartin4btc DrahtBot MarnixCroes hernanmarino luke-jr BrandonOdiwuor

Labels
Needs rebase CI failed