sitter added inline comments.
INLINE COMMENTS
> config_keys.h:5
> + */
> +
> +static const char CONFIG_TRIGGERWORD[] = "triggerWord";
Include guards please
> dictionarymatchengine.cpp:85
> m_wordLock.lockForRead();
> - foreach (ThreadData *data, m_lockers.values(source)) {
> + for (ThreadData *data: m_lockers.values(source)) {
> QMutexLocker locker(&data->mutex);
I think our style is space before and after :
> dictionaryrunner.cpp:37
> + }
> + setSyntaxes({Plasma::RunnerSyntax(Plasma::RunnerSyntax(i18nc("Dictionary
> keyword", "%1:q:", m_triggerWord),
> + i18n("Finds the
> definition of :q:.")))});
This constructs the same RunnerSyntax twice does it not?
> dictionaryrunner.cpp:75
> QString lastPartOfSpeech;
> - foreach (const QString &line, lines) {
> - if (partOfSpeech.indexIn(line) == -1)
> + for (const QString &line: qAsConst(lines)) {
> + const QRegularExpressionMatch partOfSpeechMatch =
> m_partOfSpeechRegex.match(line);
` : `
> dictionaryrunner.cpp:90
> + // TODO What to set as data?
> + //match.setData(QVariant());
> matches.append(match);
You could simply use the lastPartOfSpeech I would guess. From what I've seen
and what Kai tells me you really only need to set setData if you want to
implement actions (`::actionsForMatch`) and need to carry some information to
figure out what the match result was or what its actions ought to be, or
runability (`::run`). The latter in fact only appears to need id() so it can
persistently track how often a given thing was run and thus give it higher
relevance if the user chooses that option a lot.
That is to say: the runner as-is wouldn't benefit from setData at all.
> dictionaryrunner_config.cpp:24
> - connect(m_triggerWord, SIGNAL(textChanged(QString)), this,
> SLOT(changed()));
> - load();
> }
I wouldn't remove this without good reason.
Without this blocking load() call the UI gets set up and shown and only then
the data gets loaded in a kinda async fashion. That is cool, but only iff the
KCM is actually equipped for handling the intermediate time frame with a "I'm
still loading" UI state (busyindicator or something; or is completely empty at
least). If that is not the case then relying on async load will result in the
KCM appearing in an incomplete state until the load() is run which may take a
noticeable amount of time depending on system performance. So, when load() is
doing trivial/cheap work it makes a whole lot of sense to call it blockingly
form the ctor.
> dictionaryrunner_config.cpp:29
> {
> - KCModule::load();
> - KSharedConfig::Ptr cfg =
> KSharedConfig::openConfig(QLatin1String("krunnerrc"));
Could you elaborate on why you removed the baseclass calls for
load/save/defaults please?
REPOSITORY
R114 Plasma Addons
REVISION DETAIL
https://phabricator.kde.org/D28192
To: alex, broulik, ngraham, sitter, mlaurent
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus,
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
