Refactor foreach, use Qt5 signal syntax, remove unnecessary method call
ClosedPublic

Authored by alex on Jan 31 2020, 3:09 PM.

Details

Summary

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.

Test Plan

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
alex created this revision.Jan 31 2020, 3:09 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 31 2020, 3:09 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
alex requested review of this revision.Jan 31 2020, 3:09 PM
alex updated this revision to Diff 74793.Jan 31 2020, 3:12 PM

Remove unused include

alex updated this revision to Diff 74795.Jan 31 2020, 3:16 PM

Simplify statement

kossebau edited reviewers, added: Plasma; removed: kossebau.Jan 31 2020, 5:05 PM
vhanda removed a reviewer: vhanda.Jan 31 2020, 9:28 PM
vhanda added a subscriber: vhanda.

Sorry. I'm removing myself as a reviewer. I barely remember this code base. It has been way too many years.

vhanda removed a subscriber: vhanda.Jan 31 2020, 9:29 PM

Sorry. I'm removing myself as a reviewer. I barely remember this code base. It has been way too many years.

Thanks anyway.

davidedmundson accepted this revision.Feb 1 2020, 7:58 AM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
runners/spellchecker/spellcheck.cpp
145

Const auto &

This revision is now accepted and ready to land.Feb 1 2020, 7:58 AM
alex updated this revision to Diff 74830.Feb 1 2020, 11:05 AM

Simplify and optimize

alex marked an inline comment as done.Feb 1 2020, 11:24 AM

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.

alex updated this revision to Diff 74847.Feb 2 2020, 6:55 AM

Refactoring, improve validation in kcm, optimize

alex updated this revision to Diff 74848.Feb 2 2020, 7:03 AM

Undo changes that were supposed to be in new diff

alex updated this revision to Diff 78335.Mar 24 2020, 7:06 AM

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
alex updated this revision to Diff 78336.Mar 24 2020, 7:09 AM
davidedmundson accepted this revision.Mar 24 2020, 9:52 AM
alex added a comment.Mar 24 2020, 2:42 PM

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 revision was automatically updated to reflect the committed changes.

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] ^~~~~~~~
alex added a comment.EditedMar 24 2020, 4:52 PM

Sh**t, should I make a new patch to fix this?
And the fix is exactly what the error message suggests.

And sorry ...

My fault, I'll fix it