D10405: Don't proceed in runCommandInternal if the executable doesn't exit
hein updated this revision to Diff 27046. hein added a comment. New approach. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10405?vs=26821=27046 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10405 AFFECTED FILES src/widgets/krun.cpp To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
hein added a comment. It's totally conceivable for e.g. a KPart or a Plasma plugin to open a QWidget-based window where the KMessageBox is appropriate, and if there's only a global interface instance and Plasma overrides it to hide all message boxes it's going to break KIO users in plugins. So I don't think going to a single global is good enough, it would have to be able to set more narrowly. This also bubbles up actually - the KRun calls we're talking about are in libtaskmanager which is technically meant to be UI-agnostic, so it'd have to also expose some way to set the interface instance. I agree all of this is in theory good (we need to make KIO more toolkit-agnostic - I have another giant patch sitting around that's almost-unfinishable that adds QWindow support to some APIs that currently only accept QWidgets I don't even have), but I think it's KIO 6 material and not the quick fix I'm looking for. I found a cludge that's ugly but works to fix the crash - I'll update this in a moment to see how much you hate it. :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
dfaure added a comment. Which plugins? This stuff is for the application to decide how it wants to handle user interactions like the rename dialog, the skip dialog, the confirm-deletion dialog, and messageboxes. KIOWidgets provides a default implementation (with modal dialogs), but it can be overriden by the app if it wants to do this differently. This sounds like something Plasma might want to do, if it wants to avoid modal dialogs (although I'm not sure what it could do instead, since these methods return what the user selected). OK if the real need is for this interface to be async, that's more work indeed. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
hein added a comment. There's a Job::setUiDelegateExtension though in addition to KIO::setDefaultJobUiDelegateExtension. A global would wreak havoc with stuff like plugins, no? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
dfaure added a comment. What? no no, KIO::defaultJobUiDelegateExtension()->requestMessageBox() is a kind of singleton, no refactoring needed. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
hein added a comment. Thanks for the solution for sure, but - it requires writing a million lines of boilerplate, extensively refactoring KRun and adding reams of new overloads to its API (there would need to be some way to pass an instance of that interface to all these - not internal - statics). It's more than I have time/energy for currently and I'm not really sure it's viable without a KIO6. I have another idea, though, let me see if it's viable ... REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
dfaure added a comment. Come on, this is fun! Well, unless you feel like I took the fun out of it by providing a solution... REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
hein added a comment. /o\ I don't like life anymore REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
dfaure added a comment. The statics can be refactored, if they're internal ;) What's more troublesome is the KRun instance might have gone away by the time KProcessRunner ::slotProcessExited is called, though (which is why it's done in a different class). So maybe what's needed is rather some interface, like defaultJobUiDelegateExtension()->requestMessageBox(). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
hein added a comment. @dfaure I don't see how the virtual thing is going to work - there's numerous static affected codepaths so there's no 'this' to pass down to call the non-static error handler on. I could add a new "silent" RunFlag and pass that around to quiet the message box altogether, but this would mean zero error handling. I'm unhappy about that. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
dfaure added a comment. Yes. Well, you can't add new virtual methods, but these two already exist: virtual void handleInitError(int kioErrorCode, const QString ); virtual void handleError(KJob *job); I think you should be able to (ab)use the first one for this purpose. (It's called "init" as opposed to "when running a kjob, later", with the assumption that these are the only two cases that can happen but it really means "when we don't have a kjob"; should have been called something more generic, I see now) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
hein added a comment. Yeah, I had a feeling there were other design considerations here, but they're not documented/commented in the code so the only way to find out was to show you a patch :) You want this virtual method where? In KRun? And then Plasma should subclass KRun? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
dfaure added a comment. (FWIW I tested my desktop file in dolphin, not in plasma) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. If only it was that simple, I would have done this years ago. Download http://www.davidfaure.fr/2018/weirdcommand.desktop and then running that desktop file, without and then with your patch. If works for me without, and I bet it fails with your patch, because "if" isn't an executable. (https://commits.kde.org/kio/0e28930e6748fb45d152e7c3023a034b7db23854 shows what executablePath() will return for that Exec line). The reason this code was in slotProcessExited is that at that point we know something went wrong, and we can try to find out what happened (and if we get the diagnosis slightly wrong, the harm is minimal, an error did happen in any case). OTOH your patch will fail with any sort of shell command in the Exec line, that doesn't start with an executable name. I think the actual fix is to call a virtual method for that messagebox, so that it can be implemented differently in apps that don't like message boxes (not just plasma but also e.g. dolphin). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart, ngraham Cc: #frameworks, michaelh
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
mart added a comment. +1 from me REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart Cc: #frameworks, michaelh, ngraham
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
hein updated this revision to Diff 26821. hein added a comment. Remove unrelated changes. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10405?vs=26820=26821 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10405 AFFECTED FILES src/widgets/krun.cpp To: hein, dfaure, davidedmundson, mart Cc: #frameworks, michaelh, ngraham
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
hein added a comment. Argh, sorry for the noisy diff, I didn't do commit -a but arc did I guess ... I'll clean this up, one sec. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10405 To: hein, dfaure, davidedmundson, mart Cc: #frameworks, michaelh, ngraham
D10405: Don't proceed in runCommandInternal if the executable doesn't exit
hein created this revision. hein added reviewers: dfaure, davidedmundson, mart. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. hein requested review of this revision. REVISION SUMMARY Without this patch, runCommandInternal tries running first and handles a missing executable in slotProcessExited after process execution ended with a non-zero exit code. One of the side-effects is that a startup notification is created even if nothing is ultimately run. With this patch, we check for executable existence first and abort if we can't find it. This fixes a Plasma shell crash in the Task Manager applet: We use KRun from inside the onReleased handler of a MouseArea. In this error case this leads to a KMessageBox spinning the event loop. The delegate hosting the MouseArea is in the meanwhile deleted because it's replaced by a different one for the startup creation. After closing the dialog we're back in the destroyed delegate and crash. BUG:385942 REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D10405 AFFECTED FILES src/core/kprotocolinfofactory.cpp src/widgets/jobuidelegate.cpp src/widgets/jobuidelegate.h src/widgets/krun.cpp To: hein, dfaure, davidedmundson, mart Cc: #frameworks, michaelh, ngraham
