D28192: WIP: Refactor dictionary runner

[Alexander Lohnau]

alex added inline comments.

INLINE COMMENTS

> sitter wrote in dictionaryrunner.cpp:90
> 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.

The current behavior is that KRunner closes when a match is selected.
But if you set the data, for instance:
`match.setData(QStringLiteral("Hello There!"));`
The text `Hello There!` will show up in KRunner (like in the converter runner).
The run method won't be called, because the match type is set to InformationalMatch.

So the runner would benefit from setData, or am I missing something?

> sitter wrote in dictionaryrunner_config.cpp:24
> 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.

Thanks for the detailed explanation.
Calling the method twice seemed weird to me but  as you have said the load method does only cheap work and it should not matter that much.

> sitter wrote in dictionaryrunner_config.cpp:29
> Could you elaborate on why you removed the baseclass calls for load/save/defaults please?

The baseclass implementations for these methods handle the KConfigDialogManager widgets and the corresponding changed signals.
I removed them, because the KConfigDialogManager class is not used in this kcm and the signals are manually handled.

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/20200328/628e7668/attachment-0001.html>