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

pablomartin4btc · 2023-09-10T05:32:46Z

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.

DrahtBot · 2023-09-10T05:32:49Z

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.

MarnixCroes

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 · 2023-09-12T19:36:43Z

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 · 2023-09-13T00:32:58Z

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

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

master

PR

pablomartin4btc · 2023-09-13T15:26:02Z

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 · 2023-09-14T14:24:13Z

@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

tACK 4b00cdc

pablomartin4btc · 2023-09-15T14:41:54Z

@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:

gui/src/qt/addressbookpage.cpp

Lines 320 to 324 in 4b00cdc

    
               QString filename = GUIUtil::getSaveFileName(this, 
        
                   tr("Export Address List"), QString(), 
        
                   /*: Expanded name of the CSV file format. 
        
                       See: https://en.wikipedia.org/wiki/Comma-separated_values. */ 
        
                   tr("Comma separated file") + QLatin1String(" (*.csv)"), nullptr);

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.

hernanmarino

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

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.

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.

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.

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.

pablomartin4btc · 2023-10-07T00:44:40Z

Rebased.

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.

hernanmarino

cr ACK and tested ACK

pablomartin4btc · 2024-03-27T21:14:11Z

@luke-jr would you be interested on looking at this if you have some time?

luke-jr · 2024-03-28T00:50:03Z

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 · 2024-04-07T21:32:14Z

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.

Personally, I'd add the column on the "Requested payments history" as well (or making it optional, adding previously a feature to show/ hide columns, feature that could be more useful on the Peers table).

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

Concept ACK

Nice solution adding the AddressType column and the filter by AddressType

BrandonOdiwuor · 2024-04-12T13:18:33Z

src/qt/addressbookpage.cpp

    ui->tableView->sortByColumn(0, Qt::AscendingOrder);

    // Set column widths
-    ui->tableView->horizontalHeader()->setSectionResizeMode(AddressTableModel::Label, QHeaderView::Stretch);
+    ui->tableView->horizontalHeader()->setSectionResizeMode(AddressTableModel::Label, QHeaderView::ResizeToContents);


Why this change?

from this

to this

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 · 2024-08-14T14:45:21Z

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

DrahtBot · 2024-11-11T01:22:19Z

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

DrahtBot added the CI failed label Sep 10, 2023

pablomartin4btc force-pushed the gui-address-types-on-receiving-tab branch 3 times, most recently from af3c781 to a204706 Compare September 11, 2023 23:25

DrahtBot removed the CI failed label Sep 12, 2023

MarnixCroes reviewed Sep 12, 2023

View reviewed changes

pablomartin4btc force-pushed the gui-address-types-on-receiving-tab branch from a204706 to 4b00cdc Compare September 12, 2023 22:48

MarnixCroes reviewed Sep 13, 2023

View reviewed changes

DrahtBot mentioned this pull request Sep 14, 2023

Add wallet name to address book page title #757

Merged

MarnixCroes approved these changes Sep 14, 2023

View reviewed changes

DrahtBot added the Needs rebase label Oct 4, 2023

hernanmarino approved these changes Oct 6, 2023

View reviewed changes

DrahtBot requested a review from hernanmarino October 6, 2023 21:30

pablomartin4btc added 4 commits October 6, 2023 21:26

pablomartin4btc force-pushed the gui-address-types-on-receiving-tab branch from 4b00cdc to dee0b3d Compare October 7, 2023 00:43

pablomartin4btc force-pushed the gui-address-types-on-receiving-tab branch from dee0b3d to 6eb2511 Compare October 7, 2023 01:10

DrahtBot added the CI failed label Oct 7, 2023

pablomartin4btc force-pushed the gui-address-types-on-receiving-tab branch from 6eb2511 to 4ce72d6 Compare October 7, 2023 01:24

pablomartin4btc force-pushed the gui-address-types-on-receiving-tab branch from 4ce72d6 to dffc37f Compare October 7, 2023 01:36

DrahtBot removed CI failed Needs rebase labels Oct 7, 2023

hernanmarino approved these changes Oct 10, 2023

View reviewed changes

DrahtBot added the CI failed label Jan 14, 2024

BrandonOdiwuor reviewed Apr 12, 2024

View reviewed changes

This was referenced Jun 11, 2024

Migrate legacy wallets that are not loaded #824

Merged

Remove the legacy wallet and BDB dependency bitcoin/bitcoin#28710

Draft

DrahtBot mentioned this pull request Jul 31, 2024

GUIUtil::brintToFront workaround for Wayland #831

Merged

DrahtBot added the Needs rebase label Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pablomartin4btc commented Sep 10, 2023 •

edited

Loading

DrahtBot commented Sep 10, 2023 •

edited

Loading

MarnixCroes left a comment

pablomartin4btc commented Sep 12, 2023

pablomartin4btc commented Sep 13, 2023 •

edited

Loading

MarnixCroes left a comment

pablomartin4btc commented Sep 13, 2023

MarnixCroes commented Sep 14, 2023

MarnixCroes left a comment

pablomartin4btc commented Sep 15, 2023

hernanmarino left a comment

pablomartin4btc commented Oct 7, 2023

hernanmarino left a comment

pablomartin4btc commented Mar 27, 2024

luke-jr commented Mar 28, 2024

pablomartin4btc commented Apr 7, 2024

BrandonOdiwuor left a comment

BrandonOdiwuor Apr 12, 2024

pablomartin4btc Apr 12, 2024

DrahtBot commented Aug 14, 2024

DrahtBot commented Nov 11, 2024

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

Are you sure you want to change the base?

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

Conversation

pablomartin4btc commented Sep 10, 2023 • edited Loading

DrahtBot commented Sep 10, 2023 • edited Loading

Reviews

Conflicts

MarnixCroes left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Sep 12, 2023

pablomartin4btc commented Sep 13, 2023 • edited Loading

MarnixCroes left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Sep 13, 2023

MarnixCroes commented Sep 14, 2023

MarnixCroes left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Sep 15, 2023

hernanmarino left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Oct 7, 2023

hernanmarino left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Mar 27, 2024

luke-jr commented Mar 28, 2024

pablomartin4btc commented Apr 7, 2024

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

BrandonOdiwuor Apr 12, 2024

Choose a reason for hiding this comment

pablomartin4btc Apr 12, 2024

Choose a reason for hiding this comment

DrahtBot commented Aug 14, 2024

DrahtBot commented Nov 11, 2024

pablomartin4btc commented Sep 10, 2023 •

edited

Loading

DrahtBot commented Sep 10, 2023 •

edited

Loading

pablomartin4btc commented Sep 13, 2023 •

edited

Loading