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.

Sign up for GitHub

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

Jump to bottom

Getting ready to Qt 6 (8/n). Use QRegularExpression in AddressBookSortFilterProxyModel class #593

Merged
merged 2 commits into from
May 24, 2022
Merged

Getting ready to Qt 6 (8/n). Use QRegularExpression in AddressBookSortFilterProxyModel class #593

merged 2 commits into from
May 24, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Apr 23, 2022 •
edited
Loading

This is a step in migration to Qt 6.

Related:

No behavior change. To ensure this, tests have been added.

@hebasto hebasto mentioned this pull request Apr 23, 2022
Closed
@hebasto hebasto added the Qt 6 Transition to Qt 6 label Apr 23, 2022
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 24, 2022 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #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.

@DrahtBot DrahtBot mentioned this pull request May 10, 2022
Closed
promag
promag reviewed May 21, 2022
View reviewed changes
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.

Concept ACK.

src/qt/addressbookpage.cpp Outdated

return true;
#if (QT_VERSION >= QT_VERSION_CHECK(5, 12, 0))
const auto pattern = filterRegularExpression().pattern();
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering https://github.com/bitcoin-core/gui/pull/592/files#r878677419, I think the only required change is:

#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
    const auto pattern = filterRegularExpression()
#else
    const auto pattern = filterRegExp();
#endif

Copy link
Member Author

@hebasto hebasto May 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@promag

Unfortunately, it does not work for me (

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! Updated.

@promag
Copy link
Contributor

promag commented May 21, 2022

I think we can now close #592?

@hebasto
Copy link
Member Author

hebasto commented May 21, 2022

Updated c4e8af8 -> e280087 (pr593.01 -> pr593.02):

promag
promag approved these changes May 21, 2022
View reviewed changes
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.

Code review ACK e280087.

Could also have a test case-insensitivity search.

@hebasto
Copy link
Member Author

hebasto commented May 21, 2022

Could also have a test case-insensitivity search.

Could be a good first issue for following up :)

@hebasto
Copy link
Member Author

hebasto commented May 23, 2022

cc @luke-jr @prusnak @w0xlt

w0xlt
w0xlt approved these changes May 23, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

tACK e280087 on Ubuntu 21.10 Qt 5.15.2

Interesting note about QRegularExpression class:
https://doc.qt.io/qt-5/qregularexpression.html#notes-for-qregexp-users

@hebasto
Copy link
Member Author

hebasto commented May 23, 2022

@w0xlt

tACK e280087 on Ubuntu 21.10 Qt 5.15.2

Thank you for your review and testing!

FWIW, to test with Qt 6, the branch from bitcoin/bitcoin#24798 could be used.

promag
promag reviewed May 23, 2022
View reviewed changes
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 e280087 with Qt6 on macOS 12 M1.

jarolrod
jarolrod approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@jarolrod jarolrod 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 e280087 on M1 mac, x86 mac, x86 Linux with Qt5 and separately with Qt6

The added tests in 5c5d8f2 are useful for verifying there's nothing being messed up by the changes. I also manually tested the address book for:

  • The ability to still search effectively after the change
  • That additions to the address book appear in the same order before and after the change

@hebasto hebasto merged commit 8898906 into bitcoin-core:master May 24, 2022
@hebasto hebasto deleted the 220423-re branch May 24, 2022 08:52
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 28, 2022
@hebasto
Merge bitcoin-core/gui#593: Getting ready to Qt 6 (8/n). Use `QRegula…
c24c674 
…rExpression` in `AddressBookSortFilterProxyModel` class

e280087 qt: Use `QRegularExpression` in `AddressBookSortFilterProxyModel` class (Hennadii Stepanov)
5c5d8f2 qt, test: Add tests for searching in `AddressBookPage` dialog (Hennadii Stepanov)

Pull request description:

  This is a step in [migration](bitcoin#24798) to Qt 6.

  Related:
  - bitcoin-core/gui#578
  - bitcoin-core/gui#585

  No behavior change. To ensure this, tests have been added.

ACKs for top commit:
  hebasto:
    > tACK [e280087](bitcoin-core/gui@e280087) on Ubuntu 21.10 Qt 5.15.2
  promag:
    Tested ACK e280087 with Qt6 on macOS 12 M1.
  w0xlt:
    tACK bitcoin-core/gui@e280087 on Ubuntu 21.10 Qt 5.15.2
  jarolrod:
    Tested ACK bitcoin-core/gui@e280087 on M1 mac, x86 mac, x86 Linux with Qt5 and separately with Qt6

Tree-SHA512: 664baacc1504deb2f7fa651ea4a44f3942f5c9058befe4d2ce292beed032d4b1697710cfd10c0909602d8a4a6eeb680414e4a1f56d2038478c1ae2f34965d74f
@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.
Reviewers

@promag promag promag approved these changes

@jarolrod jarolrod jarolrod approved these changes

@w0xlt w0xlt w0xlt approved these changes

Assignees
No one assigned
Labels
Qt 6 Transition to Qt 6
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hebasto @DrahtBot @promag @jarolrod @w0xlt