Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop wildcard support for address/label search in AddressBookPage #592

Closed
wants to merge 2 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Apr 23, 2022

Wildcard support for address/label search in AddressBookPage was introduced in bitcoin/bitcoin#12080, and it is available in release binaries since v0.17.

But this feature has never being documented or mentioned in the GUI.

Thus, there are no reasons to believe that users rely on this feature.

Required for migration to Qt 6, because:

There is no direct way to do wildcard matching in QRegularExpression.

Tests included.

Based on #591, so only last 2 commits belong to this PR.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 24, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #593 (Getting ready to Qt 6 (8/n). Use QRegularExpression in AddressBookSortFilterProxyModel class by hebasto)
  • #585 (Replace QRegExp with QRegularExpression by prusnak)

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.

@luke-jr
Copy link
Member

luke-jr commented May 8, 2022

cc @promag

@luke-jr
Copy link
Member

luke-jr commented May 8, 2022

"However, the wildcardToRegularExpression method is provided to translate glob patterns into a Perl-compatible regular expression that can be used for that purpose."

Seems like it should be trivial to retain this feature, if we want to?

@laanwj
Copy link
Member

laanwj commented May 9, 2022

~0 on this. As I've said on IRC I don't particularly care for this feature to go (I don't think the entire Address Book is particularly useful, searching in the Transactions tab should cover it?), though "being compatible with Qt 6" is a strange motivation. As @luke-jr says it would still be possible by rewriting the expression.

hebasto added a commit that referenced this pull request May 9, 2022
1506913 qt, test: Add tests for `tableView` in `AddressBookPage` dialog (Hennadii Stepanov)
edae3ab qt: No need to force Qt::QueuedConnection for NotifyAddressBookChanged (Hennadii Stepanov)

Pull request description:

  This PR is a prerequisite for more thorough testing of filtering in the `AddressBookPage` class in context of #578 and #585.

  Required for #592.

ACKs for top commit:
  promag:
    Code review ACK 1506913.

Tree-SHA512: 86986d47606cbd54d813436c7afb21894e2200b6d3042a7aa0b5e84821c765bd68b14ad38a445069891ab33f2d7bcd4933b8373e14e9afb0c91f1a6ddf4da740
hebasto added 2 commits May 9, 2022 22:24
Such support was never mentioned in release notes or in the GUI.
Required for migration to Qt 6, because "There is no direct way to do
wildcard matching in QRegularExpression."
See https://doc.qt.io/qt-6/qregexp.html#porting-to-qregularexpression
@hebasto hebasto force-pushed the 220423-wildcard branch from 4600add to 555b673 Compare May 9, 2022 20:24
@hebasto
Copy link
Member Author

hebasto commented May 9, 2022

Rebased on top of the merged #591.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 9, 2022
…ssBookPage` dialog

1506913 qt, test: Add tests for `tableView` in `AddressBookPage` dialog (Hennadii Stepanov)
edae3ab qt: No need to force Qt::QueuedConnection for NotifyAddressBookChanged (Hennadii Stepanov)

Pull request description:

  This PR is a prerequisite for more thorough testing of filtering in the `AddressBookPage` class in context of bitcoin-core/gui#578 and bitcoin-core/gui#585.

  Required for bitcoin-core/gui#592.

ACKs for top commit:
  promag:
    Code review ACK 1506913.

Tree-SHA512: 86986d47606cbd54d813436c7afb21894e2200b6d3042a7aa0b5e84821c765bd68b14ad38a445069891ab33f2d7bcd4933b8373e14e9afb0c91f1a6ddf4da740
@hebasto
Copy link
Member Author

hebasto commented May 20, 2022

@luke-jr

"However, the wildcardToRegularExpression method is provided to translate glob patterns into a Perl-compatible regular expression that can be used for that purpose."

Seems like it should be trivial to retain this feature, if we want to?

Would you mind provide a working example please?

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK 555b673.

TBH I didn't think about using a wildcard in the search field, so setFilterFixedString would be sufficient.

The only issue I have is breaking someone's expectation, as this is a breaking change - using wildcard results in an empty list where before it could list some addresses. I understand it's not documented but it's available for some time. Maybe could validate the search field to disallow wildcard, like:

yle, Mode _mode,
 {
     ui->setupUi(this);

+    auto validator = new QRegularExpressionValidator(QRegularExpression("[^\\*]*"), this);
+    ui->searchLineEdit->setValidator(validator);
+
     if (!platformStyle->getImagesOnButtons()) {
         ui->newAddress->setIcon(QIcon());
         ui->copyAddress->setIcon(QIcon());

Finally, if we stick to supporting wildcard then no changes are required as explained in the comment below (but didn't test with Qt6).

@@ -143,7 +140,9 @@ void AddressBookPage::setModel(AddressTableModel *_model)
proxyModel = new AddressBookSortFilterProxyModel(type, this);
proxyModel->setSourceModel(_model);

connect(ui->searchLineEdit, &QLineEdit::textChanged, proxyModel, &QSortFilterProxyModel::setFilterWildcard);
connect(ui->searchLineEdit, &QLineEdit::textChanged, [this](const QString& pattern) {
proxyModel->setFilterFixedString(pattern);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact setFilterWildcard in Qt6 already handles the change noted in the porting documentation:

There is no direct way to do wildcard matching in QRegularExpression. However, the wildcardToRegularExpression method is provided to translate glob patterns into a Perl-compatible regular expression that can be used for that purpose.

Qt5

void QSortFilterProxyModel::setFilterFixedString(const QString &pattern)
{
    Q_D(QSortFilterProxyModel);
    d->filter_about_to_be_changed();
    QRegExp rx(pattern, d->filter_data.caseSensitivity(), QRegExp::FixedString);
    d->filter_data.setRegExp(rx);
    d->filter_changed();
}

Qt6

void QSortFilterProxyModel::setFilterWildcard(const QString &pattern)
{
    Q_D(QSortFilterProxyModel);
    d->filter_regularexpression.removeBindingUnlessInWrapper();
    d->filter_about_to_be_changed();
    d->set_filter_pattern(QRegularExpression::wildcardToRegularExpression(
            pattern, QRegularExpression::UnanchoredWildcardConversion));
    d->filter_changed(QSortFilterProxyModelPrivate::Direction::Rows);
    d->filter_regularexpression.notify();
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hebasto
Copy link
Member Author

hebasto commented May 21, 2022

Combined into #593.

@hebasto hebasto closed this May 21, 2022
@hebasto hebasto deleted the 220423-wildcard branch May 24, 2022 08:54
@bitcoin-core bitcoin-core locked and limited conversation to collaborators May 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Feature Tests UX All about "how to get things done"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants