WIP: Refactor dictionary runner
Needs ReviewPublic

Authored by alex on Sat, Mar 21, 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.Sat, Mar 21, 7:23 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSat, Mar 21, 7:23 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
alex requested review of this revision.Sat, Mar 21, 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.Thu, Mar 26, 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.Sat, Mar 28, 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.