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

Suppress Qt warnings for invalid QRegularExpression #292

Merged
merged 2 commits into from
Dec 31, 2024
Merged

Conversation

isf63
Copy link
Contributor

@isf63 isf63 commented Dec 31, 2024

Previously when entering math and other expressions that were not valid QRegularExpression's, lxqt-runner would spam hundreds of Qt warnings that could be seen from the command line or journalctl.

Previously when entering math and other expressions that were not valid
QRegularExpression's, lxqt-runner would spam hundreds of Qt warnings that
could be seen from the command line or journalctl.
@isf63
Copy link
Contributor Author

isf63 commented Dec 31, 2024

To see the warnings, open sudo journalctl -f and enter into lxqt-runner (1+1

@tsujan
Copy link
Member

tsujan commented Dec 31, 2024

To see the warnings, open sudo journalctl -f

That's not the case for all, especially with ~/.config/QtProject/qtlogging.ini containing

[Rules]
*.warning=false

It's better to be more specific; you mean the new warnings like QString(View)::contains(): called on an invalid QRegularExpression object (pattern is '*'). I think they're caused by a change in Qt.

Do these cover all instances inside the code?

@isf63
Copy link
Contributor Author

isf63 commented Dec 31, 2024

QString(View)::contains(): ... is indeed the warnings I was referring to. When typing certain math expressions, hundreds or thousands of them are logged.

Do these cover all instances inside the code?

.contains(<regExp>) is only used in providers.cpp (grepping for "contains") - this change covers all instances.

@tsujan
Copy link
Member

tsujan commented Dec 31, 2024

What about

bool CommandItemModel::filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const
{
QRegularExpression re(filterRegularExpression());
if (re.pattern().isEmpty() && !mOnlyHistory)
return false;

I'm not talking about adding a check there; I just want to know whether you see a warning about it, because if you do, Qt has a new, annoying problem.

@isf63
Copy link
Contributor Author

isf63 commented Dec 31, 2024

I added checks before _.pattern().isEmpty() without much thought. I don't think it's necessary.

I can't figure out how to link Qt6 with pkg-config to make a small test, but I suspect there is no warning.

@tsujan
Copy link
Member

tsujan commented Dec 31, 2024

OK. Thanks.

Please use your patch for a while to make sure that all such warnings are covered by it. I'll merge it later.

@tsujan
Copy link
Member

tsujan commented Dec 31, 2024

On second thought, this is what I mean:

What about doing the following, instead of handling individual instances?

diff -ruNp lxqt-runner-orig/dialog.cpp lxqt-runner/dialog.cpp
--- lxqt-runner-orig/dialog.cpp
+++ lxqt-runner/dialog.cpp
@@ -546,7 +546,8 @@ void Dialog::setFilter(const QString &te
     QString trimmedText = text.simplified();
     mCommandItemModel->setCommand(trimmedText);
     mCommandItemModel->showOnlyHistory(onlyHistory);
-    mCommandItemModel->setFilterRegularExpression(trimmedText);
+    QRegularExpression r(trimmedText);
+    mCommandItemModel->setFilterRegularExpression(r.isValid() ? r : QRegularExpression());
     mCommandItemModel->invalidate();
 
     // tidy up layout and select first item

@isf63
Copy link
Contributor Author

isf63 commented Dec 31, 2024

Tested briefly, that seems to be simpler and more robust.

@tsujan
Copy link
Member

tsujan commented Dec 31, 2024

Is the calculator OK too?

@isf63
Copy link
Contributor Author

isf63 commented Dec 31, 2024

It appears to be identical to mine. I thought, for some reason, that (3+3= was accepted (evaluated) in some previous version, but 2.1.0 has the same behavior.

@tsujan
Copy link
Member

tsujan commented Dec 31, 2024

OK. Let's merge it. Thanks again for finding and removing this Qt annoyance.

@tsujan tsujan merged commit fd53e27 into lxqt:master Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants