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
Review Request 116077: KRunner patch so it detects Iceweasel bookmarks
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116077/ --- Review request for kde-workspace. Bugs: 322399 http://bugs.kde.org/show_bug.cgi?id=322399 Repository: kde-workspace Description --- Hi, This patch was submitted to the bug tracker some time ago, and even longer in the debian bug tracker, here is the original report: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=646612 Thanks, Diffs - plasma/generic/runners/bookmarks/browserfactory.cpp 7f29f5efd8817983db67c496b850d1caba4d8046 Diff: https://git.reviewboard.kde.org/r/116077/diff/ Testing --- Thanks, Maximiliano Curia
Re: Review Request 116077: KRunner patch so it detects Iceweasel bookmarks
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116077/#review50930 --- Ship it! Patch looks sensible. Do you have a Git account at KDE? - Sebastian Kügler On Feb. 26, 2014, 10:48 a.m., Maximiliano Curia wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116077/ --- (Updated Feb. 26, 2014, 10:48 a.m.) Review request for kde-workspace. Bugs: 322399 http://bugs.kde.org/show_bug.cgi?id=322399 Repository: kde-workspace Description --- Hi, This patch was submitted to the bug tracker some time ago, and even longer in the debian bug tracker, here is the original report: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=646612 Thanks, Diffs - plasma/generic/runners/bookmarks/browserfactory.cpp 7f29f5efd8817983db67c496b850d1caba4d8046 Diff: https://git.reviewboard.kde.org/r/116077/diff/ Testing --- Thanks, Maximiliano Curia
Re: Review Request 116077: KRunner patch so it detects Iceweasel bookmarks
On Feb. 26, 2014, 1:15 p.m., Sebastian Kügler wrote: Patch looks sensible. Do you have a Git account at KDE? Hi, No, I don't have a git account, but you are the third person that ask me that, maybe I should request one. Either way, could you please commit the change? Thanks for the review. - Maximiliano --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116077/#review50930 --- On Feb. 26, 2014, 10:48 a.m., Maximiliano Curia wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116077/ --- (Updated Feb. 26, 2014, 10:48 a.m.) Review request for kde-workspace. Bugs: 322399 http://bugs.kde.org/show_bug.cgi?id=322399 Repository: kde-workspace Description --- Hi, This patch was submitted to the bug tracker some time ago, and even longer in the debian bug tracker, here is the original report: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=646612 Thanks, Diffs - plasma/generic/runners/bookmarks/browserfactory.cpp 7f29f5efd8817983db67c496b850d1caba4d8046 Diff: https://git.reviewboard.kde.org/r/116077/diff/ Testing --- Thanks, Maximiliano Curia
Re: Review Request 115695: Rework KNotification to work without KNotify daemon
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115695/ --- (Updated Feb. 26, 2014, 4:53 p.m.) Status -- This change has been discarded. Review request for kde-workspace, KDE Frameworks, Plasma, and Sune Vuorela. Repository: knotifications Description --- This patch merges KNotify daemon into KNotificationManager to have less daemons running and less dbus traffic. The patch is not yet finished (and for now it's full of QDebugs, that will all be removed and FIXMEs to indicate what needs doing), but as the Alpha2 is quite soon, I'd like to start the general review asap so some more changes can be done if needed. Now it's KNotificationManager that handles the KNotifyPlugin-s and hands them the notification directly. KNotifyConfig was repurposed a bit, now it serves mostly just as the .notifyrc wrapper, all the rest is reused from the KNotification object. There are some changes in the KNotification API - id() and appName() are now exposed to public and slotReceivedId(int) is now also public so that KNotificationManager can directly give it an id. I'd like to rename this and make it a non-slot. It's not the DBus/Galago server ID anymore, that is handled in NotifyByPopup which is responsible for communicating with the galago server (all the methods there were renamed to actually have *galago* in the name so it's clear), therefore the mapping of DBus/Galago Server ids is managed only there as it is actually only needed here. KNotitification::id() is assigned by the KNotificationManager and it's a simple increasing counter. The UI/Plasmoid changes will come next - basically the plan is to put only the Persistent notifications in the notifications history. Diffs - src/knotifyconfig.h PRE-CREATION src/knotifyconfig.cpp PRE-CREATION src/knotifyplugin.h PRE-CREATION src/knotifyplugin.cpp PRE-CREATION src/notifybypopup.h PRE-CREATION src/notifybypopup.cpp PRE-CREATION src/notifybypopupgrowl.h PRE-CREATION src/notifybypopupgrowl.cpp PRE-CREATION CMakeLists.txt 63ebf71 src/CMakeLists.txt a81b913 src/knotification.h 00554ba src/knotification.cpp 5d7405b src/knotificationmanager.cpp a4d0dfa src/knotificationmanager_p.h 81d962d Diff: https://git.reviewboard.kde.org/r/115695/diff/ Testing --- Works perfectly with both plasma notifications and kpassivepopup. Thanks, Martin Klapetek
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/#review50948 --- kio/kio/job.cpp https://git.reviewboard.kde.org/r/116017/#comment35767 This seems not needed. We can handle GET redirection just by setting redirect-to-get to true; in this case we already set d-m_command = CMD_GET before the switch statement, then case CMD_SPECIAL: will not be executed. (See also related comments on http.cpp part) kioslave/http/http.cpp https://git.reviewboard.kde.org/r/116017/#comment35760 You removed case 302 (may be unintentionally ?) We should check for m_request.sentMethodString == POST otherwise PUT (or DELETE) method with body data (i.e. m_request.method == HTTP_POST and m_request.sentMethod == (PUT || DELETE) ) will be rewritten to GET (wrong!) for 301/302 response code. This will happen in kio/job.cpp where we end up to execute the case CMD_SPECIAL code with redirectToGet != false (actually empty) kioslave/http/http.cpp https://git.reviewboard.kde.org/r/116017/#comment35764 It seems to me this case 307 is not needed, we know the only case in which a redirect to GET have to be performed is for 301/302 response and method POST (and for 303, but this case is already handled just above): 307, 308 never rewrite method. On overall i think this part of code doesn't need modifications, except removing the m_request.method = HTTP_GET; // FORCE a GET parts that, yes, seem not needed. - Andrea Iacovitti 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/#review50957 --- ... 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 - Andrea Iacovitti 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: Lokalization for KDE AppStream AppData files
Quick question: Should the AppData info be merged into the application's main po file, ot should it have a app_appdata extra po file (just like .desktop files)? I also can't really test the code I write (at least not working together) (the scripty sources are a bit confusing on what gets executed when and which script calls what - although the Umbrello diagram helps a lot!), so I will submit a patch to reviewboard soonish, to see if it's usable. Cheers, Matthias
Review Request 116097: No longer use strlcpy in kstartupconfig
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116097/ --- Review request for kde-workspace. Repository: kde-workspace Description --- No longer use strlcpy in kstartupconfig This means we no longer need to find libbsd on Linux. kstartupconfig is now uses std::string instead of pure C strings, but the performance difference should be minimal (and it is less error-prone). Diffs - CMakeLists.txt bce3cd1de65b8420a859c342db981919965071ba cmake/modules/FindLibBSD.cmake 6383083023cdbfbeffcab0cef98f48102e593d38 kstartupconfig/CMakeLists.txt ceebd6a65af4cdaa717cfb0dcff5097121cf48d9 kstartupconfig/kstartupconfig.c d8e65f48a75dad49fe6c585f89260c2a6d483809 Diff: https://git.reviewboard.kde.org/r/116097/diff/ Testing --- compiles Thanks, Alexander Richardson
Re: Review Request 116097: No longer use strlcpy in kstartupconfig
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116097/ --- (Updated Feb. 26, 2014, 7:11 p.m.) Review request for kde-workspace. Changes --- removed find_package(LibBSD) Repository: kde-workspace Description --- No longer use strlcpy in kstartupconfig This means we no longer need to find libbsd on Linux. kstartupconfig is now uses std::string instead of pure C strings, but the performance difference should be minimal (and it is less error-prone). Diffs (updated) - CMakeLists.txt bce3cd1de65b8420a859c342db981919965071ba cmake/modules/FindLibBSD.cmake 6383083023cdbfbeffcab0cef98f48102e593d38 kstartupconfig/CMakeLists.txt ceebd6a65af4cdaa717cfb0dcff5097121cf48d9 kstartupconfig/kstartupconfig.c d8e65f48a75dad49fe6c585f89260c2a6d483809 Diff: https://git.reviewboard.kde.org/r/116097/diff/ Testing --- compiles Thanks, Alexander Richardson
Re: Lokalization for KDE AppStream AppData files
On Wednesday, 2014-02-26, 18:54:43, Matthias Klumpp wrote: Quick question: Should the AppData info be merged into the application's main po file, ot should it have a app_appdata extra po file (just like .desktop files)? My guess (don't take my word on it, wait for Albert's reply) is that something similar to the handling of .desktop files would be preferable. The data at hand is very similar in nature and content. Cheers, Kevin -- Kevin Krammer, KDE developer, xdg-utils developer KDE user support, developer mentoring signature.asc Description: This is a digitally signed message part.
Re: Lokalization for KDE AppStream AppData files
El Dimecres, 26 de febrer de 2014, a les 19:45:04, Kevin Krammer va escriure: On Wednesday, 2014-02-26, 18:54:43, Matthias Klumpp wrote: Quick question: Should the AppData info be merged into the application's main po file, ot should it have a app_appdata extra po file (just like .desktop files)? My guess (don't take my word on it, wait for Albert's reply) is that something similar to the handling of .desktop files would be preferable. The data at hand is very similar in nature and content. +1 Cheers, Kevin
Re: Review Request 116097: No longer use strlcpy in kstartupconfig
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116097/#review50982 --- When you start poking at this code, why not kill the entire fopen/fgets/fixed size buffer stuff and replace it with std streams or something like that? - Rolf Eike Beer On Feb. 26, 2014, 6:11 p.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116097/ --- (Updated Feb. 26, 2014, 6:11 p.m.) Review request for kde-workspace. Repository: kde-workspace Description --- No longer use strlcpy in kstartupconfig This means we no longer need to find libbsd on Linux. kstartupconfig is now uses std::string instead of pure C strings, but the performance difference should be minimal (and it is less error-prone). Diffs - CMakeLists.txt bce3cd1de65b8420a859c342db981919965071ba cmake/modules/FindLibBSD.cmake 6383083023cdbfbeffcab0cef98f48102e593d38 kstartupconfig/CMakeLists.txt ceebd6a65af4cdaa717cfb0dcff5097121cf48d9 kstartupconfig/kstartupconfig.c d8e65f48a75dad49fe6c585f89260c2a6d483809 Diff: https://git.reviewboard.kde.org/r/116097/diff/ Testing --- compiles Thanks, Alexander Richardson
Re: KColorEdit in extragear/graphics
2014-02-23 11:56 GMT-03:00 Albert Astals Cid aa...@kde.org: El Diumenge, 23 de febrer de 2014, a les 15:53:44, Albert Astals Cid va escriure: El Dissabte, 22 de febrer de 2014, a les 19:13:39, Percy Camilo Triveño Aucahuasi va escriure: Hi all, the migration is complete now[1], big thanks to Ben for all the help and patience. The log still seems to be incomplete, first commit is kcoloredit moves to extragear svn path=/trunk/extragear/graphics/kcoloredit/; revision=674157 Seems to ignore the past of kcoloredit in trunk/KDE/kdegraphics/kcoloredit/ before that move. Can you have a look? Also it seems it has no tags at all and i can see that kcoloredit was part of http://websvn.kde.org/tags/KDE/3.0.4/kdegraphics/ so it should have a v3.0.4 tag (and probably others) I have fixed these and other problems, the history seems complete now. Can I get force-push permissions to kcoloredit.git? -- Nicolás
Re: KColorEdit in extragear/graphics
On Thu, Feb 27, 2014 at 11:15 AM, Nicolás Alvarez nicolas.alva...@gmail.com wrote: 2014-02-23 11:56 GMT-03:00 Albert Astals Cid aa...@kde.org: El Diumenge, 23 de febrer de 2014, a les 15:53:44, Albert Astals Cid va escriure: El Dissabte, 22 de febrer de 2014, a les 19:13:39, Percy Camilo Triveño Aucahuasi va escriure: Hi all, the migration is complete now[1], big thanks to Ben for all the help and patience. The log still seems to be incomplete, first commit is kcoloredit moves to extragear svn path=/trunk/extragear/graphics/kcoloredit/; revision=674157 Seems to ignore the past of kcoloredit in trunk/KDE/kdegraphics/kcoloredit/ before that move. Can you have a look? Also it seems it has no tags at all and i can see that kcoloredit was part of http://websvn.kde.org/tags/KDE/3.0.4/kdegraphics/ so it should have a v3.0.4 tag (and probably others) I have fixed these and other problems, the history seems complete now. Can I get force-push permissions to kcoloredit.git? I believe everything has now been sorted (per IRC chat). -- Nicolás Thanks, Ben
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 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/#review50995 --- This review has been submitted with commit 9fd3e380f3f1967b8ba01c5a248b6cddf79450a7 by Dawit Alemayehu to branch KDE/4.12. - Commit Hook On Feb. 26, 2014, 7:19 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, 7:19 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 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
