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

[Alexander Lohnau]

alex created this revision.
alex added reviewers: KNewStuff, ngraham, leinir.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
alex requested review of this revision.

REVISION 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.

TEST PLAN
  Make sure to have D29119 . 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 ;-).

REPOSITORY
  R304 KNewStuff

BRANCH
  handle_install_script_error (branched from master)

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

AFFECTED FILES
  src/core/installation.cpp

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Alexander Lohnau]

alex edited the summary of this revision.
alex edited the test plan for this revision.

REPOSITORY
  R304 KNewStuff

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Dan Leinir Turthra Jensen]

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


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

INLINE COMMENTS

> installation.cpp:355
>  QProcess* p = runPostInstallationCommand(installedFiles.size() 
> == 1 ? installedFiles.first() : targetPath);
> -connect(p, static_cast QProcess::ExitStatus)>(&QProcess::finished), this, installationFinished);
> +auto finishedLambda = [=](int exitCode, QProcess::ExitStatus) {
> +if (exitCode) {

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

REPOSITORY
  R304 KNewStuff

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Alexander Lohnau]

alex updated this revision to Diff 82100.
alex added a comment.


  Make lambda inline, capture variables

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29451?vs=82004&id=82100

BRANCH
  handle_install_script_error (branched from master)

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

AFFECTED FILES
  src/core/installation.cpp

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Dan Leinir Turthra Jensen]

leinir requested changes to this revision.
leinir added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> installation.cpp:355
>  QProcess* p = runPostInstallationCommand(installedFiles.size() 
> == 1 ? installedFiles.first() : targetPath);
> -connect(p, static_cast QProcess::ExitStatus)>(&QProcess::finished), this, installationFinished);
> +connect(p, static_cast QProcess::ExitStatus)>(&QProcess::finished),
> +[entry, installationFinished, this] (int exitCode, 
> QProcess::ExitStatus) {

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

REPOSITORY
  R304 KNewStuff

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Alexander Lohnau]

alex updated this revision to Diff 82177.
alex added a comment.


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

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29451?vs=82100&id=82177

BRANCH
  arcpatch-D29451

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

AFFECTED FILES
  src/core/installation.cpp

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


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

INLINE COMMENTS

> installation.cpp:355
>  QProcess* p = runPostInstallationCommand(installedFiles.size() 
> == 1 ? installedFiles.first() : targetPath);
> -connect(p, static_cast QProcess::ExitStatus)>(&QProcess::finished), this, installationFinished);
> +connect(p, static_cast QProcess::ExitStatus)>(&QProcess::finished), this,
> +[entry, installationFinished, this] (int exitCode, 
> QProcess::ExitStatus) {

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

REPOSITORY
  R304 KNewStuff

BRANCH
  arcpatch-D29451

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Dan Leinir Turthra Jensen]

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


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

REPOSITORY
  R304 KNewStuff

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Alexander Lohnau]

alex updated this revision to Diff 82180.
alex added a comment.


  Use qOverload
  
  No problem :-)

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29451?vs=82177&id=82180

BRANCH
  arcpatch-D29451

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

AFFECTED FILES
  src/core/installation.cpp

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Thank you :D Land away :)

REPOSITORY
  R304 KNewStuff

BRANCH
  arcpatch-D29451

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Alexander Lohnau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:2838e55acd17: KNS: Do not mark entry as installed if 
install script failed (authored by alex).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29451?vs=82180&id=82189

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

AFFECTED FILES
  src/core/installation.cpp

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns