Replace QRegExp with QRegularExpression #585

pull prusnak wants to merge 2 commits into bitcoin-core:master from prusnak:qregexp-obsolete changing 3 files +19 −9

prusnak commented at 6:20 PM on April 15, 2022: contributor

Fixes #578

Only made sure the code builds, not sure how to test.
hebasto commented at 6:25 PM on April 15, 2022: member

Concept ACK.

As a preparation to Qt 6, I think this PR will benefit by incorporating of https://github.com/bitcoin/bitcoin/commit/f6adcadf643c3e3375d234ef0d0987f20bc9869b and https://github.com/bitcoin/bitcoin/commit/b649103d0554055470ea00cb46489c43c90796c9 in any reasonable way.
hebasto renamed this:
~~qt: replace QRegExp with QRegularExpression~~
Replace QRegExp with QRegularExpression
on Apr 15, 2022
hebasto added the label Refactoring on Apr 15, 2022
hebasto added the label Qt 6 on Apr 15, 2022
prusnak commented at 6:31 PM on April 15, 2022: contributor
As a preparation to Qt 6, I think this PR will benefit by incorporating of bitcoin/bitcoin@f6adcad and bitcoin/bitcoin@b649103 in any reasonable way.
- bitcoin/bitcoin@f6adcad is already part of c6b6106
- the latter commit I just cherry-picked to this PR
prusnak force-pushed on Apr 15, 2022
prusnak commented at 10:29 PM on April 15, 2022: contributor

It seems the CI is failing because it does not recognize filterRegularExpression and indeed it was introduced in Qt 5.12 while we still do support Qt 5.11.

I updated the second commit of the PR to use QT_VERSION check - see 54980a7994a13fa77b80503ba8574490e80c9649
hebasto commented at 6:00 PM on April 21, 2022: member

Does it make sense to extract the "Extract first suffix from filter pattern..." functionality into a testable utility function?
hebasto commented at 5:08 PM on April 23, 2022: member

not sure how to test.

Here is a branch with tests for the AddressBookPage class -- https://github.com/hebasto/gui/commits/220423-test-ab

And test fails for the 54980a7994a13fa77b80503ba8574490e80c9649 commit.

UPDATE: mind looking at #591, #592 and #593?
DrahtBot commented at 3:51 AM on April 24, 2022: contributor


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)
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.
hebasto referenced this in commit 3dd95cb5c2 on May 9, 2022
sidhujag referenced this in commit 049283e67f on May 9, 2022

in src/qt/utilitydialog.cpp:47 in c6b6106c5c outdated

  43 | @@ -44,8 +44,7 @@ HelpMessageDialog::HelpMessageDialog(QWidget *parent, bool about) :
  44 |          /// HTML-format the license message from the core
  45 |          QString licenseInfoHTML = QString::fromStdString(LicenseInfo());
  46 |          // Make URLs clickable
  47 | -        QRegExp uri("<(.*)>", Qt::CaseSensitive, QRegExp::RegExp2);
  48 | -        uri.setMinimal(true); // use non-greedy matching
  49 | +        QRegularExpression uri("<(.*)>", QRegularExpression::InvertedGreedinessOption); // use non-greedy matching

hebasto commented at 3:09 PM on May 23, 2022:

A comment seems unneeded here because QRegularExpression::InvertedGreedinessOption is pretty self-explained.

prusnak commented at 4:33 PM on May 23, 2022:

Addressed in db71143629850e8c9bef22991ae054a0ae82c7ca

qt: replace QRegExp with QRegularExpression db71143629
qt: Use `QRegularExpression` in `AddressBookSortFilterProxyModel` class 7b896dc0ac
prusnak force-pushed on May 23, 2022
in src/qt/addressbookpage.cpp:50 in 7b896dc0ac
```
  46 | @@ -46,12 +47,19 @@ class AddressBookSortFilterProxyModel final : public QSortFilterProxyModel
  47 |  
  48 |          auto address = model->index(row, AddressTableModel::Address, parent);
  49 |  
  50 | +    #if (QT_VERSION >= QT_VERSION_CHECK(5, 12, 0))
```
jarolrod commented at 12:45 AM on May 24, 2022:

slight preference for flipping this around where we're checking if less than qt6, i think this makes the code cleaner and inline with how we've done some other qt6 preparation changes, but not a blocker

hebasto commented at 6:35 PM on May 24, 2022:

Current implementation allows to test without Qt 6.
hebasto referenced this in commit 8898906370 on May 24, 2022
DrahtBot commented at 9:23 AM on May 24, 2022: contributor



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

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
DrahtBot added the label Needs rebase on May 24, 2022
prusnak commented at 9:55 AM on May 24, 2022: contributor

I won't be able to work on this PR in the following days/weeks. Feel free to push into my branch (maintainers are allowed) or take over the PR completely if needed.
hebasto added the label Up for grabs on May 24, 2022
jarolrod commented at 11:28 PM on May 24, 2022: member

Picked up in #606
jarolrod closed this on May 24, 2022
hebasto removed the label Up for grabs on May 25, 2022
prusnak deleted the branch on May 25, 2022
sidhujag referenced this in commit c24c674950 on May 28, 2022
bitcoin-core locked this on May 25, 2023

Contributors

Labels

Refactoring Needs rebase Qt 6

Linked (view graph)

#578 Replace `QRegExp` with `QRegularExpression`#606 Replace QRegExp with QRegularExpression

Replace QRegExp with QRegularExpression #585

Conflicts