WIP: Refactor dictionary runner
Needs ReviewPublic

Authored by alex on Mar 21 2020, 7:23 PM.

Details

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:


And with a crappy kwin rule which increases the width:

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

Diff Detail

Repository
R114 Plasma Addons
Branch
dictionary_runner_improvements
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24043
Build 24061: arc lint + arc unit
alex created this revision.Mar 21 2020, 7:23 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 21 2020, 7:23 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
alex requested review of this revision.Mar 21 2020, 7:23 PM

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.

sitter added inline comments.Mar 26 2020, 11:01 AM
runners/dictionary/config_keys.h
5

Include guards please

runners/dictionary/dictionarymatchengine.cpp
85

I think our style is space before and after :

runners/dictionary/dictionaryrunner.cpp
37

This constructs the same RunnerSyntax twice does it not?

75

:

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.

runners/dictionary/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.

29

Could you elaborate on why you removed the baseclass calls for load/save/defaults please?

alex added inline comments.Mar 28 2020, 10:54 AM
runners/dictionary/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?

runners/dictionary/dictionaryrunner_config.cpp
24

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.

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.

sitter added inline comments.Apr 6 2020, 1:58 PM
runners/dictionary/dictionaryrunner.cpp
90

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 ^^

runners/dictionary/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.

alex added inline comments.Apr 6 2020, 2:15 PM
runners/dictionary/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 😃.

sitter added inline comments.Apr 6 2020, 3:20 PM
runners/dictionary/dictionaryrunner_config.cpp
29

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.