KNS: Do not mark entry as installed if install script failed
ClosedPublic

Authored by alex on Tue, May 5, 4:25 PM.

Details

Summary

If the install script failed/was aborted the entry gets marked as installed.
Now the exit code is checked and a error message is displayed.

This is the counterpart of D29123: Do not mark entry as uninstalled if uninstallation script failed.

Test Plan

Make sure to have D29119: Dolphin: Implement package kit for deb/rpm/pacman service packages. Install the deb package of the "Jetbrains" dolphin addon.
When the authorization popup comes press escape. The dolphin installer
shows an error popup and the entry is not marked as installed.
If you are not on a debian based system it should throw an error anyway ;-).

Diff Detail

Repository
R304 KNewStuff
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
alex created this revision.Tue, May 5, 4:25 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptTue, May 5, 4:25 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
alex requested review of this revision.Tue, May 5, 4:25 PM
alex edited the summary of this revision. (Show Details)Tue, May 5, 4:28 PM
alex edited the test plan for this revision. (Show Details)
leinir requested changes to this revision.Wed, May 6, 1:00 PM

A bit nitpicky, that first one, the second's more serious (i'd like to avoid that in new code), but looks good otherwise :)

src/core/installation.cpp
355–365

Not really any reason to store it in a variable so much... In the rest of the codebase, unless the same lambda is used in more than one location, it's defined inline in the connect statement. Also, i realise some of the code has the capture everything thing going on, but that's me being silly when i first learned how to use them - if possible, only capture the things you actually need to capture :)

This revision now requires changes to proceed.Wed, May 6, 1:00 PM
alex updated this revision to Diff 82100.Wed, May 6, 2:03 PM

Make lambda inline, capture variables

leinir requested changes to this revision.Thu, May 7, 8:05 AM
leinir added inline comments.
src/core/installation.cpp
355–365

Remember to give connect an object context for the slot (i did not realise until recently what leaving that out means, but it turns out to be potentially quite bad and crashy in difficult to track ways - in short, if our this instance is destroyed (say, the user quits the application) while an installation is ongoing, this would crash when attempting to emit or call installationFinished - it /should be fine, as i said, in most cases, as installation's a long lifetime object, but also just good style to add that context - see https://wiki.qt.io/New_Signal_Slot_Syntax#New:_connecting_to_simple_function for a detailed explanation, but in short, just add this, before the lambda and you're good to go :) )

This revision now requires changes to proceed.Thu, May 7, 8:05 AM
alex updated this revision to Diff 82177.Thu, May 7, 8:10 AM

Fix connect

Thanks, I wasn't aware of that until now :-)

leinir accepted this revision.Thu, May 7, 8:15 AM
In D29451#665420, @alex wrote:

Fix connect

Thanks, I wasn't aware of that until now :-)

Same, until a couple of weeks ago :D i'd sort of... taken to doing it anyway, because it just seemed nice, but yup, turns out that you really definitely want to do that unless you know very precisely what you're doing :)

src/core/installation.cpp
355–365

Terribly sorry to keep doing this, but... yeah, noticed the old-style overload thing, but since we require higher than Qt 5.6 and already require a sufficiently high version of c++, we can use qOverload instead of the static_cast :) https://doc.qt.io/qt-5/qtglobal.html#qOverload

This revision is now accepted and ready to land.Thu, May 7, 8:15 AM
leinir requested changes to this revision.Thu, May 7, 8:20 AM

(oops, mistakenly marked as accepted, sorry...)

This revision now requires changes to proceed.Thu, May 7, 8:20 AM
alex updated this revision to Diff 82180.Thu, May 7, 9:21 AM

Use qOverload

No problem :-)

leinir accepted this revision.Thu, May 7, 9:29 AM

Thank you :D Land away :)

This revision is now accepted and ready to land.Thu, May 7, 9:29 AM
This revision was automatically updated to reflect the committed changes.