Do not mark entry as uninstalled if uninstallation script failed
ClosedPublic

Authored by alex on Apr 23 2020, 9:18 AM.

Details

Summary

BUG: 420312
As described in the bug report the uninstallation failed, but the
service was marked as removed. Now the service gets only marked as
uninstalled if the script runs without an error. If there is an error
the user gets a popup.

Depends on D29101

Test Plan

Set the exit code to 1 and try to install a dolphin plugin.
You should get an error message.

Without modifying the exit code you should be able to install services.

The manual deletion can be tested by removing the installed service file. for example:
rm ~/.local/share/kservices5/ServiceMenus/iso_mounter_unmounter.desktop
Then the uninstaller will crash:
"Failed to remove .desktop file ... No such file or directory"
Then you delete the installed file manually:
rm ~/.local/share/servicemenu-download/iso_mounter_unmounter.desktop
And now you can click the uninstall button and it gets removed
from the list of installed services.

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.Apr 23 2020, 9:18 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 23 2020, 9:18 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
alex requested review of this revision.Apr 23 2020, 9:18 AM
leinir requested changes to this revision.Apr 23 2020, 9:28 AM
leinir added a subscriber: leinir.

The reporting side of this seems based on a misunderstanding of what the UI-less Core is supposed to be doing... The conceptual intention in general isn't bad, but it needs a bit of work. Thanks for spotting it, too :)

src/core/CMakeLists.txt
71 ↗(On Diff #80967)

No, that's what the Question system is for. No widget stuff in Core, thanks :)

src/core/installation.cpp
834

If you are changing the status, you need to also emit entryChanged, otherwise the cache will be inconsistent

835

As you are already issuing the signal with the error, intercept that instead. Don't spawn widgets from Core, that adds a widget dependency to the Qtquick module.

859

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

This revision now requires changes to proceed.Apr 23 2020, 9:28 AM

(and now i've done it myself, terribly sorry about that, missed the WIP at the start of the title! Hope some of my comments were useful, though :) )

alex added a comment.Apr 23 2020, 9:42 AM

No problem :-). And good to know that the concept of this patch makes sense ^^.

alex updated this revision to Diff 80976.Apr 23 2020, 10:16 AM

Use internal question system

PS: I am not sure on which branch this should land,
thats why I haven't edited the translations.

alex marked 3 inline comments as done.Apr 23 2020, 10:18 AM
alex updated this revision to Diff 81075.Apr 24 2020, 8:45 AM

Merge branch 'bugfix_uninstall'

alex updated this revision to Diff 81076.Apr 24 2020, 8:50 AM
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. (Show Details)

Emit signalEntryChanged for manually deletet entry

leinir requested changes to this revision.Apr 24 2020, 9:16 AM

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.

src/core/installation.cpp
833

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

This revision now requires changes to proceed.Apr 24 2020, 9:16 AM
alex added inline comments.Apr 24 2020, 9:26 AM
src/core/installation.cpp
833

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?

alex updated this revision to Diff 81476.Apr 28 2020, 8:30 PM

Display more technical information in error message

alex marked 2 inline comments as done.May 3 2020, 4:12 PM
alex updated this revision to Diff 81918.May 4 2020, 4:38 PM

Merge branch master

leinir requested changes to this revision.May 6 2020, 1:14 PM

Sorry about missing that bic issue before...

src/core/installation.cpp
859

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

src/core/installation.h
128

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.

This revision now requires changes to proceed.May 6 2020, 1:14 PM
alex added inline comments.May 6 2020, 3:39 PM
src/core/installation.h
128

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

alex updated this revision to Diff 82114.May 6 2020, 3:51 PM

Ensure bic

leinir accepted this revision.May 7 2020, 9:56 AM
leinir added inline comments.
src/core/installation.h
117

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

This revision is now accepted and ready to land.May 7 2020, 9:56 AM
ngraham accepted this revision.May 7 2020, 1:19 PM
alex updated this revision to Diff 82228.May 7 2020, 6:36 PM

Update docs

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

alex marked 4 inline comments as done.May 11 2020, 6:30 PM
This revision was automatically updated to reflect the committed changes.