D29123: Do not mark entry as uninstalled if uninstallation script failed

[Dan Leinir Turthra Jensen]

leinir requested changes to this revision.
leinir added a comment.
This revision now requires changes to proceed.


  Sorry about missing that bic issue before...

INLINE COMMENTS

> leinir wrote in installation.cpp:653
> 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 :)

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 :)

> installation.h:124
>       */
> -    void uninstall(KNSCore::EntryInternal entry);
>  

Hmm... i find myself wondering what effect this has on BIC... something tells me it might not be so great... Specifically, https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts 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".

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.

In the longer term (think KF6), i would also quite like all the functionality here to end up entirely asynchronous.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D29123

To: alex, #knewstuff, meven, ngraham, leinir
Cc: leinir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200506/9eb27edc/attachment.html>