D29123: WIP BUG: 420312. Do not mark entry as uninstalled if uninstallation script failed
alex updated this revision to Diff 81075. alex added a comment. Merge branch 'bugfix_uninstall' REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29123?vs=80976=81075 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
D29123: WIP BUG: 420312. Do not mark entry as uninstalled if uninstallation script failed
alex marked 3 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: WIP BUG: 420312. Do not mark entry as uninstalled if uninstallation script failed
alex updated this revision to Diff 80976. alex added a comment. Use internal question system PS: I am not sure on which branch this should land, thats why I haven't edited the translations. REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29123?vs=80967=80976 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
D29123: WIP BUG: 420312. Do not mark entry as uninstalled if uninstallation script failed
alex added a comment. No problem :-). And good to know that the concept of this patch makes sense ^^. 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: WIP BUG: 420312. Do not mark entry as uninstalled if uninstallation script failed
leinir added a comment. (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 :) ) 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: WIP BUG: 420312. Do not mark entry as uninstalled if uninstallation script failed
leinir requested changes to this revision. leinir added a comment. This revision now requires changes to proceed. 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 :) INLINE COMMENTS > CMakeLists.txt:71 > KF5::CoreAddons > +KF5::WidgetsAddons # KMessageBox error messages > Qt5::Xml No, that's what the Question system is for. No widget stuff in Core, thanks :) > installation.cpp:631 > +// can delete the files manually > +entry.setStatus(KNS3::Entry::Installed); > +KMessageBox::error(nullptr, err); If you are changing the status, you need to also emit entryChanged, otherwise the cache will be inconsistent > installation.cpp:632 > +entry.setStatus(KNS3::Entry::Installed); > +KMessageBox::error(nullptr, err); > +return; 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. > installation.cpp:653 > - > -emit signalEntryChanged(entry); > } 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 :) 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: WIP BUG: 420312. Do not mark entry as uninstalled if uninstallation script failed
alex created this revision. alex added reviewers: KNewStuff, meven, ngraham. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. alex requested review of this revision. REVISION SUMMARY 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. 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. REPOSITORY R304 KNewStuff BRANCH bugfix_install_uninstall_messages (branched from master) REVISION DETAIL https://phabricator.kde.org/D29123 AFFECTED FILES src/core/CMakeLists.txt src/core/engine.cpp src/core/installation.cpp src/core/installation.h To: alex, #knewstuff, meven, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
