- User Since
- Mar 5 2015, 12:44 PM (188 w, 5 d)
Mon, Oct 15
Sun, Oct 14
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).
Sat, Oct 13
add bug number
Fri, Oct 12
This whole method could put placesModel->url(index) into a local variable to avoid calling it so many times, though.
Thu, Oct 11
Mon, Oct 8
Sun, Oct 7
+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 <firstname.lastname@example.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?
Sat, Oct 6
This commit breaks the unittest "scalabletest" :
Thu, Oct 4
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).
Wed, Oct 3
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).
Tue, Oct 2
Fri, Sep 28
I completely forgot about pushing it, thanks for the reminder and for the target branch information.
Tue, Sep 25
Sun, Sep 23
Patch looks ok but I'm surprised by the commit log. Doesn't this method also work for directories, even after the patch?
Sat, Sep 22
This is excellent work, thanks a lot for doing this. I just have "a few" minor comments... ;)
Fri, Sep 21
The question of the caching of the KUser usage remains, though.
Thu, Sep 20
This is technically not 100% source compatible, but since the callers were already passing "TARGET" (and others) as documented, I guess it's fine.
Wed, Sep 19
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.
Tue, Sep 18
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.
Mon, Sep 17
Shouldn't this skip the invalid bookmark rather than abort completely ?
FullyEncoded makes perfect sense for sending in a protocol implementation.
Sep 17 2018
Looks OK now.
Do all frameworks build with this change?
Sep 16 2018
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.
Sep 15 2018
Thanks, pushed to master.