Port QRegExp::exactMatch() with QRegularExpression::anchoredPattern()
Port QRegExp::Wildcard with QRegularExpression::wildcardToRegularExpression()
The code compiles (there is only one unrelated unit test, it passes).
aacid | |
apol |
Frameworks |
Port QRegExp::exactMatch() with QRegularExpression::anchoredPattern()
Port QRegExp::Wildcard with QRegularExpression::wildcardToRegularExpression()
The code compiles (there is only one unrelated unit test, it passes).
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
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
src/api/KWallet/kwallet.cpp | ||
---|---|---|
179 | anchoredPattern() is excessive here, it creates a horror like that "\\A(?:\\A(?:[^/]*)\\z)\\z" with the stuff being anchored twice. | |
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. | |
src/runtime/kwalletd/backend/kwalletbackend.cc | ||
533 | Same here as well. | |
534 | and here |
src/api/KWallet/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). | |
180 | Any suggestions? QRegularExpression::wildcardToRegularExpression() is what the upstream docs offer:
|
src/api/KWallet/kwallet.cpp | ||
---|---|---|
179 | QRegularExpression::wildcardToRegularExpression() already returns an anchored output. What do you mean? |
src/api/KWallet/kwallet.cpp | ||
---|---|---|
179 | 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. |
src/api/KWallet/kwallet.cpp | ||
---|---|---|
180 |
I'm pretty sure the file path case is different from what we have here. Also
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. |
QMap<QString, QByteArray> 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
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 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.
src/api/KWallet/kwallet.cpp | ||
---|---|---|
180 | The part about "[?]"; in the QRegularExpression documentation https://doc.qt.io/qt-5/qregularexpression.html#wildcardToRegularExpression "?" is used for wild card matching:
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. |
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.
falkon doesn't show the saved passwords
It's not normal behavior anyways. But have you forgot to restart kwalletd process while testing?
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();