D26205: KWallet: Port QRegExp to QRegularExpression

[Mikołaj Płomieński]

blaze added a comment.


  I was debugging the code locally and here's what I found: simple wildcard 
like `*` is being replaced with cool stuff like `\\A(?:[^/]*)\\z`.
  As you can see, this pattern excludes forward slashes, which may be a good 
thing for a file path, but it is exactly the source of the problem in this case.
  So here's my debug code, which was tested by me and it works:
  
QRegularExpression re(QRegularExpression::wildcardToRegularExpression(key)
   .replace("[^/]", "."));
qDebug() << " == PATTERN == " << re.pattern();

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Mikołaj Płomieński]

blaze added a comment.


  In D26205#595428 , @ahmadsamir 
wrote:
  
  > I still fail to see what's broken, I tested with the system kwallet (5.65 
on tumbleweed) and I don't see any difference between it and a build from a git 
checkout; i.e. falkon doesn't show the saved passwords after restarting it. 
I've already submitted another diff to fix the doubly-anchored pattern issue.
  
  
  
  
  > falkon doesn't show the saved passwords
  
  It's not normal behavior anyways. But have you forgot to restart kwalletd 
process while testing?

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  In D26205#595111 , @blaze wrote:
  
  > > otherwise, we could just revert this patch
  >
  > The rest of the code is OK. The part that works funny is just the wildcard 
method, and since it's relatively new, I hope there could be a good workaround 
until the situation stabilizes
  
  
  I still fail to see what's broken, I tested with the system kwallet (5.65 on 
tumbleweed) and I don't see any difference between it and a build from a git 
checkout; i.e. falkon doesn't show the saved passwords after restarting it. 
I've already submitted another diff to fix the doubly-anchored pattern issue.
  
  Could you give me another test case? I want to see what's broken to try and 
fix it.
  
  Thanks in advance.

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.




INLINE COMMENTS

> blaze wrote in kwallet.cpp:180
> > The transformation is targeting file path globbing, which means in 
> > particular that path separators receive special treatment.
> 
> I'm pretty sure the file path case is different from what we have here.
> 
> Also
> 
> > In order to match one of the special characters, place it in square 
> > brackets (for example, "[?]")
> 
> Doesn't it mean you can't just use a bare raw wildcard?
> 
> So my suggestion is just to rollback to the previous solution, at least 
> temporarily, until we don't have something better.

The part about "[?]"; in the QRegularExpression documentation 
https://doc.qt.io/qt-5/qregularexpression.html#wildcardToRegularExpression "?" 
is used for wild card matching:

> ?   Matches any single character. It is the same as . in full regexps.

IIUC, the comment about "[?]" means if wildcardToRegularExpression() is used 
and you want to match a literal "?" character you'll have to use square 
brackets "[?]", different issue.

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Albert Astals Cid]

aacid added a comment.


  In D26205#595080 , @ahmadsamir 
wrote:
  
  > In D26205#595034 , @blaze wrote:
  >
  > > 
https://github.com/KDE/falkon/blob/master/src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp#L187
  > >
  > >   QMap entries;
  > >   if (m_wallet->readEntryList("*", entries) != 0) {
  > >   qWarning() << "KWalletPasswordBackend::initialize Cannot read 
entries!";
  > >   return;
  > >   }
  > >   
  > >
  > > This is the problematic code. As you can see it uses "*" as a wildcard. 
So may be it is possible just to use a different wildcard and it could be 
solved IDK
  >
  >
  > OK, I'll see if I can make it work and also fix the double-anchored issue; 
in a new diff, otherwise, we could just revert this patch.
  >
  > @apol, @aacid WDYT?
  
  
  I was going to be silent here, but since you asked my opinion is this, if you 
are not going to test the code you are changing, you should stop changing code. 
And by reading the comments it seems you aren't testing it, so either start 
testing it or stop doing blind changes.

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Mikołaj Płomieński]

blaze added a comment.


  > otherwise, we could just revert this patch
  
  The rest of the code is OK. The part that works funny is just the wildcard 
method, and since it's relatively new, I hope there could be a good workaround 
until the situation stabilizes

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  In D26205#595034 , @blaze wrote:
  
  > 
https://github.com/KDE/falkon/blob/master/src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp#L187
  >
  >   QMap entries;
  >   if (m_wallet->readEntryList("*", entries) != 0) {
  >   qWarning() << "KWalletPasswordBackend::initialize Cannot read 
entries!";
  >   return;
  >   }
  >   
  >
  > This is the problematic code. As you can see it uses "*" as a wildcard. So 
may be it is possible just to use a different wildcard and it could be solved 
IDK
  
  
  OK, I'll see if I can make it work and also fix the double-anchored issue; in 
a new diff, otherwise, we could just revert this patch.
  
  @apol, @aacid WDYT?

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Mikołaj Płomieński]

blaze added a comment.


  
https://github.com/KDE/falkon/blob/master/src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp#L187
  
QMap entries;
if (m_wallet->readEntryList("*", entries) != 0) {
qWarning() << "KWalletPasswordBackend::initialize Cannot read entries!";
return;
}
  
  This is the problematic code. As you can see it uses "*" as a wildcard. So 
may be it is possible just to use a different wildcard and it could be solved 
IDK

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Mikołaj Płomieński]

blaze added a comment.


  In D26205#594867 , @ahmadsamir 
wrote:
  
  > A test case of what is broken would be appreciated, to try and fix/debug 
the issue.

INLINE COMMENTS

> ahmadsamir wrote in kwallet.cpp:180
> Any suggestions? QRegularExpression::wildcardToRegularExpression() is what 
> the upstream docs offer:
> 
> > Wildcard matching
> > 
> > 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.

> The transformation is targeting file path globbing, which means in particular 
> that path separators receive special treatment.

I'm pretty sure the file path case is different from what we have here.

Also

> In order to match one of the special characters, place it in square brackets 
> (for example, "[?]")

Doesn't it mean you can't just use a bare raw wildcard?

So my suggestion is just to rollback to the previous solution, at least 
temporarily, until we don't have something better.

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> blaze wrote in kwallet.cpp:179
> QRegularExpression::wildcardToRegularExpression() already returns an anchored 
> output. What do you mean?

I didn't get what you meant before.

I've just tested and you're right; I didn't know that 
wildcardToRegularExpression() returned an anchored pattern, the docs didn't say 
anything about that (if they did, I must have missed it).

I'll be filing a diff to fix that (and I'll have review all my other QRegExp 
port diffs, the same issue will be in more places than just here in kwallet...).

Thanks for catching this issue.

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Mikołaj Płomieński]

blaze added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kwallet.cpp:179
> The pattern has to be anchored if we want to replicate what 
> QRegExp::exactMatch() did, with \A and \z or ^ and $.
> 
> If you have a test case where it anchoredPattern() anchors twice, file a bug 
> report upstream (to be honest, I don't know whether this is the intended 
> behaviour or not).

QRegularExpression::wildcardToRegularExpression() already returns an anchored 
output. What do you mean?

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  A test case of what is broken would be appreciated, to try and fix/debug the 
issue.

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> blaze wrote in kwallet.cpp:179
> anchoredPattern() is excessive here, it creates a horror like that 
> "\\A(?:\\A(?:[^/]*)\\z)\\z" with the stuff being anchored twice.
> It has to be removed with no doubt at all.

The pattern has to be anchored if we want to replicate what 
QRegExp::exactMatch() did, with \A and \z or ^ and $.

If you have a test case where it anchoredPattern() anchors twice, file a bug 
report upstream (to be honest, I don't know whether this is the intended 
behaviour or not).

> blaze wrote in kwallet.cpp:180
> The output of wildcardToRegularExpression() method is different from what was 
> before and it breaks the app. It has to be replaced with something else.

Any suggestions? QRegularExpression::wildcardToRegularExpression() is what the 
upstream docs offer:

> Wildcard matching
> 
> 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.

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Mikołaj Płomieński]

blaze added a comment.


  This changeset breaks the app, making it impossible to get the list of 
entries from outside. Tested against Falkon browser with KDE integration 
plugin. See my inline comments here

INLINE COMMENTS

> kwallet.cpp:179
>  if (searchItemsJob->exec()) {
> -QRegExp re(key, Qt::CaseSensitive, QRegExp::Wildcard);
> +const QRegularExpression re(QRegularExpression::anchoredPattern(
> +  
> QRegularExpression::wildcardToRegularExpression(key)));

anchoredPattern() is excessive here, it creates a horror like that 
"\\A(?:\\A(?:[^/]*)\\z)\\z" with the stuff being anchored twice.
It has to be removed with no doubt at all.

> kwallet.cpp:180
> +const QRegularExpression re(QRegularExpression::anchoredPattern(
> +  
> QRegularExpression::wildcardToRegularExpression(key)));
>  const auto list = searchItemsJob->items();

The output of wildcardToRegularExpression() method is different from what was 
before and it breaks the app. It has to be replaced with something else.

> kwalletbackend.cc:533
>  
> -QRegExp re(key, Qt::CaseSensitive, QRegExp::Wildcard);
> +QRegularExpression re(QRegularExpression::anchoredPattern(
> +   
> QRegularExpression::wildcardToRegularExpression(key)));

Same here as well.

> kwalletbackend.cc:534
> +QRegularExpression re(QRegularExpression::anchoredPattern(
> +   
> QRegularExpression::wildcardToRegularExpression(key)));
>  

and here

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: blaze, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Ahmad Samir]

This revision was automatically updated to reflect the committed changes.
Closed by commit R311:bb4eed3bfda0: KWallet: Port QRegExp to QRegularExpression 
(authored by ahmadsamir).

REPOSITORY
  R311 KWallet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26205?vs=72132=72146

REVISION DETAIL
  https://phabricator.kde.org/D26205

AFFECTED FILES
  src/api/KWallet/kwallet.cpp
  src/runtime/kwalletd/backend/kwalletbackend.cc
  src/runtime/kwalletd/kwalletd.cpp

To: ahmadsamir, #frameworks, aacid, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Aleix Pol Gonzalez]

apol accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R311 KWallet

BRANCH
  l-qregularexpression (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26205

To: ahmadsamir, #frameworks, aacid, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, aacid.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  Port QRegExp::exactMatch() with QRegularExpression::anchoredPattern()
  Port QRegExp::Wildcard with QRegularExpression::wildcardToRegularExpression()
  
  The code compiles (there is only one unrelated unit test, it passes).

REPOSITORY
  R311 KWallet

BRANCH
  l-qregularexpression (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26205

AFFECTED FILES
  src/api/KWallet/kwallet.cpp
  src/runtime/kwalletd/backend/kwalletbackend.cc
  src/runtime/kwalletd/kwalletd.cpp

To: ahmadsamir, #frameworks, aacid
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns