<table><tr><td style="">leinir requested changes to this revision.<br />leinir added a comment.<br />This revision now requires changes to proceed.
</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/D29123">View Revision</a></tr></table><br /><div><div><p>Sorry about missing that bic issue before...</p></div></div><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/D29123#inline-166181">View Inline</a><span style="color: #4b4d51; font-weight: bold;">leinir</span> wrote in <span style="color: #4b4d51; font-weight: bold;">installation.cpp:653</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Unless you report the entry as changed, the cache will not be updated and the entire reporting side will fall down. Please put that line back :)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">In reference to the comment about bic issues above (and specifically how the function is /supposed/ to be interpreted), this is exactly the point where that confusion becomes perhaps a little more obvious - i hadn't noticed what you were doing before, expecting that entry instance to change, but as you can see, the signal there is important :)</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/D29123#inline-168778">View Inline</a><span style="color: #4b4d51; font-weight: bold;">installation.h:124</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 style="color: #74777d">     */</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">    <span style="color: #aa4000">void</span> <span style="color: #004012">uninstall</span><span class="p">(</span><span class="n">KNSCore</span><span style="color: #aa2211">::</span><span class="n">EntryInternal</span> <span class="n">entry</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">void</span> <span style="color: #004012">uninstall</span><span class="p">(</span><span class="n">KNSCore</span><span style="color: #aa2211">::</span><span class="n">EntryInternal</span> <span class="bright"></span><span style="color: #aa2211"><span class="bright">&</span></span><span class="n">entry</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Hmm... i find myself wondering what effect this has on BIC... something tells me it might not be so great... Specifically, <a href="https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts" class="remarkup-link" target="_blank" rel="noreferrer">https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts</a> says that you cannot change the signature of existing functions of any type... However, it feels more like a problem with the documentation - the idea is that the entries passed into functions aren't changed (and this really should probably be a const reference, but for some reason it isn't, and again we can't change that for bic-iness), and so really what /should/ happen is that rather than saying "The entry instance will be updated" and so on, what it should be doing is say "If the entry is successfully uninstalled, listening to signalEntryChanged(const KNSCore::EntryInternal &) for an entry equal to the one you have passed in will allow you to detect the result of calling the function".</p>

<p style="padding: 0; margin: 8px;">That's what it's already doing, and how it is used in places which call the function, so probably makes sense to fix that.</p>

<p style="padding: 0; margin: 8px;">In the longer term (think KF6), i would also quite like all the functionality here to end up entirely asynchronous.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R304 KNewStuff</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D29123">https://phabricator.kde.org/D29123</a></div></div><br /><div><strong>To: </strong>alex, KNewStuff, meven, ngraham, leinir<br /><strong>Cc: </strong>leinir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns<br /></div>