D28192: WIP: Refactor dictionary runner

[Harald Sitter]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200326/1af01628/attachment-0001.html>