<table><tr><td style="">kossebau 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/D10078" rel="noreferrer">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/D10078#inline-46279" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">dbusadaptor_p.h:1</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Isn't this file compile-time generated from xml?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">It can be, yes. I had switched for reason* to  manually written one, but with the public class API no longer affected by reason* I could/should perhaps switch back.<br />
Nothing really won or lost though by having a non-generated class.</p>

<p style="padding: 0; margin: 8px;">*qdbusxml2cpp picks up the casing of the method names in the D-Bus interface and expects them also to be used in the wrapped class. D-Bus method naming standard seems to be "UpperCase", which also used in the krunner1 interface ("Actions", "Run", "Query").<br />
While in our Qt-style code we prefer method names to be with lowerCaseStart.<br />
And given the purpose of the lib is "being nice&easy to use", I switched to a manually adapted version of the adaptor code. Compare e.g. changed to API of TestRemoteRunner.<br />
Later though, when I went to use QDBusContext for enabling the async match collection, I avoided leaking that detail in the public API and moved all D-Bus stuff to the pimpl object. Where having "UpperCaseStart" methods is not harming public consumers.<br />
Whatever people prefer, no own preference.</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/D10078#inline-46278" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">querymatch.h:35</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Should be pimpl this? once we commit to this we can never add new fields but then the DBus signature is also now or never.. I think we should add subtext and url to the main struct since I always use the two. Originally David and I thought we could put them in properties as they're seldom used but turns out they're not.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Indeed, DBus signature being fixed is also why I wrote the simple struct for now instead of setter/getter pimpl API.<br />
Having learned how to use c++11 initlalizer lists recently I also now fancy their use where possible ;) and in the little sample cpde I played for now using initializer lists worked nicely, no need for explicit setters. No getters used even. After all the runners are just producing that content usually and do no own further processing, but just deliver it.</p>

<p style="padding: 0; margin: 8px;">Putting subtext and url to the main struct if you like. No idea yet how to adapt the marshalling, given those two need to be in the a{sv}, but possibly can be done with some temporary variantmap. No big runtime costs, and will be nicer API.</p>

<p style="padding: 0; margin: 8px;">No strong opinion here as well. You decide and I will write :)</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/D10078#inline-46276" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">querymatch.h:53</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Can we static assert this in the dbus runner somehow? But I guess since it's a separate lib you cannot</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">No idea yet how to automatically ensure this :/ Why are there  no guides with tips & tricks for D-Bus interface types?!</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R308 KRunner</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D10078" rel="noreferrer">https://phabricator.kde.org/D10078</a></div></div><br /><div><strong>To: </strong>kossebau, davidedmundson, broulik<br /><strong>Cc: </strong>ngraham, Frameworks<br /></div>