<table><tr><td style="">alex added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D28192">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D28192#inline-160957">View Inline</a><span style="color: #4b4d51; font-weight: bold;">sitter</span> wrote in <span style="color: #4b4d51; font-weight: bold;">dictionaryrunner.cpp:90</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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 (<tt style="background: #ebebeb; font-size: 13px;">::actionsForMatch</tt>) and need to carry some information to figure out what the match result was or what its actions ought to be, or runability (<tt style="background: #ebebeb; font-size: 13px;">::run</tt>). 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.<br />
That is to say: the runner as-is wouldn't benefit from setData at all.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The current behavior is that KRunner closes when a match is selected.<br />
But if you set the data, for instance:<br />
<tt style="background: #ebebeb; font-size: 13px;">match.setData(QStringLiteral("Hello There!"));</tt><br />
The text <tt style="background: #ebebeb; font-size: 13px;">Hello There!</tt> will show up in KRunner (like in the converter runner).<br />
The run method won't be called, because the match type is set to InformationalMatch.</p>

<p style="padding: 0; margin: 8px;">So the runner would benefit from setData, or am I missing something?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D28192#inline-160961">View Inline</a><span style="color: #4b4d51; font-weight: bold;">sitter</span> wrote in <span style="color: #4b4d51; font-weight: bold;">dictionaryrunner_config.cpp:24</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I wouldn't remove this without good reason.</p>

<p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Thanks for the detailed explanation.<br />
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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D28192#inline-160963">View Inline</a><span style="color: #4b4d51; font-weight: bold;">sitter</span> wrote in <span style="color: #4b4d51; font-weight: bold;">dictionaryrunner_config.cpp:29</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Could you elaborate on why you removed the baseclass calls for load/save/defaults please?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The baseclass implementations for these methods handle the KConfigDialogManager widgets and the corresponding changed signals.<br />
I removed them, because the KConfigDialogManager class is not used in this kcm and the signals are manually handled.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R114 Plasma Addons</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D28192">https://phabricator.kde.org/D28192</a></div></div><br /><div><strong>To: </strong>alex, broulik, ngraham, sitter, mlaurent<br /><strong>Cc: </strong>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<br /></div>