Add new "address type" column to the "receiving tab" address book page

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.

Peek 2023-09-10 19-25

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.

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.

DrahtBot added the label CI failed on Sep 10, 2023

pablomartin4btc force-pushed on Sep 10, 2023

pablomartin4btc force-pushed on Sep 11, 2023

DrahtBot removed the label CI failed on Sep 12, 2023

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

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.

pablomartin4btc force-pushed on Sep 12, 2023

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”.

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 Screenshot from 2023-09-13 14-38-15

PR Screenshot from 2023-09-13 14-39-14

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.

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

MarnixCroes approved

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

tACK 4b00cdc6fd9109a6b1094b936e175e9909155857

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.

DrahtBot added the label Needs rebase on Oct 4, 2023

hernanmarino approved

hernanmarino commented at 9:30 pm on October 6, 2023: contributor

Concept ACK, it looks like a nice functionality. Will test after rebase

DrahtBot requested review from hernanmarino on Oct 6, 2023

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

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

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

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

pablomartin4btc force-pushed on Oct 7, 2023

pablomartin4btc commented at 0:44 am on October 7, 2023: contributor

Rebased.

pablomartin4btc force-pushed on Oct 7, 2023

DrahtBot added the label CI failed on Oct 7, 2023

pablomartin4btc force-pushed on Oct 7, 2023

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

pablomartin4btc force-pushed on Oct 7, 2023

DrahtBot removed the label CI failed on Oct 7, 2023

DrahtBot removed the label Needs rebase on Oct 8, 2023

hernanmarino approved

hernanmarino commented at 8:47 pm on October 10, 2023: contributor

cr ACK and tested ACK

DrahtBot added the label CI failed on Jan 14, 2024

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?

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?

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.

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

Concept ACK

Nice solution adding the AddressType column and the filter by AddressType

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 Screenshot from 2024-04-12 16-02-55

to this Screenshot from 2024-04-12 16-03-38

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!

DrahtBot added the label Needs rebase on Aug 14, 2024

DrahtBot commented at 2:45 pm on August 14, 2024: contributor

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

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

Reviews

Conflicts