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

[Alexander Lohnau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:a2ecdd333c49: Do not mark entry as uninstalled if 
uninstallation script failed (authored by alex).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29123?vs=82228=82606

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/installation.cpp
  src/core/installation.h

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

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

[Alexander Lohnau]

alex marked 4 inline comments as done.

REPOSITORY
  R304 KNewStuff

BRANCH
  arcpatch-D29123_1

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

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

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

[Alexander Lohnau]

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


  Update docs
  
  I adjusted your suggestion a bit, the signalEntryChanged gets also emitted, 
when
  the user cancels the uninstallation :-).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29123?vs=82114=82228

BRANCH
  arcpatch-D29123_1

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/installation.cpp
  src/core/installation.h

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

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

[Nathaniel Graham]

ngraham accepted this revision.

REPOSITORY
  R304 KNewStuff

BRANCH
  arcpatch-D29123_1

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

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

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

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> installation.h:113
>   *
>   * The entry instance will be updated with any new information:
>   * 

Not blocking, but the documentation probably wants fixing before you push - 
just swap this line for "The entry emitted by signalEntryChanged (only happens 
when the uninstallation is completed with non-critical errors) will be updated 
with any new information, in particular the following:"

REPOSITORY
  R304 KNewStuff

BRANCH
  arcpatch-D29123_1

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

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

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

[Alexander Lohnau]

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


  Ensure bic

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29123?vs=81918=82114

BRANCH
  arcpatch-D29123_1

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/installation.cpp
  src/core/installation.h

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

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

[Alexander Lohnau]

alex added inline comments.

INLINE COMMENTS

> leinir wrote in installation.h:124
> 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.

I told myself yesterday that I am going to have a look at this patch again^^
And you are absolutely right :-)

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

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

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

[Alexander Lohnau]

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


  Merge branch master

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29123?vs=81476=81918

BRANCH
  arcpatch-D29123_1

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/installation.cpp
  src/core/installation.h

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

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

[Alexander Lohnau]

alex marked 2 inline comments as done.

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

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

[Alexander Lohnau]

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


  Display more technical information in error message

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29123?vs=81076=81476

BRANCH
  arcpatch-D29123

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/installation.cpp
  src/core/installation.h

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

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

[Alexander Lohnau]

alex added inline comments.

INLINE COMMENTS

> leinir wrote in installation.cpp:632
> This will want to be more... question like... "The thing failed" isn't really 
> a question, not sure how the user's supposed to make an informed choice based 
> on that... Perhaps something like "The uninstallation process failed to run 
> the command %1. The output was:\n%2\nIf you think this is incorrect, you can 
> continue, or you can cancel the process." (given how much this is an error 
> situation, it feels like we can give the user a bit of technical 
> information... cancelling in a panic would be the appropriate reaction to "I 
> don't know" anyway for this sort of thing, so thinking we'd be ok with doing 
> that).

Yes I get what you mean :-).

I was just not sure where this patch should land (I asked about this in  a 
previous comment) and wanted to know this before editing translations.
But I guess it will go on master then?

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

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.


  On a related note, i'm waiting on reviews on D28701 
 at the moment...  which i'm afraid might 
wreck some havoc with your patch, as they touch some of the same bits of the 
codebase.

INLINE COMMENTS

> installation.cpp:632
> +Question question(Question::ContinueCancelQuestion);
> +question.setQuestion(err);
> +Question::Response response = question.ask();

This will want to be more... question like... "The thing failed" isn't really a 
question, not sure how the user's supposed to make an informed choice based on 
that... Perhaps something like "The uninstallation process failed to run the 
command %1. The output was:\n%2\nIf you think this is incorrect, you can 
continue, or you can cancel the process." (given how much this is an error 
situation, it feels like we can give the user a bit of technical information... 
cancelling in a panic would be the appropriate reaction to "I don't know" 
anyway for this sort of thing, so thinking we'd be ok with doing that).

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

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

[Alexander Lohnau]

alex edited the summary of this revision.
alex added a dependency: D29101: KNewStuff: Fix file path and process call.

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

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

[Alexander Lohnau]

alex updated this revision to Diff 81076.
alex retitled this revision from "WIP BUG: 420312. Do not mark entry as 
uninstalled if uninstallation script failed" to "Do not mark entry as 
uninstalled if uninstallation script failed".
alex edited the summary of this revision.
alex added a comment.


  Emit signalEntryChanged for manually deletet entry

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29123?vs=81075=81076

BRANCH
  bugfix_install_uninstall_messages (branched from master)

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/installation.cpp
  src/core/installation.h

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