Looks good, just some minor improvement suggestions for the unittest.

Oops... thanks.

If some code is deleting this job from a slot connected to it, that code needs to be fixed. This hack isn't a fix, it will only create more problems..

Bleh, phabricator sucks. It does show a list of commits, but no way to view (and review) each commit's diff separately?

No objection from me, but I'm no utf-8 expert.

Ah I see, we copy to .part only when the destination actually exists. I had missed that in my earlier comment. Makes sense, actually.

kio_thumbnail is a separate process (like all kioslaves) so a crash doesn't bring down the user-visible application.

Thanks for working on this, here's my review.

Your reasoning seems to forget that there is a first confirmation, in case of overwriting, in the form of the "Overwrite/Skip/Cancel" dialog?

This should only be done if KRun::setEnableExternalBrowser(false) wasn't called, otherwise it will break KParts' BrowserRun subclass and Konqueror's KonqRun (who use KRun to actually find out the mimetype of the file, not to launch another browser).
T
his means adding a bool d->m_externalBrowserEnabled in KRun, since right now we can't know that it was called if the settings don't actually define an external browser.

This makes no sense to me. The whole point of " based on the contents of the URL " is to open images in an image viewer, PDFs in a PDF viewer, etc.
So hardcoding text/html breaks this and opens everything in a webbrowser -- if that's what you want, set a browser app, not "based on the contents of the URL".

https://build.kde.org/job/Frameworks/view/Everything/job/kconfig/job/kf5-qt5%20FreeBSDQt5.12/ green !!! :-)
Well, blue, because blue is the new green (!?).

Using exec() from a job implementation is a big no no, it creates an unexpected re-entrancy, source of all sorts of problems. This cannot be shipped as is.

Recreate socket as suggested

remove another pair of add/remove methods

also add lower-level unittest

Add m_startMode to AkThread to make quitThread() a no-op if NoThread.

Right, this would fail if the part was not using QFile, indeed. Still, at the level of the KParts frameworks itself it's all fine ;)

A better name would be "hasOnlySpaces" (with the opposite return value), but OK.

Pushed as https://commits.kde.org/kinit/c29988a97b3ae7abc0d196a1166eb16ee6c3fa95

Sorry for the delay

ping?

Hi Milian,