D28192: WIP: Refactor dictionary runner

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> alex wrote in dictionaryrunner_config.cpp:29
> That makes sense but one question: The doc says: `...However, if you for some 
> reason reimplement it and also are using KConfigXT, you must call this 
> function`, does this mean we can assume that the base class is not needed?
> 
> PS: In this case it is not very relevant but I would like to understand 
> concept for future patches .

That line makes no assertions about what to do when you are not using 
KConfigXT. We don't really have consistent language for docs unfortunately but 
generally unless the docs explicitly say you must or you must not, it's usually 
best to assume that you should go with best practice.

In this particular case the fact that the docs say you must call the base when 
using kconfigxt could just mean that someone forgot about forwarding the call 
and then stumbled over bugs and took the time to add a warning for future 
generations to not repeat their mistake. It doesn't necessarily mean that the 
list of musts is comprehensive and exhaustive. It certainly doesn't mean that 
implicit requirements will remain the same over time. And along that line, 
consider the two scenarios:

in 10 years

- a new requirement is added where you must forward the call to the base. say a 
sound plays on every save and that is implemented inside KCModule. if you do 
not forward the call this singular KCM will be misbehaving and someone has to 
file a bug and a dev has to go figure out that the calls aren't being forwarded
- a new requirement is added where you must **not** forward the call (terribly 
unlikely) it's easy for the baseclass to know when that requirement was 
violated and Q_ASSERT or print a qwarning. at that point you'll still have a 
bug on your hand but the library can make provisions for that bug to not be 
actually a problem

or simply put: it's easier to deal with useless/unintended calls than with 
entirely missing calls. if you don't call a library it can't tell you about 
failed runtime assertions and the like.

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

D28192: WIP: Refactor dictionary runner

[Alexander Lohnau]

alex added inline comments.

INLINE COMMENTS

> sitter wrote in dictionaryrunner_config.cpp:29
> That's an implementation detail though, is it not? From the outside we 
> shouldn't make assumption about what the implementation does unless the 
> documentation says what we can assume.
> Today the baseclass may be useless, in 10 years it may not be.
> 
> Long-winded way of saying that I would leave the base class calls in. If 
> nothing else it's at least better form in terms of API contracts.

That makes sense but one question: The doc says: `...However, if you for some 
reason reimplement it and also are using KConfigXT, you must call this 
function`, does this mean we can assume that the base class is not needed?

PS: In this case it is not very relevant but I would like to understand concept 
for future patches .

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

D28192: WIP: Refactor dictionary runner

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

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

Entirely possible, @broulik knows way more about the UI code. If data is used 
in the UI then that would run counter to its purpose though.

https://api.kde.org/frameworks/krunner/html/classPlasma_1_1QueryMatch.html#aad806b18a5fbec942f1258f6b4436cd8

  Sets data to be used internally by the associated AbstractRunner. 

putting a string into the UI is not internal anymore ^^

> alex wrote in dictionaryrunner_config.cpp:29
> 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.

That's an implementation detail though, is it not? From the outside we 
shouldn't make assumption about what the implementation does unless the 
documentation says what we can assume.
Today the baseclass may be useless, in 10 years it may not be.

Long-winded way of saying that I would leave the base class calls in. If 
nothing else it's at least better form in terms of API contracts.

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

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

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(>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 , lines) {
> -if (partOfSpeech.indexIn(line) == -1)
> +for (const QString : 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

D28192: WIP: Refactor dictionary runner

[Harald Sitter]

sitter added a comment.


  About fixing the UX:
  There is some kind of preview tech in milou which you can take a look at and 
maybe build upon.
  The line space is just never going to be enough for a text only 
representation of a dict entry, plus it looks fairly forced anyway. So, my 
thinking is that ideally we'll want a minimal text display but then be able to 
expand that with a rich "preview" of the dict entry for the clients where we 
need that (notably krunner I suppose). Rich in this case means having full on 
control over how it is displayed using a bespoke QML item. This could work a 
lot like clipboard entries do, where each entry can have multiple mimetypes, 
one of them ought to be text which is kind of the go-to fallback, but they 
others may give a richer experience when the client supports it. Matches could 
behave much the same way, and from a glance at the existing milou code that is 
kind of where things were headed.

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

D28192: WIP: Refactor dictionary runner

[Alexander Lohnau]

alex created this revision.
alex added reviewers: broulik, ngraham, sitter, mlaurent.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
alex requested review of this revision.

REVISION SUMMARY
  The config key has been moved to a separate file, some foreach have been 
refactored, the QRegExp occurrences have been ported to QRegularExpression and 
are now reused.
  Furthermore unnecessary functions/function calls have been removed and the 
code is formatted.
  
  WIP: Currently the way the matches look is not very good, because there is 
far too much text displayed for example:
  F8191533: CurrentStateNormalKWin 
  And with a crappy kwin rule which increases the width:
  F8191530: Current 
  
  My Idea would be to make the definition, example sentence and synonyms 
customizable.
  By default the word type, definition and synonyms would be shown for example:
  `n trying something to find out about it [syn: trial, trial run, test, 
tryout]`
  And also an action which shows a dialog with all the available information 
and maybe some copy buttons for the synonyms.
  
  Because this patch is refactoring/cleanup I consider making a new patch for 
these feature and would appreciate feedback for the ideas :-).

TEST PLAN
  Should work as before

REPOSITORY
  R114 Plasma Addons

BRANCH
  dictionary_runner_improvements

REVISION DETAIL
  https://phabricator.kde.org/D28192

AFFECTED FILES
  runners/dictionary/config_keys.h
  runners/dictionary/dictionarymatchengine.cpp
  runners/dictionary/dictionaryrunner.cpp
  runners/dictionary/dictionaryrunner.h
  runners/dictionary/dictionaryrunner_config.cpp
  runners/dictionary/dictionaryrunner_config.h

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