<table><tr><td style="">sitter 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-160950">View Inline</a><span style="color: #4b4d51; font-weight: bold;">config_keys.h:5</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> */</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Include guards please</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-160951">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dictionarymatchengine.cpp:85</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">m_wordLock</span><span class="p">.</span><span class="n">lockForRead</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">for<span class="bright">each</span></span> <span class="p">(</span><span class="n">ThreadData</span> <span style="color: #aa2211">*</span><span class="n">data<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span> <span class="n">m_lockers</span><span class="p">.</span><span class="n">values</span><span class="p">(</span><span class="n">source</span><span class="p">))</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span class="n">ThreadData</span> <span style="color: #aa2211">*</span><span style="color: #a0a000">data<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">:</span></span> <span class="n">m_lockers</span><span class="p">.</span><span class="n">values</span><span class="p">(</span><span class="n">source</span><span class="p">))</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">QMutexLocker</span> <span class="n">locker</span><span class="p">(</span><span style="color: #aa2211">&</span><span class="n">data</span><span style="color: #aa2211">-></span><span class="n">mutex</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I think our style is space before and after :</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-160952">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dictionaryrunner.cpp:37</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">setSyntaxes</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">QList</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright"><</span></span><span class="bright"></span><span class="n"><span class="bright">Plasma</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">RunnerSyntax</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">></span></span><span class="bright"></span><span class="p"><span class="bright">()</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright"><<</span></span><span class="bright"> </span><span class="n"><span class="bright">Plasma</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">RunnerSyntax</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">Plasma</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">RunnerSyntax</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">i18nc</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"Dictionary keyword"</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span style="color: #766510"><span class="bright">"%1:q:"</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">m_triggerWord</span></span><span class="bright"></span><span class="p"><span class="bright">),</span></span><span class="bright"> </span><span class="n"><span class="bright">i18n</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"Finds the definition of :q:."</span></span><span class="bright"></span><span class="p"><span class="bright">))));</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span class="p"><span class="bright">}</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">setSyntaxes</span><span class="p">({</span><span class="n">Plasma</span><span style="color: #aa2211">::</span><span class="n">RunnerSyntax</span><span class="p">(</span><span class="n">Plasma</span><span style="color: #aa2211">::</span><span class="n">RunnerSyntax</span><span class="p">(</span><span class="n">i18nc</span><span class="p">(</span><span style="color: #766510">"Dictionary keyword"</span><span class="p">,</span> <span style="color: #766510">"%1:q:"</span><span class="p">,</span> <span class="n">m_triggerWord</span><span class="p">),</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">i18n</span><span class="p">(</span><span style="color: #766510">"Finds the definition of :q:."</span><span class="p">)))});</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This constructs the same RunnerSyntax twice does it not?</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-160953">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dictionaryrunner.cpp:75</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">QString</span> <span class="n">lastPartOfSpeech</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">for<span class="bright">each</span></span> <span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">line<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n">lines</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="n">partOfSpeech<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">indexIn</span></span><span class="p">(</span><span class="n">line</span><span class="p">)<span class="bright"></span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">==</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">-</span></span><span class="bright"></span><span style="color: #601200"><span class="bright">1</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span style="color: #a0a000">line<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">:</span></span><span class="bright"> </span><span class="n"><span class="bright">qAsConst</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="n">lines<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">)</span>)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span class="n"><span class="bright">QRegularExpressionMatch</span></span><span class="bright"> </span><span class="n"><span class="bright">partOfSpeechMatch</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">m_</span>partOfSpeech<span class="bright">Regex</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">match</span></span><span class="p">(</span><span class="n">line</span><span class="p">)<span class="bright">;</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;"> : </tt></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-160957">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dictionaryrunner.cpp:90</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// TODO What to set as data?</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">//match.setData(QVariant());</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">matches</span><span class="p">.</span><span class="n">append</span><span class="p">(</span><span class="n">match</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><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><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;">dictionaryrunner_config.cpp:24</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">connect</span><span class="p">(</span><span class="n">m_triggerWord</span><span class="p">,</span> <span class="bright"></span><span class="n"><span class="bright">SIGNAL</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="n">textChanged<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">QString</span></span><span class="bright"></span><span class="p"><span class="bright">))</span>,</span> <span style="color: #aa4000">this</span><span class="p">,</span> <span class="bright"></span><span class="n"><span class="bright">SLOT</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">c</span>hanged<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">())</span>);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">load</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">connect</span><span class="p">(</span><span class="n">m_triggerWord</span><span class="p">,</span> <span class="bright"></span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"></span><span class="n"><span class="bright">QLineEdit</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="n">textChanged</span><span class="p">,</span> <span style="color: #aa4000">this</span><span class="p">,</span> <span class="bright"></span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"></span><span class="n"><span class="bright">DictionaryRunnerConfig</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">markAsC</span>hanged</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><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><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;">dictionaryrunner_config.cpp:29</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span class="bright"> </span><span class="n"><span class="bright">KCModule</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">load</span></span><span class="bright"></span><span class="p"><span class="bright">(</span>);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">KSharedConfig</span><span style="color: #aa2211">::</span><span class="n">Ptr</span> <span class="n">cfg</span> <span style="color: #aa2211">=</span> <span class="n">KSharedConfig</span><span style="color: #aa2211">::</span><span class="n">openConfig</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"krunnerrc"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="bright"> </span><span class="n"><span class="bright">KConfigGroup</span></span><span class="bright"> </span><span class="n"><span class="bright">grp</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">KSharedConfig</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">openConfig</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">QStringLiteral</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"krunnerrc"</span></span><span class="bright"></span><span class="p"><span class="bright">))</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">group</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"Runners"</span></span><span class="bright"></span><span class="p"><span class="bright">).</span></span><span class="bright"></span><span class="n"><span class="bright">group</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"Dictionary"</span></span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><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></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>