The two foreach occurences are refactored and the signals in the runner/kcm now use the C++ 11 for loop.
Additonally the duplicate call of the load method has been removed and smaller improvements have been made.
Details
- Reviewers
ngraham broulik davidedmundson - Group Reviewers
Plasma - Commits
- R114:5e760a0586bb: Refactor foreach, use Qt5 signal syntax, remove unnecessary method call
Compile time check for signals, trying out the runner and editing the config.
Diff Detail
- Repository
- R114 Plasma Addons
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Sorry. I'm removing myself as a reviewer. I barely remember this code base. It has been way too many years.
Sorry. I'm removing myself as a reviewer. I barely remember this code base. It has been way too many years.
Thanks anyway.
runners/spellchecker/spellcheck.cpp | ||
---|---|---|
145 | Const auto & |
Hello,
thanks for the response.
I addition to your requested change I have made some smaller improvements.
But I have a question about the concept of this runner: Why is the data for every match session newly created ? I understand that it saves a bit of memory to delete them, but this way the spellers are for every match session created
(and the prepare/teardown slots are called in the main thread).
What do you think about loading the data in the reloadConfiguration method and then reusing it ?
This way the spellers for new languages can be also reused for multiple match sessions (they are in the match method created).
PS: When I tested this plugin I found out that the "task-attention" icon for the error message (line 252 in the runner) is no longer available, but I don't know which icon would be a good replacement.
Minor improvements
I had another look at the project and changed some other things:
- optimize some overloads with QStringLiteral/QLatin1String
- rename loaddata to loadData
- remove the unnecessary check for the action in the run method
- reuse the result of config()
- make sure that the trigger word can only be required if it is not empty
Small issue: Currently this patch can't be applied (from the master branch) because in R114:47381a65debe01a0a015e1aa423cbb78af649648 the line:
QStringList terms = query.split(QLatin1Char(' '), QString::SkipEmptyParts); got changed.
I don't know how to resolve this conflict and would be thankful for help :-).
This broke the CI:
[2020-03-24T15:59:18.523Z] [ 66%] Linking CXX shared module ../../bin/krunner_datetime.so [2020-03-24T15:59:18.523Z] /usr/home/jenkins/workspace/Plasma/kdeplasma-addons/kf5-qt5 FreeBSDQt5.14/runners/spellchecker/spellcheck.cpp:198:30: error: use of undeclared identifier 'findlang'; did you mean 'findLang'? [2020-03-24T15:59:18.523Z] const QString lang = findlang(terms); [2020-03-24T15:59:18.523Z] ^~~~~~~~
Sh**t, should I make a new patch to fix this?
And the fix is exactly what the error message suggests.
And sorry ...