Re: Review Request 121092: Use QSslCertificate::fromDevice to load certs in KDE SSL Certificates dialog
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121092/ --- (Updated Nov. 17, 2014, 5:32 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs. Bugs: 333079 http://bugs.kde.org/show_bug.cgi?id=333079 Repository: kdelibs Description --- This patch changes the use of QSslCertificate::fromPath to QSslCertificate::fromDevice when importing certificates in the SSL certificate dialog. This change is necessary because QSslCertificate::fromPath in Qt4 does not seem to be able to read certs in DER format. This problem, whatever it was, seems to be have been resolved in Qt5. Diffs - kio/kssl/kcm/cacertificatespage.cpp 0a269a3a3fff41ffcc93bfb639f891dbf9016d66 Diff: https://git.reviewboard.kde.org/r/121092/diff/ Testing --- Attempt to import the cert in DER format outlined in the bug report. I have also attached a simple program that tests all the different ways one can load certs using QSslCertificate. File Attachments test program https://git.reviewboard.kde.org/media/uploaded/files/2014/11/10/797589bb-56fd-4ee0-a45a-4a9694db1e88__test_qsslcertificate_import.cpp Thanks, Dawit Alemayehu
Review Request 121092: Use QSslCertificate::fromDevice to load certs in KDE SSL Certificates dialog
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121092/ --- Review request for kdelibs. Bugs: 333079 http://bugs.kde.org/show_bug.cgi?id=333079 Repository: kdelibs Description --- This patch changes the use of QSslCertificate::fromPath to QSslCertificate::fromDevice when importing certificates in the SSL certificate dialog. This change is necessary because QSslCertificate::fromPath in Qt4 does not seem to be able to read certs in DER format. This problem, whatever it was, seems to be have been resolved in Qt5. Diffs - kio/kssl/kcm/cacertificatespage.cpp 0a269a3a3fff41ffcc93bfb639f891dbf9016d66 Diff: https://git.reviewboard.kde.org/r/121092/diff/ Testing --- Attempt to import the cert in DER format outlined in the bug report. I have also attached a simple program that tests all the different ways one can load certs using QSslCertificate. File Attachments test program https://git.reviewboard.kde.org/media/uploaded/files/2014/11/10/797589bb-56fd-4ee0-a45a-4a9694db1e88__test_qsslcertificate_import.cpp Thanks, Dawit Alemayehu
Re: Review Request 120975: Allow user to cancel out of the SSL certificate accept duration dialog
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120975/ --- (Updated Nov. 6, 2014, 12:41 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs. Bugs: 335375 http://bugs.kde.org/show_bug.cgi?id=335375 Repository: kdelibs Description --- This patch allows a user to change his/her mind about accepting SSL cetificate that failed verification after they have already chosen to accept and were being prompted for the duration of acceptance. With this change the user can click on cancel or [x] and go back to the previous, the accept/reject certificate, dialog. Diffs - kio/kio/tcpslavebase.cpp cdf28f015711f340460af2307cc08a6045eb31d2 Diff: https://git.reviewboard.kde.org/r/120975/diff/ Testing --- File Attachments Accept dialog with cancel button https://git.reviewboard.kde.org/media/uploaded/files/2014/11/04/bb4e2153-f6b3-4ca9-9645-1783578143a3__ssl_cert_accept_dialog.png Thanks, Dawit Alemayehu
Review Request 120975: Allow user to cancel out of the SSL certificate accept duration dialog
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120975/ --- Review request for kdelibs. Bugs: 335375 http://bugs.kde.org/show_bug.cgi?id=335375 Repository: kdelibs Description --- This patch allows a user to change his/her mind about accepting SSL cetificate that failed verification after they have already chosen to accept and were being prompted for the duration of acceptance. With this change the user can click on cancel or [x] and go back to the previous, the accept/reject certificate, dialog. Diffs - kio/kio/tcpslavebase.cpp cdf28f015711f340460af2307cc08a6045eb31d2 Diff: https://git.reviewboard.kde.org/r/120975/diff/ Testing --- File Attachments Accept dialog with cancel button https://git.reviewboard.kde.org/media/uploaded/files/2014/11/04/bb4e2153-f6b3-4ca9-9645-1783578143a3__ssl_cert_accept_dialog.png Thanks, Dawit Alemayehu
Re: Review Request 120418: kio_webdav: Added 'copyFromFile' support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120418/ --- (Updated Oct. 2, 2014, 12:47 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs. Bugs: 334192 http://bugs.kde.org/show_bug.cgi?id=334192 Repository: kdelibs Description --- This patch adds support for 'copyFromFile' which not only improve performance of copying files from local filesystem to a webdav server, but also fixes the bug reported in bug #334192. Diffs - kioslave/http/http.h c447b69 kioslave/http/http.cpp 7e2bca7 kioslave/http/webdav.protocol c0fbd11 Diff: https://git.reviewboard.kde.org/r/120418/diff/ Testing --- Copy file from local directory to a webdav server. Thanks, Dawit Alemayehu
Re: Review Request 120418: kio_webdav: Added 'copyFromFile' support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120418/ --- (Updated Sept. 30, 2014, 11:06 a.m.) Review request for kdelibs. Changes --- Fixed logic errors after extensive testing. Bugs: 334192 http://bugs.kde.org/show_bug.cgi?id=334192 Repository: kdelibs Description --- This patch adds support for 'copyFromFile' which not only improve performance of copying files from local filesystem to a webdav server, but also fixes the bug reported in bug #334192. Diffs (updated) - kioslave/http/webdav.protocol c0fbd11 kioslave/http/http.cpp 7e2bca7 kioslave/http/http.h c447b69 Diff: https://git.reviewboard.kde.org/r/120418/diff/ Testing --- Copy file from local directory to a webdav server. Thanks, Dawit Alemayehu
Re: Review Request 120418: kio_webdav: Added 'copyFromFile' support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120418/ --- (Updated Sept. 30, 2014, 11:39 a.m.) Review request for kdelibs. Changes --- Reverted most of the previous change since the error was caused by incorrect conversion of URL protocol and nothing else. Bugs: 334192 http://bugs.kde.org/show_bug.cgi?id=334192 Repository: kdelibs Description --- This patch adds support for 'copyFromFile' which not only improve performance of copying files from local filesystem to a webdav server, but also fixes the bug reported in bug #334192. Diffs (updated) - kioslave/http/http.h c447b69 kioslave/http/http.cpp 7e2bca7 kioslave/http/webdav.protocol c0fbd11 Diff: https://git.reviewboard.kde.org/r/120418/diff/ Testing --- Copy file from local directory to a webdav server. Thanks, Dawit Alemayehu
Review Request 120439: kio_webdav: Fix folder creation on apache + mod_dav webdav server
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120439/ --- Review request for kdelibs. Bugs: 322550 http://bugs.kde.org/show_bug.cgi?id=322550 Repository: kdelibs Description --- The attached single line patch fixes a problem with creation of directories on apache + mod_dav WebDAV server that results from us not reading the response the server sent back once the folder create request is fulfilled. This causes apache to refuse any further requests. Diffs - kioslave/http/http.cpp 7e2bca7 Diff: https://git.reviewboard.kde.org/r/120439/diff/ Testing --- Tested on own local apache + mod_dav server. Thanks, Dawit Alemayehu
Re: Review Request 120439: kio_webdav: Fix folder creation on apache + mod_dav webdav server
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120439/ --- (Updated Oct. 1, 2014, 5:15 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs. Bugs: 322550 http://bugs.kde.org/show_bug.cgi?id=322550 Repository: kdelibs Description --- The attached single line patch fixes a problem with creation of directories on apache + mod_dav WebDAV server that results from us not reading the response the server sent back once the folder create request is fulfilled. This causes apache to refuse any further requests. Diffs - kioslave/http/http.cpp 7e2bca7 Diff: https://git.reviewboard.kde.org/r/120439/diff/ Testing --- Tested on own local apache + mod_dav server. Thanks, Dawit Alemayehu
Review Request 120418: kio_webdav: Added 'copyFromFile' support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120418/ --- Review request for kdelibs. Bugs: 334192 http://bugs.kde.org/show_bug.cgi?id=334192 Repository: kdelibs Description --- This patch adds support for 'copyFromFile' which not only improve performance of copying files from local filesystem to a webdav server, but also fixes the bug reported in bug #334192. Diffs - kioslave/http/http.h c447b69 kioslave/http/http.cpp 7e2bca7 kioslave/http/webdav.protocol c0fbd11 Diff: https://git.reviewboard.kde.org/r/120418/diff/ Testing --- Copy file from local directory to a webdav server. Thanks, Dawit Alemayehu
Re: Review Request 120178: Set the kio_http responsecode metadata on error
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120178/ --- (Updated Sept. 19, 2014, 11:25 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs. Bugs: 337198 http://bugs.kde.org/show_bug.cgi?id=337198 Repository: kdelibs Description --- The attached patch sets the HTTP responsecode metadata on error. Diffs - kioslave/http/http.cpp 1068eb0e7780dd02f3af284e5d1ba932c06f4e1f Diff: https://git.reviewboard.kde.org/r/120178/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 120182: KIO::CopyJob: Do not query for free space when filesystem type is unknown
On Sept. 19, 2014, 7:03 p.m., Andrea Iacovitti wrote:
I don't know this code but David's proposed patch differs from the one
committed
if (m_asMethod || !fileInfo.exists()) {
vs
if (m_asMethod !fileInfo.exists()) {
It is a typo on my part. I will fix it.
- Dawit
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120182/#review66976
---
On Sept. 19, 2014, 11:25 a.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120182/
---
(Updated Sept. 19, 2014, 11:25 a.m.)
Review request for kdelibs and David Faure.
Bugs: 336529
http://bugs.kde.org/show_bug.cgi?id=336529
Repository: kdelibs
Description
---
This patch stops KIO::CopyJob from querying for free disk space when the
filesystem type is unknown.
Diffs
-
kio/kio/copyjob.cpp 713255b
Diff: https://git.reviewboard.kde.org/r/120182/diff/
Testing
---
Mounted Andriod filesystem through sshfs and attempted to copy files through
sftp.
Thanks,
Dawit Alemayehu
Re: Review Request 120182: KIO::CopyJob: Do not query for free space when filesystem type is unknown
On Sept. 16, 2014, 3:39 p.m., David Faure wrote: better idea, testing for KDiskFreeSpaceInfo's success. http://www.davidfaure.fr/2014/copyjob.cpp.diff That works as well. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120182/#review66682 --- On Sept. 13, 2014, 12:38 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120182/ --- (Updated Sept. 13, 2014, 12:38 p.m.) Review request for kdelibs and David Faure. Bugs: 336529 http://bugs.kde.org/show_bug.cgi?id=336529 Repository: kdelibs Description --- This patch stops KIO::CopyJob from querying for free disk space when the filesystem type is unknown. Diffs - kio/kio/copyjob.cpp 713255b123d284bbbaab67073466605efdd96aee Diff: https://git.reviewboard.kde.org/r/120182/diff/ Testing --- Mounted Andriod filesystem through sshfs and attempted to copy files through sftp. Thanks, Dawit Alemayehu
Re: Review Request 120182: KIO::CopyJob: Do not query for free space when filesystem type is unknown
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120182/ --- (Updated Sept. 17, 2014, 12:55 p.m.) Review request for kdelibs and David Faure. Changes --- Changed patch to do what David suggested. Bugs: 336529 http://bugs.kde.org/show_bug.cgi?id=336529 Repository: kdelibs Description --- This patch stops KIO::CopyJob from querying for free disk space when the filesystem type is unknown. Diffs (updated) - kio/kio/copyjob.cpp 713255b Diff: https://git.reviewboard.kde.org/r/120182/diff/ Testing --- Mounted Andriod filesystem through sshfs and attempted to copy files through sftp. Thanks, Dawit Alemayehu
Re: Review Request 120182: KIO::CopyJob: Do not query for free space when filesystem type is unknown
On Sept. 17, 2014, 7:49 p.m., David Faure wrote: I like this patch :-) Well, maybe we could even get rid of the fsType checks? I was going to suggest the same thing. For our purposes here, we do not care if the filesystem type is identified. We only care about whether or not the stat'ing the directory succeeded. I will remove that call. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120182/#review66775 --- On Sept. 17, 2014, 12:55 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120182/ --- (Updated Sept. 17, 2014, 12:55 p.m.) Review request for kdelibs and David Faure. Bugs: 336529 http://bugs.kde.org/show_bug.cgi?id=336529 Repository: kdelibs Description --- This patch stops KIO::CopyJob from querying for free disk space when the filesystem type is unknown. Diffs - kio/kio/copyjob.cpp 713255b Diff: https://git.reviewboard.kde.org/r/120182/diff/ Testing --- Mounted Andriod filesystem through sshfs and attempted to copy files through sftp. Thanks, Dawit Alemayehu
Re: Review Request 120182: KIO::CopyJob: Do not query for free space when filesystem type is unknown
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120182/ --- (Updated Sept. 18, 2014, 3:52 a.m.) Review request for kdelibs and David Faure. Changes --- Removed the check for filesystem type. Bugs: 336529 http://bugs.kde.org/show_bug.cgi?id=336529 Repository: kdelibs Description --- This patch stops KIO::CopyJob from querying for free disk space when the filesystem type is unknown. Diffs (updated) - kio/kio/copyjob.cpp 713255b Diff: https://git.reviewboard.kde.org/r/120182/diff/ Testing --- Mounted Andriod filesystem through sshfs and attempted to copy files through sftp. Thanks, Dawit Alemayehu
Review Request 120178: Set the kio_http responsecode metadata on error
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120178/ --- Review request for kdelibs. Bugs: 337198 http://bugs.kde.org/show_bug.cgi?id=337198 Repository: kdelibs Description --- The attached patch sets the HTTP responsecode metadata on error. Diffs - kioslave/http/http.cpp 1068eb0e7780dd02f3af284e5d1ba932c06f4e1f Diff: https://git.reviewboard.kde.org/r/120178/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 120182: KIO::CopyJob: Do not query for free space when filesystem type is unknown
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120182/ --- Review request for kdelibs and David Faure. Bugs: 336529 http://bugs.kde.org/show_bug.cgi?id=336529 Repository: kdelibs Description --- This patch stops KIO::CopyJob from querying for free disk space when the filesystem type is unknown. Diffs - kio/kio/copyjob.cpp 713255b123d284bbbaab67073466605efdd96aee Diff: https://git.reviewboard.kde.org/r/120182/diff/ Testing --- Mounted Andriod filesystem through sshfs and attempted to copy files through sftp. Thanks, Dawit Alemayehu
Re: Review Request 119211: Queue CMD_REPARSECONFIGRATION requests for suspended ioslaves
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119211/ --- (Updated Aug. 30, 2014, 2:07 p.m.) Status -- This change has been discarded. Review request for kdelibs, David Faure and Thiago Macieira. Repository: kdelibs Description --- This patch addresses the issue where attempting to send CMD_REPARSECONFIGURATION to suspended ioslaves results in a freezing of the application that sent the request. This problem can be clearly reproduced by following the steps outlined in the bug report. I am not entirely sure whether or not this fix really belongs in kio/connection.cpp, but I could not find a more convenient location to queue the command request to ensure the ioslave receives the reparse configuration command if it is ever reused again. Diffs - kio/kio/connection.cpp 99aea0b Diff: https://git.reviewboard.kde.org/r/119211/diff/ Testing --- Run the steps given in the bug report before and after the fix. Thanks, Dawit Alemayehu
Re: Review Request 116570: Ask user for confirmation before doing POST - POST redirection in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116570/ --- (Updated Aug. 30, 2014, 2:09 p.m.) Status -- This change has been discarded. Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- This patch is a companion to the recent POST-POST redirection implementation in KIO, https://git.reviewboard.kde.org/r/116017/. It prompts the user to approve the redirection as explicitly required in sections 10.3.[2|3] of RFC 2616: If the 301 status code is received in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user, since this might change the conditions under which the request was issued. Please note that this patch only prompts the user for confirmation on POST-POST redirections. It can be expanded to include redirections for other requests such as PUT. There is also an issue of whether this patch should be part of the 4.13 release? Since we are in a freeze and the patch has both message changes as well as a new API, I have simply marked it for inclusion in master branch, i.e. 4.14. Diffs - kio/kio/job.cpp 50b4afb kio/kio/jobuidelegate.h 17fd554 kio/kio/jobuidelegate.cpp 5aff330 Diff: https://git.reviewboard.kde.org/r/116570/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html File Attachments POST redirection confirm dialog https://git.reviewboard.kde.org/media/uploaded/files/2014/03/07/e77dd03e-cb37-49bb-8554-cca991c8c546__post_redirection_confirmation.png Thanks, Dawit Alemayehu
Review Request 119211: Queue CMD_REPARSECONFIGRATION requests for suspended ioslaves
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119211/ --- Review request for kdelibs, David Faure and Thiago Macieira. Repository: kdelibs Description --- This patch addresses the issue where attempting to send CMD_REPARSECONFIGURATION to suspended ioslaves results in a freezing of the application that sent the request. This problem can be clearly reproduced by following the steps outlined in the bug report. I am not entirely sure whether or not this fix really belongs in kio/connection.cpp, but I could not find a more convenient location to queue the command request to ensure the ioslave receives the reparse configuration command if it is ever reused again. Diffs - kio/kio/connection.cpp 99aea0b Diff: https://git.reviewboard.kde.org/r/119211/diff/ Testing --- Run the steps given in the bug report before and after the fix. Thanks, Dawit Alemayehu
Re: Review Request 119211: Queue CMD_REPARSECONFIGRATION requests for suspended ioslaves
On July 10, 2014, 5:08 p.m., David Faure wrote: But why CMD_REPARSECONFIGURATION especially? If the slave is on hold, will anything else get through? IOW should the test just be extended with || suspended, no test on cmd? I started out doing just that at first, but that resulted in a rather long pause whenever I clicked on a link that resulted in a download dialog. The pause seems to be caused by the the job getting suspended and then put on hold where the hold portion waits for the ioslave to be returned to klauncher by invoking KToolInvocation::klauncher()-waitForSlave(d-m_pid). That causes a delay because the command to put the ioslave on hold will be queued up due to the added || suspended check and will only succeed once the call times after some period (I guess 25 secs). When I thought more about this, I realized that this problem is only likely to affect this particular command because the scheduler is the one that controls how ioslaves are used and it does the right thing by not to sending commands to ioslaves on hold without first putting them back into service. Unfortunately CMD_REPARSECONFIGURATION is an exception. Hence, this fix that I am not totally sure belongs here. Perhaps the correct thing to do would be to fix the problem in the scheduler by putting the ioslave on hold back into service, sending it the reparse configuration request, and putting back on hold again? I dunno, but it seemed like an overkill by comparison to this patch. Anyways, this is the reason why I did what I did. I knew it would probably be safe to send the reparse configuration request when the ioslave is resumed, but I was not sure that was the case for all other commands. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119211/#review62085 --- On July 10, 2014, 12:04 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119211/ --- (Updated July 10, 2014, 12:04 p.m.) Review request for kdelibs, David Faure and Thiago Macieira. Repository: kdelibs Description --- This patch addresses the issue where attempting to send CMD_REPARSECONFIGURATION to suspended ioslaves results in a freezing of the application that sent the request. This problem can be clearly reproduced by following the steps outlined in the bug report. I am not entirely sure whether or not this fix really belongs in kio/connection.cpp, but I could not find a more convenient location to queue the command request to ensure the ioslave receives the reparse configuration command if it is ever reused again. Diffs - kio/kio/connection.cpp 99aea0b Diff: https://git.reviewboard.kde.org/r/119211/diff/ Testing --- Run the steps given in the bug report before and after the fix. Thanks, Dawit Alemayehu
Re: Review Request 119021: Forward port fix for 142957 to kinit in frameworks
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119021/ --- (Updated July 4, 2014, 9:17 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Bugs: 142597 https://bugs.kde.org/show_bug.cgi?id=142597 Repository: kinit Description --- This patch forward ports the fix in https://git.reviewboard.kde.org/r/118954/ to kinit in frameworks. I am posting this because I cannot compile and test the fix myself yet. Diffs - src/klauncher/klauncher.cpp 31498e0 Diff: https://git.reviewboard.kde.org/r/119021/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 119021: Forward port fix for 142957 to kinit in frameworks
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119021/ --- (Updated July 1, 2014, 11:59 a.m.) Review request for kdelibs and David Faure. Changes --- Updated bug# Bugs: 142597 https://bugs.kde.org/show_bug.cgi?id=142597 Repository: kinit Description --- This patch forward ports the fix in https://git.reviewboard.kde.org/r/118954/ to kinit in frameworks. I am posting this because I cannot compile and test the fix myself yet. Diffs - src/klauncher/klauncher.cpp 31498e0 Diff: https://git.reviewboard.kde.org/r/119021/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 119020: Forward port fix for 142957 to kio in frameworks
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119020/ --- (Updated July 1, 2014, 12:01 p.m.) Review request for kdelibs and David Faure. Changes --- Amended patch to apply only to local URL. Bugs: 142957 https://bugs.kde.org/show_bug.cgi?id=142957 Repository: kio Description --- This patch forward ports the fix in https://git.reviewboard.kde.org/r/118954/ to kio in frameworks. I am posting this because I cannot compile and test the fix myself yet. Diffs (updated) - src/widgets/krun.cpp 77708a0 Diff: https://git.reviewboard.kde.org/r/119020/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 119021: Forward port fix for 142957 to kinit in frameworks
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119021/ --- (Updated July 1, 2014, 12:02 p.m.) Review request for kdelibs and David Faure. Changes --- Amended patch to only apply to local URL. Bugs: 142597 https://bugs.kde.org/show_bug.cgi?id=142597 Repository: kinit Description --- This patch forward ports the fix in https://git.reviewboard.kde.org/r/118954/ to kinit in frameworks. I am posting this because I cannot compile and test the fix myself yet. Diffs (updated) - src/klauncher/klauncher.cpp 31498e0 Diff: https://git.reviewboard.kde.org/r/119021/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 118954: Set directory to current working directory when executing Open With... dialog
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118954/ --- (Updated June 29, 2014, 4:06 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Bugs: 142597 http://bugs.kde.org/show_bug.cgi?id=142597 Repository: kdelibs Description --- The attached patch sets the current directory to the current working directory (read: directory in which the open with dialog was run) whenever the KService path is empty. That way we tell KProcess in the context of the current directory instead of the default one it uses, $HOME. Diffs - kinit/klauncher.cpp 6c71e99 kio/kio/krun.cpp 590fcf8 Diff: https://git.reviewboard.kde.org/r/118954/diff/ Testing --- Tested with the example provided in the bug report. Verified the output of the compile process is in the current working directory instead of $HOME. Thanks, Dawit Alemayehu
Review Request 119021: Forward port fix for 142957 to kinit in frameworks
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119021/ --- Review request for kdelibs and David Faure. Repository: kinit Description --- This patch forward ports the fix in https://git.reviewboard.kde.org/r/118954/ to kinit in frameworks. I am posting this because I cannot compile and test the fix myself yet. Diffs - src/klauncher/klauncher.cpp 31498e0 Diff: https://git.reviewboard.kde.org/r/119021/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 119020: Forward port fix for 142957 to kio in frameworks
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119020/ --- Review request for kdelibs and David Faure. Bugs: 142957 https://bugs.kde.org/show_bug.cgi?id=142957 Repository: kio Description --- This patch forward ports the fix in https://git.reviewboard.kde.org/r/118954/ to kio in frameworks. I am posting this because I cannot compile and test the fix myself yet. Diffs - src/widgets/krun.cpp 77708a0 Diff: https://git.reviewboard.kde.org/r/119020/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 118954: Set directory to current working directory when executing Open With... dialog
On June 28, 2014, 8:31 a.m., David Faure wrote: kio/kio/krun.cpp, line 733 https://git.reviewboard.kde.org/r/118954/diff/2/?file=285056#file285056line733 now this line is going to be called if urls is empty, even with an empty service.path() . This should be: if !service.path.isEmpty use that else if !urls.isEmpty use first url D'oh. That was dumb. Fixed. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118954/#review61122 --- On June 28, 2014, 2:09 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118954/ --- (Updated June 28, 2014, 2:09 a.m.) Review request for kdelibs and David Faure. Bugs: 142597 http://bugs.kde.org/show_bug.cgi?id=142597 Repository: kdelibs Description --- The attached patch sets the current directory to the current working directory (read: directory in which the open with dialog was run) whenever the KService path is empty. That way we tell KProcess in the context of the current directory instead of the default one it uses, $HOME. Diffs - kinit/klauncher.cpp 6c71e99 kio/kio/krun.cpp 590fcf8 Diff: https://git.reviewboard.kde.org/r/118954/diff/ Testing --- Tested with the example provided in the bug report. Verified the output of the compile process is in the current working directory instead of $HOME. Thanks, Dawit Alemayehu
Re: Review Request 118954: Set directory to current working directory when executing Open With... dialog
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118954/ --- (Updated June 28, 2014, 11:32 a.m.) Review request for kdelibs and David Faure. Changes --- Fixed logic error in previous patch. Bugs: 142597 http://bugs.kde.org/show_bug.cgi?id=142597 Repository: kdelibs Description --- The attached patch sets the current directory to the current working directory (read: directory in which the open with dialog was run) whenever the KService path is empty. That way we tell KProcess in the context of the current directory instead of the default one it uses, $HOME. Diffs (updated) - kinit/klauncher.cpp 6c71e99 kio/kio/krun.cpp 590fcf8 Diff: https://git.reviewboard.kde.org/r/118954/diff/ Testing --- Tested with the example provided in the bug report. Verified the output of the compile process is in the current working directory instead of $HOME. Thanks, Dawit Alemayehu
Re: Review Request 118954: Set directory to current working directory when executing Open With... dialog
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118954/ --- (Updated June 28, 2014, 2:09 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch based on feedback. Bugs: 142597 http://bugs.kde.org/show_bug.cgi?id=142597 Repository: kdelibs Description --- The attached patch sets the current directory to the current working directory (read: directory in which the open with dialog was run) whenever the KService path is empty. That way we tell KProcess in the context of the current directory instead of the default one it uses, $HOME. Diffs (updated) - kinit/klauncher.cpp 6c71e99 kio/kio/krun.cpp 590fcf8 Diff: https://git.reviewboard.kde.org/r/118954/diff/ Testing --- Tested with the example provided in the bug report. Verified the output of the compile process is in the current working directory instead of $HOME. Thanks, Dawit Alemayehu
Review Request 118954: Set directory to current working directory when executing Open With... dialog
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118954/ --- Review request for kdelibs and David Faure. Bugs: 142597 http://bugs.kde.org/show_bug.cgi?id=142597 Repository: kdelibs Description --- The attached patch sets the current directory to the current working directory (read: directory in which the open with dialog was run) whenever the KService path is empty. That way we tell KProcess in the context of the current directory instead of the default one it uses, $HOME. Diffs - kio/kio/krun.cpp 590fcf8 Diff: https://git.reviewboard.kde.org/r/118954/diff/ Testing --- Tested with the example provided in the bug report. Verified the output of the compile process is in the current working directory instead of $HOME. Thanks, Dawit Alemayehu
Re: Review Request 118811: Fix compile with giflib-5.1.0 and upwards.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118811/#review60463 --- Is this patch not going into 4.13 branch? You can no longer compile KDE on rolling release based distros like ArchLinux without this patch. - Dawit Alemayehu On June 18, 2014, 11:40 a.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118811/ --- (Updated June 18, 2014, 11:40 a.m.) Review request for kdelibs, Aleix Pol Gonzalez, Andreas Schwab, David Faure, and Raymond Wooninck. Repository: kdelibs Description --- Fix compile with giflib-5.1.0 and upwards. See news about the giflib-5.1.0 release about the API break here: http://fossies.org/linux/giflib/NEWS Diffs - khtml/imload/decoders/gifloader.cpp 6c61ff502b7891b2973847c198f0aaf55e5c9143 Diff: https://git.reviewboard.kde.org/r/118811/diff/ Testing --- Thanks, Milian Wolff
Re: Review Request 118749: Prevent crashes caused by invalid access of Konqueror's URL edit widget
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118749/ --- (Updated June 16, 2014, 3:48 a.m.) Review request for KDE Base Apps and David Faure. Changes --- Updated patch based on review. Bugs: 320500 http://bugs.kde.org/show_bug.cgi?id=320500 Repository: kde-baseapps Description --- The attached patch is intended to prevent all crashes that are caused by invalid access of the QLineEdit in Konqueror's URL combobox. Diffs (updated) - konqueror/src/konqmainwindow.h 4ad4c4f konqueror/src/konqmainwindow.cpp da8c82e Diff: https://git.reviewboard.kde.org/r/118749/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 118749: Prevent crashes caused by invalid access of Konqueror's URL edit widget
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118749/ --- (Updated June 16, 2014, 3:50 a.m.) Status -- This change has been marked as submitted. Review request for KDE Base Apps and David Faure. Bugs: 320500 http://bugs.kde.org/show_bug.cgi?id=320500 Repository: kde-baseapps Description --- The attached patch is intended to prevent all crashes that are caused by invalid access of the QLineEdit in Konqueror's URL combobox. Diffs - konqueror/src/konqmainwindow.h 4ad4c4f konqueror/src/konqmainwindow.cpp da8c82e Diff: https://git.reviewboard.kde.org/r/118749/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 118749: Prevent crashes caused by invalid access of Konqueror's URL edit widget
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118749/ --- Review request for KDE Base Apps and David Faure. Bugs: 320500 http://bugs.kde.org/show_bug.cgi?id=320500 Repository: kde-baseapps Description --- The attached patch is intended to prevent all crashes that are caused by invalid access of the QLineEdit in Konqueror's URL combobox. Diffs - konqueror/src/konqmainwindow.h 4ad4c4f konqueror/src/konqmainwindow.cpp da8c82e Diff: https://git.reviewboard.kde.org/r/118749/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
On May 19, 2014, 12:05 p.m., Frank Reininghaus wrote:
We are seeing quite a few bug reports about a severe regression between KDE
SC 4.13.0 and KDE SC 4.13.1:
https://bugs.kde.org/show_bug.cgi?id=334776
According to the reporter of https://bugs.kde.org/show_bug.cgi?id=334988
the regression has been caused by this commit.
I'm surprised that this was committed to KDE/4.13 at all - the review
request was for 'master', and it does not really fix a serious bug. Please
make sure that such patches, which fix little annoyances but have the
potential to cause serious trouble, get a lot of testing in betas and RCs
before they are shipped. Testing on a single system is definitely not
enough!
I propose to revert this patch - maybe a better solution which prevents
automounting can be found for master/frameworks, but IMHO we should not
take any further risks in KDE/4.13. Any objections?
Thomas Lübking wrote:
The patch does somehow not what it promises.
KMountPoint::Ptr mp = KMountPoint::currentMountPoints().findByPath( path
);
KMountPoint::currentMountPoints() promises to return mtab, ie. already
used mountpoints, so if there's mp, it's mounted, iow. it looks like (i've
just looked at this for the first time) as if it only shortcuts for already
mounted autofs mounts, but will still statvfs on unmounted autofs mounts
(mounting them implicitly)
Ie. what the patch probably should do was to:
if (!mp) { // unmounted, avoid automounts
KMountPoint::Ptr pmp =
KMountPoint::possibleMountPoints().findByPath(path);
if (pmp pmp-mountType() == QLatin1String(autofs))
return info;
}
I think the submitter took David's suggestion and changed the original
implementation without understanding the consequences. Thomas' suggested patch
would probably be a better starting point.
David: I could not find a isDirectoryMounted() method in kfileitem.cpp to see
what you meant by looking at that implementation.
In the meantime I think this patch should be reverted because it caused a
regression.
- Dawit
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117044/#review58143
---
On May 6, 2014, 8:01 p.m., Tomáš Trnka wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117044/
---
(Updated May 6, 2014, 8:01 p.m.)
Review request for kdelibs.
Repository: kdelibs
Description
---
Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
Previously, all unmounted autofs mountpoints were always mounted
by krunner/plasma-desktop on startup, defeating the purpose of
automounting. Let's ignore the unmounted filesystems instead when
gathering free space stats, just like the df utility does.
Everything still gets mounted properly on first real access.
The change itself is pretty trivial and I would regard it as a bugfix
(and thus eligible for 4.13), but I'm posting it for review to see
what you kdelibs people think.
Diffs
-
kio/kfile/kdiskfreespaceinfo.cpp f11eb0998f0e718e9b366f8c26da30586bfa44ef
Diff: https://git.reviewboard.kde.org/r/117044/diff/
Testing
---
I'm using this patch on kdelibs since 4.11 and I have noted no problems
in connection with ~4 automounted filesystems.
Thanks,
Tomáš Trnka
Re: Review Request 116602: fix setting ssl_was_in_use metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116602/#review54178 --- Do you want me to apply this patch with the minor changes I suggested? - Dawit Alemayehu On March 4, 2014, 9:43 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116602/ --- (Updated March 4, 2014, 9:43 p.m.) Review request for kdelibs and Dawit Alemayehu. Repository: kdelibs Description --- If incomingMetaData doesn't contain item with key ssl_in_use (and i verified this happen) using [] operator results in inserting a new item with key ssl_in_use and NullString value in incomingMetaData map and, as a consequence, a new item with key ssl_was_in_use and NullString value in outgoingMetaData map. This patch aims to avoid this situation by looking up the key using queryMetadata(). Diffs - kio/kio/job.cpp edc5fed Diff: https://git.reviewboard.kde.org/r/116602/diff/ Testing --- Thanks, Andrea Iacovitti
Re: Review Request 116602: fix setting ssl_was_in_use metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116602/#review54260 --- Ship it! In that case then you can ship it. I do not care about the other suggestion I made. - Dawit Alemayehu On March 4, 2014, 9:43 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116602/ --- (Updated March 4, 2014, 9:43 p.m.) Review request for kdelibs and Dawit Alemayehu. Repository: kdelibs Description --- If incomingMetaData doesn't contain item with key ssl_in_use (and i verified this happen) using [] operator results in inserting a new item with key ssl_in_use and NullString value in incomingMetaData map and, as a consequence, a new item with key ssl_was_in_use and NullString value in outgoingMetaData map. This patch aims to avoid this situation by looking up the key using queryMetadata(). Diffs - kio/kio/job.cpp edc5fed Diff: https://git.reviewboard.kde.org/r/116602/diff/ Testing --- Thanks, Andrea Iacovitti
Re: Review Request 116784: Fix incorrect use of KDateTime.toTime_t in kio_http
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116784/ --- (Updated March 20, 2014, 1:10 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs, Andreas Hartmetz and David Faure. Repository: kdelibs Description --- The attached patch does the following: - It corrects a mistake in assumption that KDateTime.toTime_t() will return -1 for invalidate dates. It does not. The result is an overflow which is interpreted in kio_http as a timestamp in the distant future which obviously is wrong. See https://bugs.kde.org/show_bug.cgi?id=331774 for example. This assumption also affects the timestamp variables used for cache management. - It converts cache management timestamp variables to 64 bits so they can accomodates dates beyond Feb 7, 2106. Diffs - kioslave/http/http.h dd85622 kioslave/http/http.cpp e4f1eba Diff: https://git.reviewboard.kde.org/r/116784/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 116783: Change ftpFileExists to use relative paths for SIZE requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116783/ --- Review request for kdelibs and David Faure. Repository: kdelibs Description --- I completely missed ftpFileExists issued a SIZE command to determine the existence of a file when addressing bug# 326292. See https://git.reviewboard.kde.org/r/116524/ review request. The attached patch addresses that oversight to insure renaming files work properly on the android ftp server listed in the bug report. Diffs - kioslave/ftp/ftp.h cbcd096 kioslave/ftp/ftp.cpp b9d90e6 Diff: https://git.reviewboard.kde.org/r/116783/diff/ Testing --- Rerun all the tests run for 116524. Thanks, Dawit Alemayehu
Review Request 116784: Fix incorrect use of KDateTime.toTime_t in kio_http
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116784/ --- Review request for kdelibs, Andreas Hartmetz and David Faure. Repository: kdelibs Description --- The attached patch does the following: - It corrects a mistake in assumption that KDateTime.toTime_t() will return -1 for invalidate dates. It does not. The result is an overflow which is interpreted in kio_http as a timestamp in the distant future which obviously is wrong. See https://bugs.kde.org/show_bug.cgi?id=331774 for example. This assumption also affects the timestamp variables used for cache management. - It converts cache management timestamp variables to 64 bits so they can accomodates dates beyond Feb 7, 2106. Diffs - kioslave/http/http.h dd85622 kioslave/http/http.cpp e4f1eba Diff: https://git.reviewboard.kde.org/r/116784/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 116783: Change ftpFileExists to use relative paths for SIZE requests
On March 13, 2014, 12:40 p.m., David Faure wrote: kioslave/ftp/ftp.cpp, line 2260 https://git.reviewboard.kde.org/r/116783/diff/2/?file=253641#file253641line2260 It's a bit confusing to have two methods called ftpSize. Maybe call this one ftpSizeCmd() or something? (check existing method names for consistency) Ok. I will rename it ftpSendSizeCmd for consistency and adjust the spaces to 2 for this branches and correct it properly for the frameworks branch. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116783/#review52887 --- On March 13, 2014, 12:33 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116783/ --- (Updated March 13, 2014, 12:33 p.m.) Review request for kdelibs and David Faure. Repository: kdelibs Description --- I completely missed ftpFileExists issued a SIZE command to determine the existence of a file when addressing bug# 326292. See https://git.reviewboard.kde.org/r/116524/ review request. The attached patch addresses that oversight to insure renaming files work properly on the android ftp server listed in the bug report. Diffs - kioslave/ftp/ftp.h cbcd096 kioslave/ftp/ftp.cpp b9d90e6 Diff: https://git.reviewboard.kde.org/r/116783/diff/ Testing --- Rerun all the tests run for 116524. Thanks, Dawit Alemayehu
Re: Review Request 116783: Change ftpFileExists to use relative paths for SIZE requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116783/ --- (Updated March 13, 2014, 12:58 p.m.) Review request for kdelibs and David Faure. Changes --- Updated patch. Repository: kdelibs Description --- I completely missed ftpFileExists issued a SIZE command to determine the existence of a file when addressing bug# 326292. See https://git.reviewboard.kde.org/r/116524/ review request. The attached patch addresses that oversight to insure renaming files work properly on the android ftp server listed in the bug report. Diffs (updated) - kioslave/ftp/ftp.h cbcd096 kioslave/ftp/ftp.cpp b9d90e6 Diff: https://git.reviewboard.kde.org/r/116783/diff/ Testing --- Rerun all the tests run for 116524. Thanks, Dawit Alemayehu
Re: Review Request 116783: Change ftpFileExists to use relative paths for SIZE requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116783/ --- (Updated March 13, 2014, 10:15 p.m.) Review request for kdelibs and David Faure. Changes --- updated patch Repository: kdelibs Description --- I completely missed ftpFileExists issued a SIZE command to determine the existence of a file when addressing bug# 326292. See https://git.reviewboard.kde.org/r/116524/ review request. The attached patch addresses that oversight to insure renaming files work properly on the android ftp server listed in the bug report. Diffs (updated) - kioslave/ftp/ftp.h cbcd096 kioslave/ftp/ftp.cpp b9d90e6 Diff: https://git.reviewboard.kde.org/r/116783/diff/ Testing --- Rerun all the tests run for 116524. Thanks, Dawit Alemayehu
Re: Review Request 116783: Change ftpFileExists to use relative paths for SIZE requests
On March 13, 2014, 1:10 p.m., David Faure wrote: kioslave/ftp/ftp.cpp, line 2302 https://git.reviewboard.kde.org/r/116783/diff/3/?file=253646#file253646line2302 Misunderstanding, I wasn't talking about indentation (which indeed ends up being 4 spaces everywhere in frameworks). The issue is more: no space after this if, one space too many after psz on the previous line, too many spaces inside the parenthesis on the line Ftp::ftpSize( ... ), etc. My suggestion was to just do this automatically in frameworks using a tool, otherwise you have to do it correctly by hand :-) I will use the tool you mentioned for frameworks, but for this one I have tried to fix all the glaring ones you mentioned - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116783/#review52891 --- On March 13, 2014, 10:15 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116783/ --- (Updated March 13, 2014, 10:15 p.m.) Review request for kdelibs and David Faure. Repository: kdelibs Description --- I completely missed ftpFileExists issued a SIZE command to determine the existence of a file when addressing bug# 326292. See https://git.reviewboard.kde.org/r/116524/ review request. The attached patch addresses that oversight to insure renaming files work properly on the android ftp server listed in the bug report. Diffs - kioslave/ftp/ftp.h cbcd096 kioslave/ftp/ftp.cpp b9d90e6 Diff: https://git.reviewboard.kde.org/r/116783/diff/ Testing --- Rerun all the tests run for 116524. Thanks, Dawit Alemayehu
Re: Review Request 116784: Fix incorrect use of KDateTime.toTime_t in kio_http
On March 13, 2014, 5:30 p.m., David Jarvie wrote: The handling of return values from KDateTime::toTime_t() in the existing kio_http code is not correct, because the return value's type is implicitly cast to other types before being checked. For example, in one place it is cast to qint64, which will result in a value of 0x instead of 0x (= -1). This type of error will mask the fact that the error value is being returned. Instead of changing the calling code to detect invalid dates using other methods, it should be fixed to properly cast the uint value returned from KDateTime::toTime_t(). For types other than int, it needs to specifically check for uint(-1) and set the cast value to -1 in that case. For example: uint t = KDateTime::toTime_t(...); // Set the qint64 to be -1 if an error occurred: qint64 result = (t == uint(-1)) ? -1 : t; Note: KDateTime::toTime_t() is *supposed* to return uint(-1) to indicate an error. If it doesn't always do this, *it* should be fixed instead of changing code elsewhere, since kio_http is unlikely to be the only module that will have trouble if that is happening. Perhaps it was not clear from the description, but I am not implying nor have I implied there to be a bug in KDateTime. As I have clearly stated the problem is with the assumption the code in kio_http makes about what KDateTime::toTime_t returns for an invalid date. No matter how you see it the toTime_t() function can not and does not return a literal -1, which is exactly what the code in kio_http assumes! Of course that is clearly wrong. Anyhow, this patch is specifically intended to fix that issue and nothing else. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116784/#review52897 --- On March 13, 2014, 12:49 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116784/ --- (Updated March 13, 2014, 12:49 p.m.) Review request for kdelibs, Andreas Hartmetz and David Faure. Repository: kdelibs Description --- The attached patch does the following: - It corrects a mistake in assumption that KDateTime.toTime_t() will return -1 for invalidate dates. It does not. The result is an overflow which is interpreted in kio_http as a timestamp in the distant future which obviously is wrong. See https://bugs.kde.org/show_bug.cgi?id=331774 for example. This assumption also affects the timestamp variables used for cache management. - It converts cache management timestamp variables to 64 bits so they can accomodates dates beyond Feb 7, 2106. Diffs - kioslave/http/http.h dd85622 kioslave/http/http.cpp e4f1eba Diff: https://git.reviewboard.kde.org/r/116784/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 116783: Change ftpFileExists to use relative paths for SIZE requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116783/ --- (Updated March 13, 2014, 11:30 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Repository: kdelibs Description --- I completely missed ftpFileExists issued a SIZE command to determine the existence of a file when addressing bug# 326292. See https://git.reviewboard.kde.org/r/116524/ review request. The attached patch addresses that oversight to insure renaming files work properly on the android ftp server listed in the bug report. Diffs - kioslave/ftp/ftp.h cbcd096 kioslave/ftp/ftp.cpp b9d90e6 Diff: https://git.reviewboard.kde.org/r/116783/diff/ Testing --- Rerun all the tests run for 116524. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
On March 4, 2014, 7:42 p.m., David Faure wrote: I don't get it. What's the problem with sending a mimetype for empty files? I would think this is actually expected - for all files, including empty ones. Why does this fix the bug? Dawit Alemayehu wrote: It fails and ends up sending an error message. See the if statement in while(true) loop in ftpSendMimeType. Even if that check does not fail and the while(true) loop completes successfully, the next statement if (!buffer.isEmpty()) will be false for empty files. Hence it is completely useless to call ftpSendMimeType from ftpGet for empty files. Dawit Alemayehu wrote: dfaure: any further questions on this? David Faure wrote: I still think this is wrong. By contract the kioslave must emit a mimetype. Instead, ftpSendMimeType should be fixed, to detect the case where totalSize is 0, and skip the loop in that case since we have nothing to read. Ahh... I forgot that the mimeType signal MUST be emitted. But even if we avoid the while loop the if statement also prevents the emission of that signal. What mime-type should be emitted in case of zero sized files would the better question I guess. KMimeType::defaultMimeType()? - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review51922 --- On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 2, 2014, 2:20 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 11:35 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch based on feedback. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs (updated) - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 12, 2014, 11:36 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch based on feedback. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs (updated) - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 11:39 a.m.) Review request for kdelibs and David Faure. Changes --- Reverted incorrect patch update. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs (updated) - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 12:47 p.m.) Review request for kdelibs and David Faure. Changes --- Updated patch to send the proper mimetype for completely empty files (0 bytes). Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs (updated) - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 1:26 p.m.) Review request for kdelibs and David Faure. Changes --- updated patch. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs (updated) - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
On March 12, 2014, 2:39 p.m., David Faure wrote: Thanks. I hope we don't get into the case of Unknown Size when the size is actually 0, though. That case would still be buggy, and I'm not sure how we could fix it. Well I could think of one scenario where that could potentially happen. The FTP server does not respond correctly to the SIZE command and the size of the file we are trying to retrieve is actually 0, but I am sure that is a rare server like the one reported in the very old bug report #168011. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review52761 --- On March 12, 2014, 1:26 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 1:26 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 13, 2014, 5:05 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 13, 2014, 5:05 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
On March 5, 2014, 7:47 a.m., David Faure wrote: kioslave/ftp/ftp.cpp, line 2275 https://git.reviewboard.kde.org/r/116524/diff/2/?file=251597#file251597line2275 This is surely wrong. If you're in /home/dfaure (as the CWD would be, by default, on non-anonymous FTP) and you're downloading a file /home/dfaure/dir1/dir2/file.txt then surely you want to call SIZE dir1/dir2/file.txt and not SIZE home/dfaure/dir1/dir2/file.txt It has to be relative to the CWD, not just skip the first slash, which only works if the CWD is /. This gives two alternatives for the fix: make this code relative to the current CWD whatever it is, or call ftpFolder() with the directory name (e.g. /home/dfaure/dir1/dir2) followed by SIZE with just the filename. The latter sounds like it might work better on android (if it doesn't support absolute paths, maybe it doesn't support ../../foo/bar.txt either?). Dawit Alemayehu wrote: I have not been able to test whether it supported ../../foo/bar.txt yet. However, making the code relative to m_currentPath seems to work just fine ; so I can avoid having to call ftpFolder and hence sending a cwd request. I am done testing this patch and it seems to work with a bunch of different ftp servers I tried so far. I tested with whatever ftp.kde.org runs, the android ftp server that was the source of this fix and both pro-ftpd and vsftpd servers on my own machines. If there are no objections, I am going to commit this after the beta2 is out ; so it can go in for the next beta release. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review52013 --- On March 7, 2014, 6:48 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 7, 2014, 6:48 a.m.) Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
On March 4, 2014, 7:42 p.m., David Faure wrote: I don't get it. What's the problem with sending a mimetype for empty files? I would think this is actually expected - for all files, including empty ones. Why does this fix the bug? Dawit Alemayehu wrote: It fails and ends up sending an error message. See the if statement in while(true) loop in ftpSendMimeType. Even if that check does not fail and the while(true) loop completes successfully, the next statement if (!buffer.isEmpty()) will be false for empty files. Hence it is completely useless to call ftpSendMimeType from ftpGet for empty files. dfaure: any further questions on this? - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review51922 --- On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 2, 2014, 2:20 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
On March 8, 2014, 12:40 p.m., Andrea Iacovitti wrote: I'm a bit lost with your latest patch uploaded... Speaking about the third patch in this review (Diff r3) it seems it works as expected. That is if a server redirects a collection DELETE operation to a new Location with trailing slash added, redirection is correctly followed. From my part it is a ship for this one (Diff r3). Sorry I posted the wrong patch!! Will post the correct one. It is the same as Diff r3 except it adds a trailing slash to the directory URL before sending it to the http ioslave. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/#review52381 --- On March 7, 2014, 6:05 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 7, 2014, 6:05 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs - kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 8, 2014, 5:08 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Uploaded the correct and final patch. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs (updated) - kio/kio/deletejob.cpp b0268ef kioslave/http/http.cpp b4d64d4 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 8, 2014, 9:42 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs - kio/kio/deletejob.cpp b0268ef kioslave/http/http.cpp b4d64d4 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
On March 6, 2014, 9:10 p.m., Andrea Iacovitti wrote: I tested your patch, no trailing slash is added whether the request refers to a collection or resource (as it was before commit 58294ac). Dawit Alemayehu wrote: Right, but now the webdav server should redirect to the right address (with trailing slash) and that redirect should succeed. I guess we can always add a trailing slash if there are servers that do not redirect or we want to avoid the redirect. Andrea Iacovitti wrote: Well, not mine, i get a 204 and the forlder successfully deleted. Well that is fine. If you try one of the test servers I listed above, you will see that deleting directory without a trailing slash results in a 302 response that redirects you to the same directory but with the trailing slash appended. The problem before this patch was that redirection would have simply been ignored. That is because the code in DeleteJobPrivate::deleteNextSrc blindly calls KIO::rmdir even for http/webdav resources. And Since KIO::rmdir uses DeleteJob which inherits from SimpleJob, there is no support for redirection. You need TransferJob or its descendents if the protocol requires redirection support. Once we convert to properly call KIO::http_delete for HTTP based protocols, then it works as expected because we will be able to handle the redirection of the delete request. Anyhow, all I was suggesting in my prior response above for us to optimize the directory deletion requests for the servers that require trailing slash. That way we avoid the additional redirection when requesting deletion from such servers without impacting those servers that do not care about the trailing slash. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/#review52294 --- On March 5, 2014, 2:01 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 5, 2014, 2:01 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs - kio/kio/deletejob.cpp b0268ef kioslave/http/http.cpp b4d64d4 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Review Request 116570: Ask user for confirmation before doing POST - POST redirection in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116570/ --- Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- This patch is a companion to the recent POST-POST redirection implementation in KIO, https://git.reviewboard.kde.org/r/116017/. It prompts the user to approve the redirection as explicitly required in sections 10.3.[2|3] of RFC 2616: If the 301 status code is received in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user, since this might change the conditions under which the request was issued. Please note that this patch only prompts the user for confirmation on POST-POST redirections. It can be expanded to include redirections for other requests such as PUT. There is also an issue of whether this patch should be part of the 4.13 release? Since we are in a freeze and the patch has both message changes as well as a new API, I have simply marked it for inclusion in master branch, i.e. 4.14. Diffs - kio/kio/job.cpp 50b4afb kio/kio/jobuidelegate.h 17fd554 kio/kio/jobuidelegate.cpp 5aff330 Diff: https://git.reviewboard.kde.org/r/116570/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html File Attachments POST redirection confirm dialog https://git.reviewboard.kde.org/media/uploaded/files/2014/03/07/e77dd03e-cb37-49bb-8554-cca991c8c546__post_redirection_confirmation.png Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
On March 5, 2014, 7:47 a.m., David Faure wrote: kioslave/ftp/ftp.cpp, line 2275 https://git.reviewboard.kde.org/r/116524/diff/2/?file=251597#file251597line2275 This is surely wrong. If you're in /home/dfaure (as the CWD would be, by default, on non-anonymous FTP) and you're downloading a file /home/dfaure/dir1/dir2/file.txt then surely you want to call SIZE dir1/dir2/file.txt and not SIZE home/dfaure/dir1/dir2/file.txt It has to be relative to the CWD, not just skip the first slash, which only works if the CWD is /. This gives two alternatives for the fix: make this code relative to the current CWD whatever it is, or call ftpFolder() with the directory name (e.g. /home/dfaure/dir1/dir2) followed by SIZE with just the filename. The latter sounds like it might work better on android (if it doesn't support absolute paths, maybe it doesn't support ../../foo/bar.txt either?). I have not been able to test whether it supported ../../foo/bar.txt yet. However, making the code relative to m_currentPath seems to work just fine ; so I can avoid having to call ftpFolder and hence sending a cwd request. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review52013 --- On March 3, 2014, 12:16 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 3, 2014, 12:16 a.m.) Review request for kdelibs and David Faure. Bugs: 168011 and 326292 http://bugs.kde.org/show_bug.cgi?id=168011 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
On March 4, 2014, 7:42 p.m., David Faure wrote: I don't get it. What's the problem with sending a mimetype for empty files? I would think this is actually expected - for all files, including empty ones. Why does this fix the bug? It fails and ends up sending an error message. See the if statement in while(true) loop in ftpSendMimeType. Even if that check does not fail and the while(true) loop completes successfully, the next statement if (!buffer.isEmpty()) will be false for empty files. Hence it is completely useless to call ftpSendMimeType from ftpGet for empty files. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review51922 --- On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 2, 2014, 2:20 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 5, 2014, 2:01 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Updated patch. The problem was actually in DeleteJob where http_delete and not rmdir should have been used to delete http/webdav folder. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs (updated) - kio/kio/deletejob.cpp b0268ef kioslave/http/http.cpp b4d64d4 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
On March 4, 2014, 2:51 p.m., Andrea Iacovitti wrote: Still broken for webdav protocol (tested using konq/dolphin), can't delete file. isFile is always false whether you try to delete a file or folder so a trailing slash is always added. Well it works for me. I could not delete a webdav folder before this patch and I can afterwards. Are you sure you killed all existing kio_http processes before you run tests with the patched version? - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/#review51879 --- On March 3, 2014, 3:50 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 3, 2014, 3:50 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs - kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
On March 4, 2014, 2:51 p.m., Andrea Iacovitti wrote: Still broken for webdav protocol (tested using konq/dolphin), can't delete file. isFile is always false whether you try to delete a file or folder so a trailing slash is always added. Dawit Alemayehu wrote: Well it works for me. I could not delete a webdav folder before this patch and I can afterwards. Are you sure you killed all existing kio_http processes before you run tests with the patched version? Andrea Iacovitti wrote: I'm pretty sure.. May it be that the behaviour depends on the type of webdav server being used? I tested against apache 2.2 with mod_dav enabled and a very basic configuration just to be able to create and delete files and folders. With or without this patch, using webdav protocol, a trailing slash is always added to the url (isFile == false) and i can successfully delete folders while i can't delete files. Dawit Alemayehu wrote: There are webdav servers listed in the second link. Can you test with those? You have to change the protocol from https to webdavs. Three of the test targets listed are apache 2.4.x with mod_dav. I can indeed reproduce this problem. I guess I did not try deleting files. Let me see if we are able to set the isFile flag properly for webdav resources. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/#review51879 --- On March 3, 2014, 3:50 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 3, 2014, 3:50 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs - kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 3, 2014, 3:50 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Updated patch after testing. Makes no sense to do redirection on delete. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs (updated) - kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing (updated) --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- Review request for kdelibs and David Faure. Summary (updated) - Do not call ftpSendMimeType for empty files in Ftp::ftpGet Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description (updated) --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs (updated) - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing (updated) --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
On March 2, 2014, 12:14 a.m., Dawit Alemayehu wrote: I am curious if stripping the leading / and sending a relative path with the SIZE command would work for every FTP server? David Faure wrote: It's worth a try. I took at look at the very old svn history, and it has just always been an absolute path, we never tried relative. (so it's not like this would be reverting some bugfix) I think it was just a way to avoid sending one more command (cwd). But it seems logical to do that (cwd, then size, then get). Yeah well I changed my mind once I saw the amount of change I would have to make. I have to change how both ftpGet and ftpPut work since everything in kio_ftp uses absolute paths. Instead of changing something that already works quite well, I rather stick to this simple patch that retries the SIZE command for those few servers that do not support using it with absolute paths. I have also updated the patch not to return an error by checking if m_iRespType != 2. Instead it should return UnknownSize. That way when a ftp server has no support or has a disabled SIZE command, kio_ftp will continue to work. This would solve a very old bug in kio_ftp #168011. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review51562 --- On March 2, 2014, 12:12 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 2, 2014, 12:12 a.m.) Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 3, 2014, 12:16 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch. Now fix also addresses bug# 168011. Bugs: 168011 and 326292 http://bugs.kde.org/show_bug.cgi?id=168011 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs (updated) - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review51562 --- I am curious if stripping the leading / and sending a relative path with the SIZE command would work for every FTP server? - Dawit Alemayehu On March 2, 2014, 12:12 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 2, 2014, 12:12 a.m.) Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 27, 2014, 1:41 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Added associated bug report number to the ticket. Bugs: 330890 http://bugs.kde.org/show_bug.cgi?id=330890 Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kioslave/http/http.cpp 9eba5d1 kio/kio/job_p.h 5ead3ed kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing (updated) --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html http://www.w3.org/People/Bos/Test/redirect307.html Thanks, Dawit Alemayehu
Review Request 116122: Do not add trailing slash on http DELETE requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs - Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 28, 2014, 5:42 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Updated patch. Bugs: 330890 http://bugs.kde.org/show_bug.cgi?id=330890 Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs (updated) - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html http://www.w3.org/People/Bos/Test/redirect307.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 28, 2014, 5:44 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Updated the branch this patch applies to. Bugs: 330890 http://bugs.kde.org/show_bug.cgi?id=330890 Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html http://www.w3.org/People/Bos/Test/redirect307.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 26, 2014, 8:34 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Updated version of the patch that cleans up the HTTP protocol changes and tested using both webkit and khtml engines against the greenbytes test server. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs (updated) - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
On Feb. 26, 2014, 4:15 p.m., Andrea Iacovitti wrote: ... may be it's more clear what I mean in my comments by showing code, see http://paste.kde.org/pvzj0ppio I did many tests and didn't found issues so far No I completely understood your point. It was just that my implementation was from the perspective of I am going to preserve the current functionality and only allow the new POST - POST redirection when explicitly requested. However, there was no need for that since the behavior in the http ioslave was also changed at the same time. As such I will change KIO's default POST redirection behavior to do POST by default and that override that through the use of the new redirect-to-get meta-data. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/#review50957 --- On Feb. 26, 2014, 8:34 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 26, 2014, 8:34 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 27, 2014, 6:04 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Final change based on feedback. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs (updated) - kioslave/http/http.cpp 9eba5d1 kio/kio/job_p.h 5ead3ed kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 116048: Remove Content-Type header when redirecting to GET
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116048/#review50896 --- Ship it! Ship It! - Dawit Alemayehu On Feb. 25, 2014, 4:21 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116048/ --- (Updated Feb. 25, 2014, 4:21 p.m.) Review request for kdelibs and Dawit Alemayehu. Repository: kdelibs Description --- This fix is for stable branch KDE/4.12 and it's needed because of latest changes in redirection code handling. Noticed while reviewing https://git.reviewboard.kde.org/r/116017/ (Dawit, you may want to rebase it if this goes in). Diffs - kio/kio/job.cpp 9cf2b57 Diff: https://git.reviewboard.kde.org/r/116048/diff/ Testing --- various Thanks, Andrea Iacovitti
Review Request 116073: Do not use encoded URL when creating relative symlinks
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116073/ --- Review request for kdelibs and David Faure. Bugs: 330463 http://bugs.kde.org/show_bug.cgi?id=330463 Repository: kdelibs Description --- When creating symlinks in KNewFileMenuPrivate::_k_slotSymLink, call prettyUrl() instead url() to retrieve the user entered text. Otherwise, a percent encoded version of the URL will be used to create the symlink which of course results in the creation of an invalid symlink. Note that this call needs to probably be changed toString() in kf5 since it is using QUrl. Diffs - kfile/knewfilemenu.cpp e7fe237 Diff: https://git.reviewboard.kde.org/r/116073/diff/ Testing --- Follow the steps outlined in the bug report to create a symlink to a file whose path or name contains characters that are not allowed in a URL. Thanks, Dawit Alemayehu
Re: Review Request 116073: Do not use encoded URL when creating relative symlinks
On Feb. 26, 2014, 6:47 a.m., David Faure wrote: Hmm, KUrl isn't very good with relative urls indeed. This fix is incomplete: a symlink to a path with a % in it would still lead to %25. The Qt5 fix is obvious: toString(QUrl::FullyDecoded). But in kdelibs4/qt4, I can't find a good solution. path() truncates at a '#', so no go either. Ah, I found it... QUrl(kurl).toString() :-) The method I always declared completely wrong (for not encoding '#' in paths, breaking round-tripping) finally has its usefulness... (for the case of relative urls, rather rare in KDE code). I just committed a unittest extension to kurltest.cpp in KDE/4.12 which proves all this :) Great! Always fan of unit testing to prove stuff out. I will change the patch to use QUrl(kurl).toString() then. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116073/#review50899 --- On Feb. 26, 2014, 6:31 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116073/ --- (Updated Feb. 26, 2014, 6:31 a.m.) Review request for kdelibs and David Faure. Bugs: 330463 http://bugs.kde.org/show_bug.cgi?id=330463 Repository: kdelibs Description --- When creating symlinks in KNewFileMenuPrivate::_k_slotSymLink, call prettyUrl() instead url() to retrieve the user entered text. Otherwise, a percent encoded version of the URL will be used to create the symlink which of course results in the creation of an invalid symlink. Note that this call needs to probably be changed toString() in kf5 since it is using QUrl. Diffs - kfile/knewfilemenu.cpp e7fe237 Diff: https://git.reviewboard.kde.org/r/116073/diff/ Testing --- Follow the steps outlined in the bug report to create a symlink to a file whose path or name contains characters that are not allowed in a URL. Thanks, Dawit Alemayehu
Re: Review Request 116073: Do not use encoded URL when creating relative symlinks
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116073/ --- (Updated Feb. 26, 2014, 7:19 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch. Bugs: 330463 http://bugs.kde.org/show_bug.cgi?id=330463 Repository: kdelibs Description --- When creating symlinks in KNewFileMenuPrivate::_k_slotSymLink, call prettyUrl() instead url() to retrieve the user entered text. Otherwise, a percent encoded version of the URL will be used to create the symlink which of course results in the creation of an invalid symlink. Note that this call needs to probably be changed toString() in kf5 since it is using QUrl. Diffs (updated) - kfile/knewfilemenu.cpp e7fe237 Diff: https://git.reviewboard.kde.org/r/116073/diff/ Testing --- Follow the steps outlined in the bug report to create a symlink to a file whose path or name contains characters that are not allowed in a URL. Thanks, Dawit Alemayehu
Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp 9cf2b57 kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
On Feb. 24, 2014, 8:47 p.m., Andrea Iacovitti wrote: I quickly tested the patch using a basic html form and it seems it break http post operation (regardless of redirection). I get back a 400 Bad Request from the server and can see the following debug message: kio_http(17852)/kio (kioslave) KIO::SlaveBasePrivate::verifyState: special() did not call finished() or error()! Please fix the KIO slave. As for these page http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html they can't be used for testing as they do make XHR requests with no body data. Fixed. It would actually help if the QBuffer was opened before attempting to read from it. :( - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/#review50761 --- On Feb. 24, 2014, 2:07 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 24, 2014, 2:07 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp 9cf2b57 kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 115864: kio_http: fixes for 30X response code redirection handling
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115864/#review50296 --- Ship it! - Dawit Alemayehu On Feb. 19, 2014, 9:50 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115864/ --- (Updated Feb. 19, 2014, 9:50 p.m.) Review request for kdelibs and Dawit Alemayehu. Repository: kdelibs Description --- Check if method has been overridden via CustomHTTPMethod metaData when redirecting POST to GET in case of 301/302 response code. Handle 308 Permanent Redirect response code. Diffs - kioslave/http/http.h 076ad2b kioslave/http/http.cpp 68b5247 kioslave/http/httpauthentication.cpp 80d7995 Diff: https://git.reviewboard.kde.org/r/115864/diff/ Testing --- Thanks, Andrea Iacovitti
Re: Review Request 115864: kio_http: fixes for 30X response code redirection handling
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115864/#review50188 --- kioslave/http/http.cpp https://git.reviewboard.kde.org/r/115864/#comment35291 I think 301 can simply do the setMetaData call and pass on through 302 instead of duplicating the same exact code. - Dawit Alemayehu On Feb. 18, 2014, 9:36 a.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115864/ --- (Updated Feb. 18, 2014, 9:36 a.m.) Review request for kdelibs and Dawit Alemayehu. Repository: kdelibs Description --- Check if method has been overridden via CustomHTTPMethod metaData when redirecting POST to GET in case of 301/302 response code. Handle 308 Permanent Redirect response code. Diffs - kioslave/http/http.cpp 68b5247 Diff: https://git.reviewboard.kde.org/r/115864/diff/ Testing --- Thanks, Andrea Iacovitti
Re: Review Request 115864: kio_http: fixes for 30X response code redirection handling
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115864/#review50189 --- Did you abandon the original change you proposed on purpose? - Dawit Alemayehu On Feb. 18, 2014, 9:36 a.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115864/ --- (Updated Feb. 18, 2014, 9:36 a.m.) Review request for kdelibs and Dawit Alemayehu. Repository: kdelibs Description --- Check if method has been overridden via CustomHTTPMethod metaData when redirecting POST to GET in case of 301/302 response code. Handle 308 Permanent Redirect response code. Diffs - kioslave/http/http.cpp 68b5247 Diff: https://git.reviewboard.kde.org/r/115864/diff/ Testing --- Thanks, Andrea Iacovitti
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 17, 2014, 5:29 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and Andrea Iacovitti. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
On Feb. 16, 2014, 12:38 a.m., Andrea Iacovitti wrote: khtml/ecma/xmlhttprequest.cpp, line 512 https://git.reviewboard.kde.org/r/115689/diff/2/?file=244040#file244040line512 This does not seems to fix the POST-POST redirection with no content on 307 response issue, instead it has the side effect that POST method is rewritten to GET in 307 redirection.Also it doesn't make possible to do an XHR PUT request with content (!_body.isEmpty()) as actually a POST request is sent to the origin server and not a PUT. We use KIO::http_post (regardless of method) whenever we need to send a request that includes content, even for methods that do not have a defined semantics for entity-body (e.g. DELETE). So setting custom method it's always needed. The reason a POST method is rewritten to GET in a 307 redirection has to do with how we originally chose to deal with POST redirection in KIO::TransferJob. Right now all POST redirections are converted to GET regardless of the HTTP status code in TransferJob::slotFinished in job.cpp. See the CMD_SPECIAL section of the switch statement in that function. As far as then issue with the PUT request and an entity-body, kio_http was designed only to send the entity-body to the server for POST and other webdav requests. Anything that is done to skirt around that to me is a hack by definition. What would be the purpose of allowing entity-body for methods that are not supposed to have such thing, e.g. DELETE? Anyhow, I do not want to cause regression ; so I will roll back this patch to the way things were. However, those POST-POST redirection tests it seems to pass are wrong because it never sends the entity-body to the redirected resource. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/#review49889 --- On Feb. 14, 2014, 4 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 14, 2014, 4 p.m.) Review request for kdelibs and Andrea Iacovitti. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 16, 2014, 7:10 p.m.) Review request for kdelibs and Andrea Iacovitti. Changes --- Always send the CustomHTTPmethod meta data. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs (updated) - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 14, 2014, 4 p.m.) Review request for kdelibs and Andrea Iacovitti. Changes --- Updated the patch because I discovered more issues with the way khtml does its xmlhttp requests. Andrea please review and see if you have any objections. As stated in my comments, setting a custom HTTP header blindly has unintended consequences. It pretends to correctly redirect a POST request to another POST request for 307/308 HTTP response codes when it really does not. IOW, it simply changes the method from GET to POST, because of the presence of the custom HTTP method, but does NOT send any content to the redirected resource like a real POST-POST redirection should. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs (updated) - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 12, 2014, 9:56 a.m.) Review request for kdelibs and Andrea Iacovitti. Changes --- Uploaded the patch. Please note that the relevant patch to this pull request is the one that changes khtml/ecma/xmlhttprequest.cpp and adds CMD_STAT to TransferJob::slotFinished's switch statement. Unfortunately, this patch depends on another one that is currently in review, https://git.reviewboard.kde.org/r/115651/, hence the posted patch here contains the changes from that review request as well. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs (updated) - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/DESIGN.metadata 1351119 kio/kio/accessmanager.cpp 7a806e8 kio/kio/job.cpp 13107c2 kioslave/http/http.cpp b13eed1 Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Review Request 115651: Fix HTTP redirection handling (3XX status code)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115651/ --- Review request for kdelibs, Andreas Hartmetz and David Faure. Bugs: 330795 http://bugs.kde.org/show_bug.cgi?id=330795 Repository: kdelibs Description --- The attached patch fixes how we handle HTTP redirection. Currently KIO does not correctly handle a 303 See Other response. Instead of converting the redirection request to a GET operation as specified in the RFC, KIO simply repeats the same operation with the redirect URL. Additionally, KIO does not handle redirection of a delete operation that is handled internally. Diffs - kio/DESIGN.metadata 1351119 kio/kio/accessmanager.cpp 7a806e8 kio/kio/job.cpp 13107c2 kioslave/http/http.cpp b13eed1 Diff: https://git.reviewboard.kde.org/r/115651/diff/ Testing --- Run tests at http://greenbytes.de/tech/tc/httpredirects/t301methods.html http://greenbytes.de/tech/tc/httpredirects/t302methods.html http://greenbytes.de/tech/tc/httpredirects/t303methods.html Thanks, Dawit Alemayehu
Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- Review request for kdelibs and Andrea Iacovitti. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs - Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115250: Try PASV mode when using Socks proxy
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115250/#review49496 --- Ship it! Ship It! - Dawit Alemayehu On Jan. 23, 2014, 11:31 a.m., Emil Sedgh wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115250/ --- (Updated Jan. 23, 2014, 11:31 a.m.) Review request for kdelibs, Dawit Alemayehu and David Faure. Repository: kdelibs Description --- FTP has two modes: PASV and EPSV. Not all server's support EPSV. Currently, kio_ftp gives up on PASV mode if socks proxy is enabled. That is because QHostAddress.protocol() returns -1 (unknown protocol) on KUrl(socks://localhost:3128). So kio_ftp fails using SOCKS proxy on server's that lack EPSV support. This patch makes sure kio_ftp will try PASV mode if socks proxy is enabled. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/115250/diff/ Testing --- Tested a server that lacks EPSV support with and without proxy. Seems fine now. Used to throw 'Internal Server error'. Thanks, Emil Sedgh
Re: Review Request 114924: Konqueror: fix crash wnen switching between view modes
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114924/#review47101 --- I guess we tried to fix the same problem. I think my fix is much simpler. See https://git.reviewboard.kde.org/r/114926/ - Dawit Alemayehu On Jan. 9, 2014, 12:09 p.m., Jonathan Marten wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114924/ --- (Updated Jan. 9, 2014, 12:09 p.m.) Review request for KDE Base Apps and Dawit Alemayehu. Bugs: 322686 http://bugs.kde.org/show_bug.cgi?id=322686 Repository: kde-baseapps Description --- The referenced bug describes an assert crash which happens when switching view modes in Konqueror, using the View - View Mode menu. It does not happen when switching view modes via the Dolphin Part toolbar. Although the problem does not happen for adawit, this changes fixes the crash for me. Diffs - konqueror/src/konqmainwindow.cpp 8a21c1b Diff: https://git.reviewboard.kde.org/r/114924/diff/ Testing --- Built kde-baseapps with these changes. Checked no crash when switching view mode both by toolbar and menu, and that the correct items in each case are checked. Thanks, Jonathan Marten
