You're right. Fixed, thanks.

See also https://phabricator.kde.org/D16183

I would certainly like a style reformat, but I'm not sure who is the current/active maintainer for this code.

Looks good, but this might be worth fixing in QTest::qWaitForWindowActive itself, for all other unittests using that (including Qt's own).

add bug number

This whole method could put placesModel->url(index) into a local variable to avoid calling it so many times, though.

+2, assuming the commit log doesn't have M$ anymore (the phab review does, but you don't use arc apparently so I guess the two things are unrelated)

This looks like the kind of commit where a review by Oswald Buddenhagen <ossi@kde.org> would be useful.

I don't really know, but see https://phabricator.kde.org/D4254 for the full reasoning of the unittest.

Possible fix in https://phabricator.kde.org/D16020

More generally, please please run the unittests after changing a class. I hate that I currently am the one doing this, on the day of the release, which forces me to bugfix such things, or delay the release.

This commit breaks kfileplacesmodeltest.

Will you port the kreadconfig usage in startkde to add --ignore-globals --ignore-cascading in order to preserve performance?

This commit breaks the unittest "scalabletest" :

Great stuff. Only found some typos and very minor things, feel free to fix and push.

I guess the implicit part of the question is "what if kio-extras isn't installed ?".
I assume it will lead to a broken item.

No ECM foo for local copying, so this CMakeLists.txt looks fine to me.

I think I'm fine with it now, but please wait until next Monday (Oct 8) before pushing, so it doesn't break the upcoming KF5 release. I'm not 100% confident.... (given that earlier versions of the patch had obvious bugs that were not detected by the unittests).

kio_file returns KIO::ERR_CANNOT_RMDIR when trying to delete a non-empty directory, maybe kio_smb could do the same? Although I see that the above isn't dependent on the command being executed.... But well, is there any other operation that would return ENOTEMPTY? If not, then ERR_CANNOT_RMDIR would be better IMHO (so KIO code or app code can check for it, if needed).

I completely forgot about pushing it, thanks for the reminder and for the target branch information.

Patch looks ok but I'm surprised by the commit log. Doesn't this method also work for directories, even after the patch?

This is excellent work, thanks a lot for doing this. I just have "a few" minor comments... ;)

Makes sense.

The question of the caching of the KUser usage remains, though.

This is technically not 100% source compatible, but since the callers were already passing "TARGET" (and others) as documented, I guess it's fine.

I agree, extensions are not reliable over HTTP, which is why mimeTypeForUrl doesn't use them for HTTP urls.
But here we're in KFileItem, so much more likely talking about local files or FTP/SFTP/FISH/SMB/etc. where the *.php issue doesn't happen.

loadMimeTypeIcon also has the fallback to application-octet-stream if the mimetype icon isn't found, you could pass that as 2nd argument to fromTheme().

In code that is specific to images, I have no problem with determination from content.

(BTW thanks for what you said about FUSE, I agree 100% and it makes me glad to see some people from the opposite camp, when so many people are trying to convince me that network mounts via FUSE is the solution to all problems on earth, see bug 75324)

No, no. Too unreliable and against the MIME spec.

Shouldn't this skip the invalid bookmark rather than abort completely ?

FullyEncoded makes perfect sense for sending in a protocol implementation.

Looks OK now.

Do all frameworks build with this change?

I would sprinkle some const on local variables, but looks good otherwise.

"perhaps fix storedPut to do that?" --> I'm not in favour. Generally speaking, we want to do this at the highest level possible, so that batching N jobs doesn't trigger N notifications.

Yes to chunking.

Thanks, pushed to master.