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 115723: Use Q_OS_UNIX instead of HAVE_X11 to determine the platform we are on
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115723/#review49709 --- Ship it! That seems like a reasonable compromise to me. I wonder what Mozilla/Chromium send when the windowing system is something other than X11. Probably the same thing. - Dawit Alemayehu On Feb. 13, 2014, 1:03 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115723/ --- (Updated Feb. 13, 2014, 1:03 p.m.) Review request for KDE Frameworks, Dawit Alemayehu and Bernhard Beschow. Repository: kio Description --- Use Q_OS_UNIX instead of HAVE_X11 to determine the platform we are on We cannot properly determine the windowing system platform on unix like systems in kprotocolmanager as it's not linking gui. Thus we don't know whether we are on X11 or Wayland and there is no proper way to figure it out, because both DISPLAY and WAYLAND_DISPLAY could be defined. As a solution we just force the platform to be always X11 when we are on unix like systems (modulo mac). Diffs - src/core/kprotocolmanager.cpp f81b6797887eebd868c36b98e867eb055b05a1e2 Diff: https://git.reviewboard.kde.org/r/115723/diff/ Testing --- Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
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 115613: Drop platform name from default user agent string
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115613/#review49479 --- Well the platform name was added for compatibility with what Firefox at the time. And Chromium seems to have adapted that as well. The latest stable version of Firefox (version 27) for example sends the following user agent string by default: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 And the latest Chromium (version 32) sends the following: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.107 Safari/537.36 So removing the platform name from the user-agent string might have consequences on sites that rely on it. - Dawit Alemayehu On Feb. 10, 2014, 9:15 a.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115613/ --- (Updated Feb. 10, 2014, 9:15 a.m.) Review request for KDE Frameworks, Dawit Alemayehu and Bernhard Beschow. Repository: kio Description --- Drop platform name from default user agent string The platform name (e.g. X11) was currently broken on compile time. On Linux it returned unknown and on all other platforms the same name as already included in the OS name. We cannot really determine the platform name as this is a core application and the Qt's platform name is only available in a GUI application. Compile time is no solution as we cannot know whether the binary is executed on X11, Wayland, Android or whatever. Diffs - src/core/kprotocolmanager.cpp f81b6797887eebd868c36b98e867eb055b05a1e2 Diff: https://git.reviewboard.kde.org/r/115613/diff/ Testing --- Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
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
Review Request 114926: Append -viewmode to the view actions from DolphinPart
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114926/ --- Review request for KDE Base Apps and David Faure. Bugs: 322686 http://bugs.kde.org/show_bug.cgi?id=322686 Repository: kde-baseapps Description --- The attached patch fixes a bug where the actions for the views from the DolphinPart don't have -viewmode appended to them and hence are not correctly updated when slotInternalViewModeChanged() is invoked. This results the View-View Mode items not being updated correctly when the mode is changed through Dolphin's view mode toolbar. Diffs - konqueror/src/konqmainwindow.cpp 8a21c1b Diff: https://git.reviewboard.kde.org/r/114926/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 114926: Append -viewmode to the view actions from DolphinPart
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114926/ --- (Updated Jan. 9, 2014, 1:32 p.m.) Status -- This change has been marked as submitted. Review request for KDE Base Apps and David Faure. Bugs: 322686 http://bugs.kde.org/show_bug.cgi?id=322686 Repository: kde-baseapps Description --- The attached patch fixes a bug where the actions for the views from the DolphinPart don't have -viewmode appended to them and hence are not correctly updated when slotInternalViewModeChanged() is invoked. This results the View-View Mode items not being updated correctly when the mode is changed through Dolphin's view mode toolbar. Diffs - konqueror/src/konqmainwindow.cpp 8a21c1b Diff: https://git.reviewboard.kde.org/r/114926/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 114780: Remove the Undo closed window entry from the manager before opening the closed window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114780/ --- Review request for KDE Base Apps and David Faure. Bugs: 301974 http://bugs.kde.org/show_bug.cgi?id=301974 Repository: kde-baseapps Description --- This patch fixes part of a problem that was causing Konqueror to crash when undo'ing a previously closed window. The actual cause of the crash is the restored window was not removed from KonqClosedWindowsManager before it was restored. As a result, the Undo: Closed Window action was enabled under the edit menu even when there was no window other closed window! The reason this happens is because the current action that is being undone was note removed from the undo manager before the action itself was carried out. Hence, when the closed window starts it checks and see there was a window closed before (itself) that can be restored and hence enables the Edit-Undo:... menu. With this fix, the reopened window will not longer show that there is another Undo action when there isn't one. On a related note I have already fixed the actual place where the crash was happening as a result of this bug, KIO::FileUndoManager::undo. Though it was not the source of the crash, that code should not invoke first/last on a list without first checking for empty. Diffs - konqueror/src/konqundomanager.cpp cdbb1bd Diff: https://git.reviewboard.kde.org/r/114780/diff/ Testing --- 1. Close all Konqueror instances. 2. Remove the [Undo] section from konquerorrc. 3. Launch Konqueror and visit a site or browse a directory. 4. Close it. 5. Launch it again and click on Edit-Undo: Closed Window. 6. In the new Konqueror window that is opened with the site or directory you last visited, repeat step #5. Step #6 should result in a crash. Thanks, Dawit Alemayehu
Re: Review Request 114780: Remove the Undo closed window entry from the manager before opening the closed window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114780/ --- (Updated Jan. 1, 2014, 7:59 p.m.) Status -- This change has been marked as submitted. Review request for KDE Base Apps and David Faure. Bugs: 301974 http://bugs.kde.org/show_bug.cgi?id=301974 Repository: kde-baseapps Description --- This patch fixes part of a problem that was causing Konqueror to crash when undo'ing a previously closed window. The actual cause of the crash is the restored window was not removed from KonqClosedWindowsManager before it was restored. As a result, the Undo: Closed Window action was enabled under the edit menu even when there was no window other closed window! The reason this happens is because the current action that is being undone was note removed from the undo manager before the action itself was carried out. Hence, when the closed window starts it checks and see there was a window closed before (itself) that can be restored and hence enables the Edit-Undo:... menu. With this fix, the reopened window will not longer show that there is another Undo action when there isn't one. On a related note I have already fixed the actual place where the crash was happening as a result of this bug, KIO::FileUndoManager::undo. Though it was not the source of the crash, that code should not invoke first/last on a list without first checking for empty. Diffs - konqueror/src/konqundomanager.cpp cdbb1bd Diff: https://git.reviewboard.kde.org/r/114780/diff/ Testing --- 1. Close all Konqueror instances. 2. Remove the [Undo] section from konquerorrc. 3. Launch Konqueror and visit a site or browse a directory. 4. Close it. 5. Launch it again and click on Edit-Undo: Closed Window. 6. In the new Konqueror window that is opened with the site or directory you last visited, repeat step #5. Step #6 should result in a crash. Thanks, Dawit Alemayehu
Re: Review Request 114479: Select the newly created bookmark folder as the current item
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114479/ --- (Updated Dec. 24, 2013, 2:26 p.m.) Review request for kdelibs and David Faure. Changes --- Updated patch based on feedback. Bugs: 152158 http://bugs.kde.org/show_bug.cgi?id=152158 Repository: kdelibs Description --- When a user creates a new bookmark folder in the Add Bookmark dialog, make it the current selected item. Diffs (updated) - kio/bookmarks/kbookmarkdialog.h a746c22 kio/bookmarks/kbookmarkdialog.cc 713ceff Diff: https://git.reviewboard.kde.org/r/114479/diff/ Testing --- File Attachments Add new folder w/o patch https://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/1be1b4c9-eddd-43cf-b3fa-18cc0a44b212__before.png Add new folder w/ patch https://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/353b3c50-ccc8-4390-9d76-a9f85a703987__after.png Thanks, Dawit Alemayehu
Re: Review Request 114479: Select the newly created bookmark folder as the current item
On Dec. 24, 2013, 8:14 a.m., David Faure wrote: kio/bookmarks/kbookmarkdialog.cc, line 350 https://git.reviewboard.kde.org/r/114479/diff/3/?file=227047#file227047line350 this method could now call the other one with an empty selectGroup, to remove the code duplication, right? Indeed. Fixed. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114479/#review46109 --- On Dec. 24, 2013, 6:04 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114479/ --- (Updated Dec. 24, 2013, 6:04 a.m.) Review request for kdelibs and David Faure. Bugs: 152158 http://bugs.kde.org/show_bug.cgi?id=152158 Repository: kdelibs Description --- When a user creates a new bookmark folder in the Add Bookmark dialog, make it the current selected item. Diffs - kio/bookmarks/kbookmarkdialog.h a746c22 kio/bookmarks/kbookmarkdialog.cc 713ceff Diff: https://git.reviewboard.kde.org/r/114479/diff/ Testing --- File Attachments Add new folder w/o patch https://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/1be1b4c9-eddd-43cf-b3fa-18cc0a44b212__before.png Add new folder w/ patch https://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/353b3c50-ccc8-4390-9d76-a9f85a703987__after.png Thanks, Dawit Alemayehu
Re: Review Request 114479: Select the newly created bookmark folder as the current item
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114479/ --- (Updated Dec. 24, 2013, 2:50 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Bugs: 152158 http://bugs.kde.org/show_bug.cgi?id=152158 Repository: kdelibs Description --- When a user creates a new bookmark folder in the Add Bookmark dialog, make it the current selected item. Diffs - kio/bookmarks/kbookmarkdialog.h a746c22 kio/bookmarks/kbookmarkdialog.cc 713ceff Diff: https://git.reviewboard.kde.org/r/114479/diff/ Testing --- File Attachments Add new folder w/o patch https://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/1be1b4c9-eddd-43cf-b3fa-18cc0a44b212__before.png Add new folder w/ patch https://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/353b3c50-ccc8-4390-9d76-a9f85a703987__after.png Thanks, Dawit Alemayehu
Re: Review Request 114436: Set WindowModality of all KIO message box to Qt::WindowModal
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114436/ --- (Updated Dec. 24, 2013, 3:01 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs, David Faure and Frank Reininghaus. Repository: kdelibs Description --- The attached patch changes the WindowModality of all the message/information boxes displayed by KIO::JobUiDelegate to Qt::WindowModal instead of Qt::ApplicationModal. This prevents a message box in one window from blocking all other windows. Diffs - kio/kio/jobuidelegate.cpp 8534863 Diff: https://git.reviewboard.kde.org/r/114436/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 114436: Set WindowModality of all KIO message box to Qt::WindowModal
On Dec. 13, 2013, 2:27 p.m., Frank Reininghaus wrote: Thanks for looking into this, Dawit! I greatly appreciate this effort. Two questions come to my mind: 1. Why should these dialogs be modal at all? Everything that KIO does is asynchronous, so it could very well be that the window isn't even showing the directory (where the action took place that triggered the dialog) any more. 2. Since every little change can have unexpected side effects, and the modality issue is not causing a lot of trouble for users right now (please correct me if I'm wrong), maybe this change should better be done in master? Currently, the only situation in which a single process can have multiple windows that can perform file management actions is that there are two Konqueror windows, one of which was opened from the other one with Open New Window, I think (but I might be overlooking some other possibilities). Some background for people who have not followed the modality discussion in the past: some time ago, Thomas raised the question why Dolphin is not a KUniqueApplication any more. This was done mostly because Strigi made Dolphin crash a lot, and it was quite annoying for users to see all their Dolphin windows disappear if one of them crashed (this is not a big problem any more), but also because it was a bit irritating that KIO dialogs would freeze all Dolphin windows. Some more information can be found in these threads: http://lists.kde.org/?t=13752968312r=1w=2 http://lists.kde.org/?t=13753723594r=1w=2 Thomas Lübking wrote: 1. Why should these dialogs be modal at all? Unless anybody has a better explanation I assume it was done because of either a) the wrong assumption that modal equals transient b) the assumption of (a) actually may hold on some systems? A modal window is used if sequential action is mandatory, eg. if you open a file you can not edit it before you opened it - the modal dialog makes sense to enforce the workflow and assert() it in the code. This is obviously not the case here: the system MUST be prepared to filesystem changes during the nested eventloop of the modal dilaog, eg. while the dialog asks do you really want to delete foo/bar.txt this could just happen in another dolphin window, in konsole, VT1 or through a script or cron job. On top of this, I do not even think the dialog should be transient. Eg. I often start a longer (network, crap USB stick) copy job and close dolphin immediately. Popping up questions (override) arrive for the copy job and not the causing process (long closed dolphin) The user must get aware that this action is currently halted and requires interaction to continue, but that isn't related to a particular other window. Some system interaction spot would be nice but it had to significantly differ from the common i don't care notification that pops up because phonon thinks it lost a resource or so. Alternatively the process indicator in the notification area could just start blinking or show a interaction required! message/icon/whatsoever. This is however probably beyond KDE4. The fallback (non-plasma context) solution could simply be a keep above on all desktops dialog (it doesn't have to get the focus, but must show up visible) what might be a usable approach even for KDE4 Kai Uwe Broulik wrote: Unfortunately that [1] never made it to a final state :-/ [1] http://en.munknex.net/2012/06/new-kde-copy-dialog-first-preview.html Dawit Alemayehu wrote: 1. Why should these dialogs be modal at all? Everything that KIO does is asynchronous, so it could very well be that the window isn't even showing the directory (where the action took place that triggered the dialog) any more. Which dialogs? There are dialogs requested by the jobs and those that originate from the ioslaves themselves. The prompts from the jobs and ioslaves are distinctly different and cannot be lumped together. Though I am not the original creator of these dialogs, I can think of at least one reason why it might have been done this way. Making the dialogs modal is the simplest way to avoid the problem of multiple File Already Exists dialog boxes from popping up when copying/moving several files and more than one of those files exist in the destination. Another reason is the jobs themselves might not originally have been written in such a way that they are capable of accommodating asynch responses from prompts. As far as prompts from the ioslaves, the user almost always needs to respond to them in order for the ioslaves to proceed. Almost all are mandatory prompts. For example, when a user visits a site and gets prompted with untrusted SSL certificate warning dialog
Re: Review Request 114479: Select the newly created bookmark folder as the current item
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114479/ --- (Updated Dec. 24, 2013, 6 a.m.) Review request for kdelibs and David Faure. Changes --- Overloaded the fillGroup function to allow the current item to be selected. Bugs: 152158 http://bugs.kde.org/show_bug.cgi?id=152158 Repository: kdelibs Description --- When a user creates a new bookmark folder in the Add Bookmark dialog, make it the current selected item. Diffs (updated) - kio/bookmarks/kbookmarkdialog.h a746c22 kio/bookmarks/kbookmarkdialog.cc 713ceff Diff: https://git.reviewboard.kde.org/r/114479/diff/ Testing --- File Attachments Add new folder w/o patch https://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/1be1b4c9-eddd-43cf-b3fa-18cc0a44b212__before.png Add new folder w/ patch https://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/353b3c50-ccc8-4390-9d76-a9f85a703987__after.png Thanks, Dawit Alemayehu
Re: Review Request 114479: Select the newly created bookmark folder as the current item
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114479/ --- (Updated Dec. 24, 2013, 6:04 a.m.) Review request for kdelibs and David Faure. Changes --- Minor update. Bugs: 152158 http://bugs.kde.org/show_bug.cgi?id=152158 Repository: kdelibs Description --- When a user creates a new bookmark folder in the Add Bookmark dialog, make it the current selected item. Diffs (updated) - kio/bookmarks/kbookmarkdialog.h a746c22 kio/bookmarks/kbookmarkdialog.cc 713ceff Diff: https://git.reviewboard.kde.org/r/114479/diff/ Testing --- File Attachments Add new folder w/o patch https://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/1be1b4c9-eddd-43cf-b3fa-18cc0a44b212__before.png Add new folder w/ patch https://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/353b3c50-ccc8-4390-9d76-a9f85a703987__after.png Thanks, Dawit Alemayehu
Re: Review Request 114473: Change modality of dialog in KRun::displayOpenWithDialog to Qt::WindowModal
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114473/ --- (Updated Dec. 22, 2013, 1:29 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Bugs: 153421 http://bugs.kde.org/show_bug.cgi?id=153421 Repository: kdelibs Description --- The attached patch changes the KOpenWith dialog in KRun::displayOpenWithDialog from blocking Konqueror windows when a user clicks on OpenWith in the context menu or unknown file in the Dolphin part. Diffs - kio/kio/krun.cpp ad5656e Diff: https://git.reviewboard.kde.org/r/114473/diff/ Testing --- Thanks, Dawit Alemayehu
[kdelibs] [Bug 302342] KWebView with remote file url causes endless KIO redirections when kde-runtime is built without samba libraries
https://bugs.kde.org/show_bug.cgi?id=302342 Dawit Alemayehu ada...@kde.org changed: What|Removed |Added Component|file|kdewebkit Platform|Compiled Sources|MS Windows Assignee|kde-windows@kde.org |webkit-de...@kde.org Product|kio |kdelibs -- You are receiving this mail because: You are the assignee for the bug. ___ Kde-windows mailing list Kde-windows@kde.org https://mail.kde.org/mailman/listinfo/kde-windows
Re: Review Request 114436: Set WindowModality of all KIO message box to Qt::WindowModal
On Dec. 13, 2013, 2:27 p.m., Frank Reininghaus wrote: Thanks for looking into this, Dawit! I greatly appreciate this effort. Two questions come to my mind: 1. Why should these dialogs be modal at all? Everything that KIO does is asynchronous, so it could very well be that the window isn't even showing the directory (where the action took place that triggered the dialog) any more. 2. Since every little change can have unexpected side effects, and the modality issue is not causing a lot of trouble for users right now (please correct me if I'm wrong), maybe this change should better be done in master? Currently, the only situation in which a single process can have multiple windows that can perform file management actions is that there are two Konqueror windows, one of which was opened from the other one with Open New Window, I think (but I might be overlooking some other possibilities). Some background for people who have not followed the modality discussion in the past: some time ago, Thomas raised the question why Dolphin is not a KUniqueApplication any more. This was done mostly because Strigi made Dolphin crash a lot, and it was quite annoying for users to see all their Dolphin windows disappear if one of them crashed (this is not a big problem any more), but also because it was a bit irritating that KIO dialogs would freeze all Dolphin windows. Some more information can be found in these threads: http://lists.kde.org/?t=13752968312r=1w=2 http://lists.kde.org/?t=13753723594r=1w=2 Thomas Lübking wrote: 1. Why should these dialogs be modal at all? Unless anybody has a better explanation I assume it was done because of either a) the wrong assumption that modal equals transient b) the assumption of (a) actually may hold on some systems? A modal window is used if sequential action is mandatory, eg. if you open a file you can not edit it before you opened it - the modal dialog makes sense to enforce the workflow and assert() it in the code. This is obviously not the case here: the system MUST be prepared to filesystem changes during the nested eventloop of the modal dilaog, eg. while the dialog asks do you really want to delete foo/bar.txt this could just happen in another dolphin window, in konsole, VT1 or through a script or cron job. On top of this, I do not even think the dialog should be transient. Eg. I often start a longer (network, crap USB stick) copy job and close dolphin immediately. Popping up questions (override) arrive for the copy job and not the causing process (long closed dolphin) The user must get aware that this action is currently halted and requires interaction to continue, but that isn't related to a particular other window. Some system interaction spot would be nice but it had to significantly differ from the common i don't care notification that pops up because phonon thinks it lost a resource or so. Alternatively the process indicator in the notification area could just start blinking or show a interaction required! message/icon/whatsoever. This is however probably beyond KDE4. The fallback (non-plasma context) solution could simply be a keep above on all desktops dialog (it doesn't have to get the focus, but must show up visible) what might be a usable approach even for KDE4 Kai Uwe Broulik wrote: Unfortunately that [1] never made it to a final state :-/ [1] http://en.munknex.net/2012/06/new-kde-copy-dialog-first-preview.html Dawit Alemayehu wrote: 1. Why should these dialogs be modal at all? Everything that KIO does is asynchronous, so it could very well be that the window isn't even showing the directory (where the action took place that triggered the dialog) any more. Which dialogs? There are dialogs requested by the jobs and those that originate from the ioslaves themselves. The prompts from the jobs and ioslaves are distinctly different and cannot be lumped together. Though I am not the original creator of these dialogs, I can think of at least one reason why it might have been done this way. Making the dialogs modal is the simplest way to avoid the problem of multiple File Already Exists dialog boxes from popping up when copying/moving several files and more than one of those files exist in the destination. Another reason is the jobs themselves might not originally have been written in such a way that they are capable of accommodating asynch responses from prompts. As far as prompts from the ioslaves, the user almost always needs to respond to them in order for the ioslaves to proceed. Almost all are mandatory prompts. For example, when a user visits a site and gets prompted with untrusted SSL certificate warning dialog
Review Request 114473: Change modality of dialog in KRun::displayOpenWithDialog to Qt::WindowModal
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114473/ --- Review request for kdelibs and David Faure. Bugs: 153421 http://bugs.kde.org/show_bug.cgi?id=153421 Repository: kdelibs Description --- The attached patch changes the KOpenWith dialog in KRun::displayOpenWithDialog from blocking Konqueror windows when a user clicks on OpenWith in the context menu or unknown file in the Dolphin part. Diffs - kio/kio/krun.cpp ad5656e Diff: http://git.reviewboard.kde.org/r/114473/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 114479: Select the newly created bookmark folder as the current item
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114479/ --- Review request for kdelibs and David Faure. Bugs: 152158 http://bugs.kde.org/show_bug.cgi?id=152158 Repository: kdelibs Description --- When a user creates a new bookmark folder in the Add Bookmark dialog, make it the current selected item. Diffs - kio/bookmarks/kbookmarkdialog.cc 713ceff Diff: http://git.reviewboard.kde.org/r/114479/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 114479: Select the newly created bookmark folder as the current item
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114479/ --- (Updated Dec. 15, 2013, 4:40 p.m.) Review request for kdelibs and David Faure. Bugs: 152158 http://bugs.kde.org/show_bug.cgi?id=152158 Repository: kdelibs Description --- When a user creates a new bookmark folder in the Add Bookmark dialog, make it the current selected item. Diffs - kio/bookmarks/kbookmarkdialog.cc 713ceff Diff: http://git.reviewboard.kde.org/r/114479/diff/ Testing --- File Attachments (updated) Add new folder w/o patch http://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/1be1b4c9-eddd-43cf-b3fa-18cc0a44b212__before.png Add new folder w/ patch http://git.reviewboard.kde.org/media/uploaded/files/2013/12/15/353b3c50-ccc8-4390-9d76-a9f85a703987__after.png Thanks, Dawit Alemayehu
Re: Review Request 114436: Set WindowModality of all KIO message box to Qt::WindowModal
such prompts non-modal! Otherwise, the user can do whatever including going to a new site by entering another address in the address bar. If the user then goes back and chose to accept the untrusted SSL certificate the browser will to go back to the previous URL. 2. Since every little change can have unexpected side effects, and the modality issue is not causing a lot of trouble for users right now please correct me if I'm wrong), maybe this change should better be done in master? Currently, the only situation in which a single process can have multiple windows that can perform file management actions is that there are two Konqueror windows, one of which was opened from the other one with Open New Window, I think (but I might be overlooking some other possibilities). Well I have no particular preference to what branch the patch is applied. I just did not see a problem with applying it to the current stable branch because the issue is actually a bug and the change itself does not get rid of modality. It simply restricts it to a given window. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114436/#review45646 --- On Dec. 13, 2013, 1:53 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114436/ --- (Updated Dec. 13, 2013, 1:53 p.m.) Review request for kdelibs, David Faure and Frank Reininghaus. Repository: kdelibs Description --- The attached patch changes the WindowModality of all the message/information boxes displayed by KIO::JobUiDelegate to Qt::WindowModal instead of Qt::ApplicationModal. This prevents a message box in one window from blocking all other windows. Diffs - kio/kio/jobuidelegate.cpp 8534863 Diff: http://git.reviewboard.kde.org/r/114436/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 114436: Set WindowModality of all KIO message box to Qt::WindowModal
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114436/ --- Review request for kdelibs, David Faure and Frank Reininghaus. Repository: kdelibs Description --- The attached patch changes the WindowModality of all the message/information boxes displayed by KIO::JobUiDelegate to Qt::WindowModal instead of Qt::ApplicationModal. This prevents a message box in one window from blocking all other windows. Diffs - kio/kio/jobuidelegate.cpp 8534863 Diff: http://git.reviewboard.kde.org/r/114436/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 114105: kcm proxy: fix for noProxyFor setting when 'system proxy' type
On Nov. 28, 2013, 5:17 a.m., Dawit Alemayehu wrote: konqueror/settings/kio/kproxydlg.cpp, line 295 http://git.reviewboard.kde.org/r/114105/diff/1/?file=219633#file219633line295 nitpick: no need for KSaveIOConfig here. Andrea Iacovitti wrote: ?. Not sure what do you mean Disregard this. I thought the function was being called from KSaveIOConfig itself. My mistake. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114105/#review44646 --- On Nov. 27, 2013, 9:22 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114105/ --- (Updated Nov. 27, 2013, 9:22 p.m.) Review request for KDE Base Apps and Dawit Alemayehu. Repository: kde-baseapps Description --- In case of 'system proxy' proxyType, NoProxyFor config key holds the name of the env variable (e.g. no_proxy) and not its value. Because of this KProtocolManager::noProxyFor() can not be used to get NoProxyFor config setting in KProxyDialog::load(), as it returns the resolved value of the environment variable and not its name: i added helper method KSaveIOConfig::noProxyFor() to read that value directly from config file. Also make sure to uncheck showEnvValueCheckBox before filling proxy edit fields with environment variable names in KProxyDialog::on_autoDetectButton_clicked(). Diffs - konqueror/settings/kio/kproxydlg.cpp e80afeb konqueror/settings/kio/ksaveioconfig.h 2318198 konqueror/settings/kio/ksaveioconfig.cpp c822f7b Diff: http://git.reviewboard.kde.org/r/114105/diff/ Testing --- To reproduce the issue: $ export no_proxy=kde.org $ kcmshell4 proxy choose Use system proxy configuration, push Auto Detect button, close the gui interface and reopen it: $ kcmshell4 proxy see how Exceptions fields contains kde.org and not no_proxy Thanks, Andrea Iacovitti
Re: Review Request 114105: kcm proxy: fix for noProxyFor setting when 'system proxy' type
On Nov. 28, 2013, 5:17 a.m., Dawit Alemayehu wrote: konqueror/settings/kio/kproxydlg.cpp, line 444 http://git.reviewboard.kde.org/r/114105/diff/1/?file=219633#file219633line444 This does not make sense. I explicitly check show me the value and you uncheck it as a result of me clicking on the Auto Detect button? No. Andrea Iacovitti wrote: This is my (user) point of view: if i'm configuring system proxy and i click Auto Detect button i'm interested to know/see what env vars i'm actually going to use, not really their values. If i'm a curious user i could ask the gui to show me their *actual* values (those values can also not be the same later on, for example because i re-assigned the env vars being configured). This change is also a fast and simple fix for the following use case: suppose i have set the following env vars on my system $export my_proxy=http://localhost: $export http_proxy=http://localhost: and i have configured system proxy to use my_proxy (in kioslaverc httpProxy=my_proxy). I open proxy config module and check the Show the value of the environment variables checkbox, http://localhost:; is showed in HTTP Proxy:, then i push Auto Detect, HTTP Proxy: changes to http://localhost:;, i save and exit: nothing is really saved because of the logic adopted in KProxyDialog::save(). Well when a user explicitly checks an option, which by default is not checked, it should be not reverted as a result of another action. I as a user do no want to see that. After all I purposefully checked that option to begin with, which can only mean one thing. I actually wanted to see the values and not the *actual* proxy variables. Otherwise, I would not have checked it to begin with. So I do not agree with this particular change and would like to see it removed from this patch. Having said that the logic for the auto detection button needs to be fixed to ensure that it shows the proper text in those line edits. Currently it does not and that is what breaks KProxyDialog::save(), but that is easy enough to fix and something that needs to be absent this patch. I will deal with that after the rest of this patch is committed. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114105/#review44646 --- On Nov. 27, 2013, 9:22 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114105/ --- (Updated Nov. 27, 2013, 9:22 p.m.) Review request for KDE Base Apps and Dawit Alemayehu. Repository: kde-baseapps Description --- In case of 'system proxy' proxyType, NoProxyFor config key holds the name of the env variable (e.g. no_proxy) and not its value. Because of this KProtocolManager::noProxyFor() can not be used to get NoProxyFor config setting in KProxyDialog::load(), as it returns the resolved value of the environment variable and not its name: i added helper method KSaveIOConfig::noProxyFor() to read that value directly from config file. Also make sure to uncheck showEnvValueCheckBox before filling proxy edit fields with environment variable names in KProxyDialog::on_autoDetectButton_clicked(). Diffs - konqueror/settings/kio/kproxydlg.cpp e80afeb konqueror/settings/kio/ksaveioconfig.h 2318198 konqueror/settings/kio/ksaveioconfig.cpp c822f7b Diff: http://git.reviewboard.kde.org/r/114105/diff/ Testing --- To reproduce the issue: $ export no_proxy=kde.org $ kcmshell4 proxy choose Use system proxy configuration, push Auto Detect button, close the gui interface and reopen it: $ kcmshell4 proxy see how Exceptions fields contains kde.org and not no_proxy Thanks, Andrea Iacovitti
Re: Review Request 114105: kcm proxy: fix for noProxyFor setting when 'system proxy' type
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114105/#review44690 --- Ship it! Minus this change = mUi.showEnvValueCheckBox-setChecked(false); - Dawit Alemayehu On Nov. 27, 2013, 9:22 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114105/ --- (Updated Nov. 27, 2013, 9:22 p.m.) Review request for KDE Base Apps and Dawit Alemayehu. Repository: kde-baseapps Description --- In case of 'system proxy' proxyType, NoProxyFor config key holds the name of the env variable (e.g. no_proxy) and not its value. Because of this KProtocolManager::noProxyFor() can not be used to get NoProxyFor config setting in KProxyDialog::load(), as it returns the resolved value of the environment variable and not its name: i added helper method KSaveIOConfig::noProxyFor() to read that value directly from config file. Also make sure to uncheck showEnvValueCheckBox before filling proxy edit fields with environment variable names in KProxyDialog::on_autoDetectButton_clicked(). Diffs - konqueror/settings/kio/kproxydlg.cpp e80afeb konqueror/settings/kio/ksaveioconfig.h 2318198 konqueror/settings/kio/ksaveioconfig.cpp c822f7b Diff: http://git.reviewboard.kde.org/r/114105/diff/ Testing --- To reproduce the issue: $ export no_proxy=kde.org $ kcmshell4 proxy choose Use system proxy configuration, push Auto Detect button, close the gui interface and reopen it: $ kcmshell4 proxy see how Exceptions fields contains kde.org and not no_proxy Thanks, Andrea Iacovitti
Re: Review Request 112463: Port SMB kioslave to KF5/Qt5
On Nov. 26, 2013, 5:12 p.m., Kevin Ottens wrote: It's been stalled for almost three months now, any chance to see progress or should it be discarded? Mark Gaiser wrote: No, it should most certainly not be disgarded. It was even working when i posted this post up for review. Dawit, how are you doing in the SMB improvements that you had in mind? Sorry, I forgot to inform you. I have already merged my changes into 4.12 and master branches. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/#review44515 --- On Sept. 2, 2013, 7:16 p.m., Mark Gaiser wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/ --- (Updated Sept. 2, 2013, 7:16 p.m.) Review request for KDE Runtime and KDE Frameworks. Repository: kde-runtime Description --- This is the initial port! I added two TODO lines in the diff for parts where i'm not sure if I've ported them correctly. Also, i needed a change in FindSamba.cmake to even get the samba detection working. That reviewrequest is waiting here: https://git.reviewboard.kde.org/r/112448/ you're probably OK if you still use samba 3.x Once i know that this is actually working then i will comment some qDebug lines. Diffs - kioslave/CMakeLists.txt ff66ab6 kioslave/smb/CMakeLists.txt a3a2265 kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb.cpp 2c2523a kioslave/smb/kio_smb_auth.cpp 4d236b4 kioslave/smb/kio_smb_browse.cpp fec6449 kioslave/smb/kio_smb_config.cpp 81ce29c kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/kio_smb_file.cpp 827a519 kioslave/smb/kio_smb_internal.h b895b81 kioslave/smb/kio_smb_internal.cpp 3c35583 kioslave/smb/kio_smb_mount.cpp a5a7e8e Diff: http://git.reviewboard.kde.org/r/112463/diff/ Testing --- It compiles and gets loaded just fine. I tried testing this on an actual samba share, but i kept getting a 111 error (connection refused) from kio_smb so i'm hoping that is a local issue here. If someone else could try this out and verify that it's either working or broken. Thanks, Mark Gaiser ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114105: kcm proxy: fix for noProxyFor setting when 'system proxy' type
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114105/#review44646 --- konqueror/settings/kio/kproxydlg.cpp http://git.reviewboard.kde.org/r/114105/#comment31875 Please remove this unnecessary whitespace since you already modified this file. konqueror/settings/kio/kproxydlg.cpp http://git.reviewboard.kde.org/r/114105/#comment31873 nitpick: no need for KSaveIOConfig here. konqueror/settings/kio/kproxydlg.cpp http://git.reviewboard.kde.org/r/114105/#comment31874 This does not make sense. I explicitly check show me the value and you uncheck it as a result of me clicking on the Auto Detect button? No. - Dawit Alemayehu On Nov. 27, 2013, 9:22 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114105/ --- (Updated Nov. 27, 2013, 9:22 p.m.) Review request for KDE Base Apps and Dawit Alemayehu. Repository: kde-baseapps Description --- In case of 'system proxy' proxyType, NoProxyFor config key holds the name of the env variable (e.g. no_proxy) and not its value. Because of this KProtocolManager::noProxyFor() can not be used to get NoProxyFor config setting in KProxyDialog::load(), as it returns the resolved value of the environment variable and not its name: i added helper method KSaveIOConfig::noProxyFor() to read that value directly from config file. Also make sure to uncheck showEnvValueCheckBox before filling proxy edit fields with environment variable names in KProxyDialog::on_autoDetectButton_clicked(). Diffs - konqueror/settings/kio/kproxydlg.cpp e80afeb konqueror/settings/kio/ksaveioconfig.h 2318198 konqueror/settings/kio/ksaveioconfig.cpp c822f7b Diff: http://git.reviewboard.kde.org/r/114105/diff/ Testing --- To reproduce the issue: $ export no_proxy=kde.org $ kcmshell4 proxy choose Use system proxy configuration, push Auto Detect button, close the gui interface and reopen it: $ kcmshell4 proxy see how Exceptions fields contains kde.org and not no_proxy Thanks, Andrea Iacovitti
Re: Review Request 112463: Port SMB kioslave to KF5/Qt5
On Nov. 26, 2013, 5:12 p.m., Kevin Ottens wrote: It's been stalled for almost three months now, any chance to see progress or should it be discarded? Mark Gaiser wrote: No, it should most certainly not be disgarded. It was even working when i posted this post up for review. Dawit, how are you doing in the SMB improvements that you had in mind? Sorry, I forgot to inform you. I have already merged my changes into 4.12 and master branches. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/#review44515 --- On Sept. 2, 2013, 7:16 p.m., Mark Gaiser wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/ --- (Updated Sept. 2, 2013, 7:16 p.m.) Review request for KDE Runtime and KDE Frameworks. Repository: kde-runtime Description --- This is the initial port! I added two TODO lines in the diff for parts where i'm not sure if I've ported them correctly. Also, i needed a change in FindSamba.cmake to even get the samba detection working. That reviewrequest is waiting here: https://git.reviewboard.kde.org/r/112448/ you're probably OK if you still use samba 3.x Once i know that this is actually working then i will comment some qDebug lines. Diffs - kioslave/CMakeLists.txt ff66ab6 kioslave/smb/CMakeLists.txt a3a2265 kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb.cpp 2c2523a kioslave/smb/kio_smb_auth.cpp 4d236b4 kioslave/smb/kio_smb_browse.cpp fec6449 kioslave/smb/kio_smb_config.cpp 81ce29c kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/kio_smb_file.cpp 827a519 kioslave/smb/kio_smb_internal.h b895b81 kioslave/smb/kio_smb_internal.cpp 3c35583 kioslave/smb/kio_smb_mount.cpp a5a7e8e Diff: http://git.reviewboard.kde.org/r/112463/diff/ Testing --- It compiles and gets loaded just fine. I tried testing this on an actual samba share, but i kept getting a 111 error (connection refused) from kio_smb so i'm hoping that is a local issue here. If someone else could try this out and verify that it's either working or broken. Thanks, Mark Gaiser
Re: Review Request 112982: copy[To|From]File support for kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Nov. 25, 2013, 12:34 a.m.) Review request for KDE Runtime and David Faure. Changes --- Use #ifdef for permission conversion code present only in KDE 4.11.80 or higher. That way I do not have to change the required kdelibs version to compile kde-runtime. Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- The attach patch adds support for the following to kio_smb: - copyToFile/copyFromFile optimizations so uploading and downloading files from window shares is faster so long as one end is a local file. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Diffs (updated) - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 1f013df kioslave/smb/kio_smb_internal.h b895b81 kioslave/smb/kio_smb_internal.cpp 3c35583 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- - Copied files to and from smb share. - Resumed interrupted file copy to and from smb share. Thanks, Dawit Alemayehu
Re: Review Request 112982: copy[To|From]File support for kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Nov. 25, 2013, 5:34 a.m.) Status -- This change has been marked as submitted. Review request for KDE Runtime and David Faure. Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- The attach patch adds support for the following to kio_smb: - copyToFile/copyFromFile optimizations so uploading and downloading files from window shares is faster so long as one end is a local file. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Diffs - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 1f013df kioslave/smb/kio_smb_internal.h b895b81 kioslave/smb/kio_smb_internal.cpp 3c35583 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- - Copied files to and from smb share. - Resumed interrupted file copy to and from smb share. Thanks, Dawit Alemayehu
Re: Review Request 113915: kio_smb, Increase MAX_XFER_BUF_SIZE
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113915/#review43921 --- No. It is not acceptable to set the maximum xfer buffer size to such a large value. See http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx - Dawit Alemayehu On Nov. 17, 2013, 8:33 p.m., Federico Cuello wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113915/ --- (Updated Nov. 17, 2013, 8:33 p.m.) Review request for KDE Runtime. Bugs: 291835 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- Fixes #291835 Several reports show that a buffer size increase greatly improves transfer speed. Diffs - kioslave/smb/kio_smb.h 55efb44 Diff: http://git.reviewboard.kde.org/r/113915/diff/ Testing --- Thanks, Federico Cuello
Re: Review Request 112982: copy[To|From]File support for kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Nov. 15, 2013, 11:59 a.m.) Review request for KDE Runtime and David Faure. Changes --- Updated patch based on feedback and testing. Not sure what to do about the fact that this patch requires new API available only in kdelibs = 4.11.80. Should I change the required package in the CMakeLists.txt file or #ifdef the code? Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- The attach patch adds support for the following to kio_smb: - copyToFile/copyFromFile optimizations so uploading and downloading files from window shares is faster so long as one end is a local file. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Diffs (updated) - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 186b687 kioslave/smb/kio_smb_internal.h b895b81 kioslave/smb/kio_smb_internal.cpp 3c35583 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing (updated) --- - Copied files to and from smb share. - Resumed interrupted file copy to and from smb share. Thanks, Dawit Alemayehu
Re: Review Request 112982: copy[To|From]File support for kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Nov. 14, 2013, 2:15 p.m.) Review request for KDE Runtime and David Faure. Changes --- Updated patch with both copyFrom and copyTo optimizations implemented. Summary (updated) - copy[To|From]File support for kio_smb Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description (updated) --- The attach patch adds support for the following to kio_smb: - copyToFile/copyFromFile optimizations so uploading and downloading files from window shares is faster so long as one end is a local file. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Diffs (updated) - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 186b687 kioslave/smb/kio_smb_internal.h b895b81 kioslave/smb/kio_smb_internal.cpp 3c35583 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 113785: Fix Mozilla bookmark importing
On Nov. 10, 2013, 3:51 p.m., David Faure wrote: kio/bookmarks/kbookmarkimporter_ns.cc, line 65 http://git.reviewboard.kde.org/r/113785/diff/1/?file=212403#file212403line65 (could be more optimized with startsWith(HR, Qt::CaseInsensitive) -- but ok, the rest of the code uses toUpper, so this is ok for this commit) No because t is a QByteArray and its startsWith method does not have a Qt::CaseSensitvity parameter like QString. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113785/#review43352 --- On Nov. 10, 2013, 2:51 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113785/ --- (Updated Nov. 10, 2013, 2:51 p.m.) Review request for kdelibs and David Faure. Bugs: 292495 http://bugs.kde.org/show_bug.cgi?id=292495 Repository: kdelibs Description --- Currently the Mozilla bookmark importing code simply ignores any HTML tags preceded by the HR element resulting in lines like HRDTH3 ADD_DATE=1364760832 LAST_MODIFIED=1364760921magie/H3 simply being treated as a separator instead of a separator followed by a bookmark entry. The attached patch addresses this problem. Diffs - kio/bookmarks/kbookmarkimporter_ns.cc d3f0eaa Diff: http://git.reviewboard.kde.org/r/113785/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 113785: Fix Mozilla bookmark importing
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113785/ --- (Updated Nov. 10, 2013, 5 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Bugs: 292495 http://bugs.kde.org/show_bug.cgi?id=292495 Repository: kdelibs Description --- Currently the Mozilla bookmark importing code simply ignores any HTML tags preceded by the HR element resulting in lines like HRDTH3 ADD_DATE=1364760832 LAST_MODIFIED=1364760921magie/H3 simply being treated as a separator instead of a separator followed by a bookmark entry. The attached patch addresses this problem. Diffs - kio/bookmarks/kbookmarkimporter_ns.cc d3f0eaa Diff: http://git.reviewboard.kde.org/r/113785/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 113492: kio_http: set the application level proxy to NoProxy before making a direct connection to host (server or proxy)
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113492/#review42624
---
kioslave/http/http.cpp
http://git.reviewboard.kde.org/r/113492/#comment30882
This actually needs to be a dbus call to the proxyscout kded module.
Please remove the TODO in the badProxyUrls if block and add the following code
instead:
QDBusInterface proxyScout(QLatin1String(org.kde.kded),
QLatin1String(/modules/proxyscout), QLatin1String(org.kde.KPAC.ProxyScout));
if (proxyScout.isValid()) {
Q_FOREACH(const KUrl url, badProxyUrls) {
(void)proxyScout.asyncCall(QLatin1String(blackListProxy), url.url());
}
}
- Dawit Alemayehu
On Oct. 28, 2013, 9:52 p.m., Andrea Iacovitti wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113492/
---
(Updated Oct. 28, 2013, 9:52 p.m.)
Review request for kdelibs and Dawit Alemayehu.
Bugs: 326799
http://bugs.kde.org/show_bug.cgi?id=326799
Repository: kdelibs
Description
---
Make sure application level proxy is set to QNetworkProxy::NoProxy before
connecting to remote host when no Qt application proxy is required, that is
when not using socks proxy or tunneling SSL.
Also warn the user about collected bad proxies url, if any.
Diffs
-
kioslave/http/http.cpp 417569a
Diff: http://git.reviewboard.kde.org/r/113492/diff/
Testing
---
Thanks,
Andrea Iacovitti
Re: Review Request 113492: kio_http: set the application level proxy to NoProxy before making a direct connection to host (server or proxy)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113492/#review42672 --- Ship it! Ship It! - Dawit Alemayehu On Oct. 29, 2013, 5:40 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113492/ --- (Updated Oct. 29, 2013, 5:40 p.m.) Review request for kdelibs and Dawit Alemayehu. Bugs: 326799 http://bugs.kde.org/show_bug.cgi?id=326799 Repository: kdelibs Description --- Make sure application level proxy is set to QNetworkProxy::NoProxy before connecting to remote host when no Qt application proxy is required, that is when not using socks proxy or tunneling SSL. Also warn the user about collected bad proxies url, if any. Diffs - kioslave/http/http.cpp 417569a Diff: http://git.reviewboard.kde.org/r/113492/diff/ Testing --- Thanks, Andrea Iacovitti
Re: Review Request 112982: copyToFile support for kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Oct. 25, 2013, 1:10 p.m.) Review request for KDE Runtime. Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- The attach patch adds support for the following to kio_smb: - copyToFile optimization so downloading files from window shares is faster. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Note that in this patch the latter two features only apply to smb - file downloads. The second part of this patch that will add support for the other half, the copyFromFile optimization. Diffs - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 113364: kio_http: fix for proxy connection
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113364/#review42328 --- I have no objection to this patch. However, I want to make sure you have tested the other modes before I give it a thumbs up. Did you try the manual proxy setup with your patch? Also adding some exceptions to make sure it honors those as well? If not can you please do that I do not have the bandwidth to test that at the moment. - Dawit Alemayehu On Oct. 21, 2013, 1:41 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113364/ --- (Updated Oct. 21, 2013, 1:41 p.m.) Review request for kdelibs and Dawit Alemayehu. Repository: kdelibs Description --- Fix the code to determine if a proxy or direct connection should be made in the case where multiple proxies are availables and between them the keyword DIRECT is present. We should first verify if the candidate proxy string is equal to DIRECT (and then try to make a direct connection) and after try to construct KUrl object. Otherwise we would never be able to direct connect because KUrl(DIRECT) is not a valid (proxy) url. Diffs - kioslave/http/http.cpp 81182eb Diff: http://git.reviewboard.kde.org/r/113364/diff/ Testing --- Tested against various PAC scripts returning, for example, proxies list like: (http://127.0.0.1:;, DIRECT) or (DIRECT, http://127.0.0.1:;) Note: in case PAC script returns DIRECT only, this code path is not executed as m_request.proxyUrls will be empty. Thanks, Andrea Iacovitti
Re: Review Request 112982: copyToFile support for kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Oct. 23, 2013, 1:21 p.m.) Review request for KDE Runtime. Changes --- Updated patch based on feedback. Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description (updated) --- The attach patch adds support for the following to kio_smb: - copyToFile optimization so downloading files from window shares is faster. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Note that in this patch the latter two features only apply to smb - file downloads. The second part of this patch that will add support for the other half, the copyFromFile optimization. Diffs (updated) - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112982: copyToFile support for kio_smb
On Oct. 22, 2013, 8:08 a.m., David Faure wrote: kioslave/smb/kio_smb_dir.cpp, line 45 http://git.reviewboard.kde.org/r/112982/diff/2/?file=193087#file193087line45 this TODO can be removed now, right? Yes it can, but only as soon as I implement the other half, copyFromFile. On Oct. 22, 2013, 8:08 a.m., David Faure wrote: kioslave/smb/kio_smb_dir.cpp, line 285 http://git.reviewboard.kde.org/r/112982/diff/2/?file=193087#file193087line285 I'm pretty sure this cast is wrong. QFile::Permissions doesn't map to mode_t, at least not with a simple cast. Either do like kio_ftp does (KDE::open() instead of QFile), or add a call to chmod() at the end of the operation, or use a function to convert from mode_t to QFile::Permissions (maybe we want to have that in KFileItem, in fact). I prefer solution 2 or 3 above solution 1, so that QFile can be used as much as possible. Well you are probably right that it is a bad idea to do the cast here since everything does not perfectly map from mode_t to QFile::Permissions. However, at least the rwx permissions match exactly: S_IRUSR 00400 owner has read permission S_IWUSR 00200 owner has write permission S_IXUSR 00100 owner has execute permission S_IRGRP 00040 group has read permission S_IWGRP 00020 group has write permission S_IXGRP 00010 group has execute permission S_IROTH 4 others have read permission S_IWOTH 2 others have write permission S_IXOTH 1 others have execute permission QFile::ReadUser0x0400 The file is readable by the user. QFile::WriteUser 0x0200The file is writable by the user. QFile::ExeUser 0x0100 The file is executable by the user. QFile::ReadGroup 0x0040The file is readable by the group. QFile::WriteGroup0x0020 The file is writable by the group. QFile::ExeGroup0x0010 The file is executable by the group. QFile::ReadOther 0x0004The file is readable by anyone. QFile::WriteOther0x0002 The file is writable by anyone. QFile::ExeOther0x0001 The file is executable by anyone. Anyhow, I will see what I can do about finding a solution for this. However, I am confused why you would want it in KFileItem since that class is not even used in this particular code base. Should not something like that be added in a more convenience place? Perhaps kio/global.*? - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/#review42150 --- On Oct. 5, 2013, 3:07 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Oct. 5, 2013, 3:07 p.m.) Review request for KDE Runtime. Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- The attach patch adds support for the following to kio_smb: - copyToFile optimization so downloading files from window shares is faster. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Note that in this patch the latter two features only apply to smb - file downloads. The second part of this patch will that will follow soon will add support for the other half, the copyFromFile optimization. Diffs - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 113366: kio_http: small optimization in response code parsing
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113366/#review42146 --- Ship it! Ship It! - Dawit Alemayehu On Oct. 21, 2013, 3:23 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113366/ --- (Updated Oct. 21, 2013, 3:23 p.m.) Review request for kdelibs and Dawit Alemayehu. Repository: kdelibs Description --- Execute the long if...else if... only if the previous condition if (m_request.responseCode != 200 m_request.responseCode != 304) is verified. If it fails (that is responseCode == 200 or responseCode == 304) none of the nine conditions in the subsequent if..else if.. can be true. Diffs - kioslave/http/http.cpp 81182eb Diff: http://git.reviewboard.kde.org/r/113366/diff/ Testing --- Thanks, Andrea Iacovitti
Re: Review Request 113131: Added missing slot to bookmarksrunner.*
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113131/ --- (Updated Oct. 12, 2013, 5:34 p.m.) Status -- This change has been marked as submitted. Review request for kde-workspace. Repository: kde-workspace Description --- This trivial patch adds a missing slot, teardown, to bookmarksrunner.h to stop the following warning message in the logs: Object::connect: No such slot KDEBrowser::teardown() Object::connect: (sender name: 'Bookmarks') Diffs - plasma/generic/runners/bookmarks/browsers/kdebrowser.h 3302e41 plasma/generic/runners/bookmarks/bookmarksrunner.cpp b36ad7f Diff: http://git.reviewboard.kde.org/r/113131/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 113132: Move all Dolphin KPart extensions into dolphin_ext.*
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113132/ --- (Updated Oct. 12, 2013, 5:36 p.m.) Status -- This change has been marked as submitted. Review request for KDE Base Apps and Frank Reininghaus. Repository: kde-baseapps Description --- This cosmetic patch moves all the Dolphin part extension related code into dolphinpart_ext.* Diffs - dolphin/src/dolphinpart.h c70bc5a dolphin/src/dolphinpart.cpp 6609735 dolphin/src/dolphinpart_ext.h 423e79e dolphin/src/dolphinpart_ext.cpp e98c064 Diff: http://git.reviewboard.kde.org/r/113132/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 113129: proxy configuration: fix setting the use of proxy environment variable
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113129/#review41298 --- Ship it! Thanks for the fix. - Dawit Alemayehu On Oct. 6, 2013, 12:36 p.m., Andrea Iacovitti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113129/ --- (Updated Oct. 6, 2013, 12:36 p.m.) Review request for KDE Base Apps and Dawit Alemayehu. Bugs: 304707 http://bugs.kde.org/show_bug.cgi?id=304707 Repository: kde-baseapps Description --- - Use the correct variable, instead of the entire QStringList, to fill in lineedit's text in autoDetectSystemProxy. - When saving for System proxy configuration, in KProxyDialog::save(), be sure to write the name of environment variables and not their values in the configuration file. - Added missing signal connection for mUi.systemProxyRadioButton. - Connect slotChanged() to textEdited() signal (instead of textChanged()) for systemProxy's lineedits, this avoid to emit changed() when Show the value of the envirnment variables checkbox is clicked. NOTE: Currently (and from kde3 time) the proxy configuration keys in kioslaverc: httpProxy,httpsProxy,ftpProxy,socksProxy,NoProxyFor, are used with a double semantic. They hold the proxy address in case of Manual Proxy Configuration or the name of environment variables to use in case of System Proxy Configuration. (See getSystemProxyFor() in kprotocolmanager.cpp) Diffs - konqueror/settings/kio/kproxydlg.cpp 7c777d3 Diff: http://git.reviewboard.kde.org/r/113129/diff/ Testing --- Thanks, Andrea Iacovitti
Review Request 113131: Added missing slot to bookmarksrunner.*
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113131/ --- Review request for kde-workspace. Repository: kde-workspace Description --- This trivial patch adds a missing slot, teardown, to bookmarksrunner.h to stop the following warning message in the logs: Object::connect: No such slot KDEBrowser::teardown() Object::connect: (sender name: 'Bookmarks') Diffs - plasma/generic/runners/bookmarks/browsers/kdebrowser.h 3302e41 plasma/generic/runners/bookmarks/bookmarksrunner.cpp b36ad7f Diff: http://git.reviewboard.kde.org/r/113131/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 113132: Move all Dolphin KPart extensions into dolphin_ext.*
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113132/ --- Review request for KDE Base Apps and Frank Reininghaus. Repository: kde-baseapps Description --- This cosmetic patch moves all the Dolphin part extension related code into dolphinpart_ext.* Diffs - dolphin/src/dolphinpart.h c70bc5a dolphin/src/dolphinpart.cpp 6609735 dolphin/src/dolphinpart_ext.h 423e79e dolphin/src/dolphinpart_ext.cpp e98c064 Diff: http://git.reviewboard.kde.org/r/113132/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112982: copyToFile support for kio_smb
On Oct. 5, 2013, 12:12 p.m., Mark Gaiser wrote: Hi Dawit, Sorry for the late reply. I will test this patch out and report my findings later today. I guess this is the long awaited patch that improves the file copy speed from SMB to local. Yes, amongst the other fixes I outlined above. Please note that this patch is currently only for one direction: smb - file. I am working on the other half: file - smb right now. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/#review41273 --- On Sept. 29, 2013, 4:10 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Sept. 29, 2013, 4:10 p.m.) Review request for KDE Runtime. Bugs: 291835 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- The attach patch adds support for the following to kio_smb: - copyToFile optimization so downloading files from window shares is faster. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Note that in this patch the latter two features only apply to smb - file downloads. The second part of this patch will that will follow soon will add support for the other half, the copyFromFile optimization. Diffs - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112982: copyToFile support for kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Oct. 5, 2013, 3:07 p.m.) Review request for KDE Runtime. Changes --- Updated bug list. Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- The attach patch adds support for the following to kio_smb: - copyToFile optimization so downloading files from window shares is faster. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Note that in this patch the latter two features only apply to smb - file downloads. The second part of this patch will that will follow soon will add support for the other half, the copyFromFile optimization. Diffs - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112982: copyToFile support for kio_smb
On Oct. 5, 2013, 4:01 p.m., Mark Gaiser wrote: Tested it. PRE patch: ~17MB/s POST patch: ~27MB/s So in functionality terms this patch makes a file copy from a windows share (note: a linux machine sharing through samba, not an actual windows machine) much faster. However, mounting the same share through CIFS (mount.cifs) gives mu much greater speed with ~86MB/s. Is there anything you can do to make this about equal in speed compared to cifs? Can you provide the mount options you used? We use libsmbclient for so there should not be such a large difference. I will have to check if there are known performance issues with libsmbclient itself. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/#review41277 --- On Oct. 5, 2013, 3:07 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Oct. 5, 2013, 3:07 p.m.) Review request for KDE Runtime. Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- The attach patch adds support for the following to kio_smb: - copyToFile optimization so downloading files from window shares is faster. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Note that in this patch the latter two features only apply to smb - file downloads. The second part of this patch will that will follow soon will add support for the other half, the copyFromFile optimization. Diffs - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112982: copyToFile support for kio_smb
On Oct. 5, 2013, 4:01 p.m., Mark Gaiser wrote: Tested it. PRE patch: ~17MB/s POST patch: ~27MB/s So in functionality terms this patch makes a file copy from a windows share (note: a linux machine sharing through samba, not an actual windows machine) much faster. However, mounting the same share through CIFS (mount.cifs) gives mu much greater speed with ~86MB/s. Is there anything you can do to make this about equal in speed compared to cifs? Dawit Alemayehu wrote: Can you provide the mount options you used? We use libsmbclient for so there should not be such a large difference. I will have to check if there are known performance issues with libsmbclient itself. Mark Gaiser wrote: Hi Dawit, I did't provide any mount options besides my username. The actual line: mount.cifs //ip data -o username=mark Yes, that's all :) I found this gnome vfs change: https://mail.gnome.org/archives/commits-list/2009-August/msg06304.html. It also checked to see if that code remains the same today and it seems like it does. See http://code.ohloh.net/file?fid=ZGtFwvN9JyhO9xIWTTQS-H6jau8cid=Wya2WEWTTh4s=gvfsbackendsmb.cpp=0ff=1filterChecked=truefp=309479mp=1ml=1me=1md=1projSelected=true#L0 Do you get comparable speed if you change MAX_XFER_BUF_SIZE definition in kio_smb.h to 65534? - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/#review41277 --- On Oct. 5, 2013, 3:07 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Oct. 5, 2013, 3:07 p.m.) Review request for KDE Runtime. Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- The attach patch adds support for the following to kio_smb: - copyToFile optimization so downloading files from window shares is faster. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Note that in this patch the latter two features only apply to smb - file downloads. The second part of this patch will that will follow soon will add support for the other half, the copyFromFile optimization. Diffs - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112982: copyToFile support for kio_smb
On Oct. 5, 2013, 4:01 p.m., Mark Gaiser wrote: Tested it. PRE patch: ~17MB/s POST patch: ~27MB/s So in functionality terms this patch makes a file copy from a windows share (note: a linux machine sharing through samba, not an actual windows machine) much faster. However, mounting the same share through CIFS (mount.cifs) gives mu much greater speed with ~86MB/s. Is there anything you can do to make this about equal in speed compared to cifs? Dawit Alemayehu wrote: Can you provide the mount options you used? We use libsmbclient for so there should not be such a large difference. I will have to check if there are known performance issues with libsmbclient itself. Mark Gaiser wrote: Hi Dawit, I did't provide any mount options besides my username. The actual line: mount.cifs //ip data -o username=mark Yes, that's all :) Dawit Alemayehu wrote: I found this gnome vfs change: https://mail.gnome.org/archives/commits-list/2009-August/msg06304.html. It also checked to see if that code remains the same today and it seems like it does. See http://code.ohloh.net/file?fid=ZGtFwvN9JyhO9xIWTTQS-H6jau8cid=Wya2WEWTTh4s=gvfsbackendsmb.cpp=0ff=1filterChecked=truefp=309479mp=1ml=1me=1md=1projSelected=true#L0 Do you get comparable speed if you change MAX_XFER_BUF_SIZE definition in kio_smb.h to 65534? Mark Gaiser wrote: Well, your patch changed it to 65536. I doubt decreasing it to 65534 will magically make it equally fast as mount.cifs. My POST patch result is with MAX_XFER_BUF_SIZE set to 65536. One thing i do notice in searching for CIFS is that it's advertised as the SMB successor. Perhaps it's just better and faster? No. It is the same thing. CIFS is just a new name for the same old SMB protocol in its modern incarnation. If you are testing it against a Windows share and not a Samba share, then that might make a difference. You can also try to double the buffer to 128 KB and see if that makes any difference. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/#review41277 --- On Oct. 5, 2013, 3:07 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Oct. 5, 2013, 3:07 p.m.) Review request for KDE Runtime. Bugs: 176271 and 291835 http://bugs.kde.org/show_bug.cgi?id=176271 http://bugs.kde.org/show_bug.cgi?id=291835 Repository: kde-runtime Description --- The attach patch adds support for the following to kio_smb: - copyToFile optimization so downloading files from window shares is faster. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Note that in this patch the latter two features only apply to smb - file downloads. The second part of this patch will that will follow soon will add support for the other half, the copyFromFile optimization. Diffs - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112982: copyToFile support for kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- (Updated Sept. 29, 2013, 4:10 p.m.) Review request for KDE Runtime. Changes --- Updated patch Description --- The attach patch adds support for the following to kio_smb: - copyToFile optimization so downloading files from window shares is faster. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Note that in this patch the latter two features only apply to smb - file downloads. The second part of this patch will that will follow soon will add support for the other half, the copyFromFile optimization. This addresses bug 291835. http://bugs.kde.org/show_bug.cgi?id=291835 Diffs (updated) - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 112982: copyToFile support for kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/ --- Review request for KDE Runtime. Description --- The attach patch adds support for the following to kio_smb: - copyToFile optimization so downloading files from window shares is faster. - partial download resumption as part of the copyToFile implementation. - preservation of modified file timstamp. Again as part of the copyToFile implementation. Note that in this patch the latter two features only apply to smb - file downloads. The second part of this patch will that will follow soon will add support for the other half, the copyFromFile optimization. This addresses bug 291835. http://bugs.kde.org/show_bug.cgi?id=291835 Diffs - kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/smb.protocol 654bcfb Diff: http://git.reviewboard.kde.org/r/112982/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112463: Port SMB kioslave to KF5/Qt5
On Sept. 6, 2013, 1:10 p.m., Dawit Alemayehu wrote: I have several changes coming to this ioslave. Mainly I am going to implement the copyFrom and copyTo optimizations like I did for the sftp ioslave and add support for upload/download resumptions. The changes are going to come in parts because they are not small and should be done in the next week or two. If you feel that it would be easier to merge those changes after your port, then feel free to ship this, otherwise would it be possible for you to hold off on this port? For the record I am not the maintainer of this code base. I am just trying to improve as many of these ioslaves as I can when I get the chance. Mark Gaiser wrote: Question though: did you test this port and did it work for you? As for the porting and your changes. You seem to be making those changes in KDE/4.11, not in frameworks-scratch, so i would prefer if you make the changes you want to make. Once those are in i will have to rebase my changes against it and request a ship it again :) I cannot test it because I am not setup for KF5. I just went through the code. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/#review39463 --- On Sept. 2, 2013, 7:16 p.m., Mark Gaiser wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/ --- (Updated Sept. 2, 2013, 7:16 p.m.) Review request for KDE Runtime and KDE Frameworks. Description --- This is the initial port! I added two TODO lines in the diff for parts where i'm not sure if I've ported them correctly. Also, i needed a change in FindSamba.cmake to even get the samba detection working. That reviewrequest is waiting here: https://git.reviewboard.kde.org/r/112448/ you're probably OK if you still use samba 3.x Once i know that this is actually working then i will comment some qDebug lines. Diffs - kioslave/CMakeLists.txt ff66ab6 kioslave/smb/CMakeLists.txt a3a2265 kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb.cpp 2c2523a kioslave/smb/kio_smb_auth.cpp 4d236b4 kioslave/smb/kio_smb_browse.cpp fec6449 kioslave/smb/kio_smb_config.cpp 81ce29c kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/kio_smb_file.cpp 827a519 kioslave/smb/kio_smb_internal.h b895b81 kioslave/smb/kio_smb_internal.cpp 3c35583 kioslave/smb/kio_smb_mount.cpp a5a7e8e Diff: http://git.reviewboard.kde.org/r/112463/diff/ Testing --- It compiles and gets loaded just fine. I tried testing this on an actual samba share, but i kept getting a 111 error (connection refused) from kio_smb so i'm hoping that is a local issue here. If someone else could try this out and verify that it's either working or broken. Thanks, Mark Gaiser ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 112463: Port SMB kioslave to KF5/Qt5
On Sept. 6, 2013, 1:10 p.m., Dawit Alemayehu wrote: I have several changes coming to this ioslave. Mainly I am going to implement the copyFrom and copyTo optimizations like I did for the sftp ioslave and add support for upload/download resumptions. The changes are going to come in parts because they are not small and should be done in the next week or two. If you feel that it would be easier to merge those changes after your port, then feel free to ship this, otherwise would it be possible for you to hold off on this port? For the record I am not the maintainer of this code base. I am just trying to improve as many of these ioslaves as I can when I get the chance. Mark Gaiser wrote: Question though: did you test this port and did it work for you? As for the porting and your changes. You seem to be making those changes in KDE/4.11, not in frameworks-scratch, so i would prefer if you make the changes you want to make. Once those are in i will have to rebase my changes against it and request a ship it again :) I cannot test it because I am not setup for KF5. I just went through the code. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/#review39463 --- On Sept. 2, 2013, 7:16 p.m., Mark Gaiser wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/ --- (Updated Sept. 2, 2013, 7:16 p.m.) Review request for KDE Runtime and KDE Frameworks. Description --- This is the initial port! I added two TODO lines in the diff for parts where i'm not sure if I've ported them correctly. Also, i needed a change in FindSamba.cmake to even get the samba detection working. That reviewrequest is waiting here: https://git.reviewboard.kde.org/r/112448/ you're probably OK if you still use samba 3.x Once i know that this is actually working then i will comment some qDebug lines. Diffs - kioslave/CMakeLists.txt ff66ab6 kioslave/smb/CMakeLists.txt a3a2265 kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb.cpp 2c2523a kioslave/smb/kio_smb_auth.cpp 4d236b4 kioslave/smb/kio_smb_browse.cpp fec6449 kioslave/smb/kio_smb_config.cpp 81ce29c kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/kio_smb_file.cpp 827a519 kioslave/smb/kio_smb_internal.h b895b81 kioslave/smb/kio_smb_internal.cpp 3c35583 kioslave/smb/kio_smb_mount.cpp a5a7e8e Diff: http://git.reviewboard.kde.org/r/112463/diff/ Testing --- It compiles and gets loaded just fine. I tried testing this on an actual samba share, but i kept getting a 111 error (connection refused) from kio_smb so i'm hoping that is a local issue here. If someone else could try this out and verify that it's either working or broken. Thanks, Mark Gaiser
Re: Review Request 112463: Port SMB kioslave to KF5/Qt5
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/#review39463 --- I have several changes coming to this ioslave. Mainly I am going to implement the copyFrom and copyTo optimizations like I did for the sftp ioslave and add support for upload/download resumptions. The changes are going to come in parts because they are not small and should be done in the next week or two. If you feel that it would be easier to merge those changes after your port, then feel free to ship this, otherwise would it be possible for you to hold off on this port? For the record I am not the maintainer of this code base. I am just trying to improve as many of these ioslaves as I can when I get the chance. - Dawit Alemayehu On Sept. 2, 2013, 7:16 p.m., Mark Gaiser wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/ --- (Updated Sept. 2, 2013, 7:16 p.m.) Review request for KDE Runtime and KDE Frameworks. Description --- This is the initial port! I added two TODO lines in the diff for parts where i'm not sure if I've ported them correctly. Also, i needed a change in FindSamba.cmake to even get the samba detection working. That reviewrequest is waiting here: https://git.reviewboard.kde.org/r/112448/ you're probably OK if you still use samba 3.x Once i know that this is actually working then i will comment some qDebug lines. Diffs - kioslave/CMakeLists.txt ff66ab6 kioslave/smb/CMakeLists.txt a3a2265 kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb.cpp 2c2523a kioslave/smb/kio_smb_auth.cpp 4d236b4 kioslave/smb/kio_smb_browse.cpp fec6449 kioslave/smb/kio_smb_config.cpp 81ce29c kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/kio_smb_file.cpp 827a519 kioslave/smb/kio_smb_internal.h b895b81 kioslave/smb/kio_smb_internal.cpp 3c35583 kioslave/smb/kio_smb_mount.cpp a5a7e8e Diff: http://git.reviewboard.kde.org/r/112463/diff/ Testing --- It compiles and gets loaded just fine. I tried testing this on an actual samba share, but i kept getting a 111 error (connection refused) from kio_smb so i'm hoping that is a local issue here. If someone else could try this out and verify that it's either working or broken. Thanks, Mark Gaiser ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 112529: By default hide SMB shares that end with $
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112529/ --- (Updated Sept. 6, 2013, 12:35 p.m.) Review request for KDE Runtime. Changes --- Updated patch. Description --- The attached patch marks all files, directories, shares etc as hidden by default. That what the GUI applications can correctly hide them and only make them visible when the Show Hidden Files option is checked. This addresses bug 197903. http://bugs.kde.org/show_bug.cgi?id=197903 Diffs (updated) - kioslave/smb/kio_smb_browse.cpp fec6449 Diff: http://git.reviewboard.kde.org/r/112529/diff/ Testing (updated) --- Tested against shares from a Windows 7 machine. Thanks, Dawit Alemayehu
Re: Review Request 112529: By default hide SMB shares that end with $
On Sept. 6, 2013, 11:04 a.m., Mark Gaiser wrote: kioslave/smb/kio_smb_browse.cpp, lines 348-354 http://git.reviewboard.kde.org/r/112529/diff/1/?file=187202#file187202line348 This - while most was there already - seems odd to me. Your other code catches IPC$, ADMIN$, printer$ and print$ (because of the $) so i think you can trim this down to just . and .. and then just break out of the current loop interation. All of them were there already. I did not add any new ones. I simply moved them around. Anyhow, I completely misread what Windows administrative shares meant. I thought the drive letters that ended with $ were different. That is why I kept the existing code and did the patch the way I did it. Upon a second review all so called hidden shares, those that end with $, are administrative shares ; so I am going to change this to do exactly as you suggested above. On Sept. 6, 2013, 11:04 a.m., Dawit Alemayehu wrote: I don't know much in the samba department, but just happened to touch the exact same code a few days ago when porting it to Qt5/KF5 - which is still waiting for someone to review it btw ;) I saw that. I was going to comment on it but I forgot. I will do that now. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112529/#review39456 --- On Sept. 6, 2013, 12:35 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112529/ --- (Updated Sept. 6, 2013, 12:35 p.m.) Review request for KDE Runtime. Description --- The attached patch marks all files, directories, shares etc as hidden by default. That what the GUI applications can correctly hide them and only make them visible when the Show Hidden Files option is checked. This addresses bug 197903. http://bugs.kde.org/show_bug.cgi?id=197903 Diffs - kioslave/smb/kio_smb_browse.cpp fec6449 Diff: http://git.reviewboard.kde.org/r/112529/diff/ Testing --- Tested against shares from a Windows 7 machine. Thanks, Dawit Alemayehu
Re: Review Request 112529: By default hide SMB shares that end with $
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112529/ --- (Updated Sept. 6, 2013, 1:04 p.m.) Review request for KDE Runtime. Changes --- Updated patch Description --- The attached patch marks all files, directories, shares etc as hidden by default. That what the GUI applications can correctly hide them and only make them visible when the Show Hidden Files option is checked. This addresses bug 197903. http://bugs.kde.org/show_bug.cgi?id=197903 Diffs (updated) - kioslave/smb/kio_smb_browse.cpp fec6449 Diff: http://git.reviewboard.kde.org/r/112529/diff/ Testing --- Tested against shares from a Windows 7 machine. Thanks, Dawit Alemayehu
Re: Review Request 112463: Port SMB kioslave to KF5/Qt5
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/#review39463 --- I have several changes coming to this ioslave. Mainly I am going to implement the copyFrom and copyTo optimizations like I did for the sftp ioslave and add support for upload/download resumptions. The changes are going to come in parts because they are not small and should be done in the next week or two. If you feel that it would be easier to merge those changes after your port, then feel free to ship this, otherwise would it be possible for you to hold off on this port? For the record I am not the maintainer of this code base. I am just trying to improve as many of these ioslaves as I can when I get the chance. - Dawit Alemayehu On Sept. 2, 2013, 7:16 p.m., Mark Gaiser wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112463/ --- (Updated Sept. 2, 2013, 7:16 p.m.) Review request for KDE Runtime and KDE Frameworks. Description --- This is the initial port! I added two TODO lines in the diff for parts where i'm not sure if I've ported them correctly. Also, i needed a change in FindSamba.cmake to even get the samba detection working. That reviewrequest is waiting here: https://git.reviewboard.kde.org/r/112448/ you're probably OK if you still use samba 3.x Once i know that this is actually working then i will comment some qDebug lines. Diffs - kioslave/CMakeLists.txt ff66ab6 kioslave/smb/CMakeLists.txt a3a2265 kioslave/smb/kio_smb.h 55efb44 kioslave/smb/kio_smb.cpp 2c2523a kioslave/smb/kio_smb_auth.cpp 4d236b4 kioslave/smb/kio_smb_browse.cpp fec6449 kioslave/smb/kio_smb_config.cpp 81ce29c kioslave/smb/kio_smb_dir.cpp 5573266 kioslave/smb/kio_smb_file.cpp 827a519 kioslave/smb/kio_smb_internal.h b895b81 kioslave/smb/kio_smb_internal.cpp 3c35583 kioslave/smb/kio_smb_mount.cpp a5a7e8e Diff: http://git.reviewboard.kde.org/r/112463/diff/ Testing --- It compiles and gets loaded just fine. I tried testing this on an actual samba share, but i kept getting a 111 error (connection refused) from kio_smb so i'm hoping that is a local issue here. If someone else could try this out and verify that it's either working or broken. Thanks, Mark Gaiser
Re: Review Request 112529: By default hide SMB shares that end with $
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112529/ --- (Updated Sept. 5, 2013, 1:32 p.m.) Review request for KDE Runtime. Changes --- Uploaded patch Description --- The attached patch marks all files, directories, shares etc as hidden by default. That what the GUI applications can correctly hide them and only make them visible when the Show Hidden Files option is checked. This addresses bug 197903. http://bugs.kde.org/show_bug.cgi?id=197903 Diffs (updated) - kioslave/smb/kio_smb_browse.cpp fec6449 Diff: http://git.reviewboard.kde.org/r/112529/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 112528: Make it possible to preserve mtime for copy jobs not just move ones
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112528/ --- Review request for kdelibs and David Faure. Description --- This patch changes the current logic outlined at https://bugs.kde.org/show_bug.cgi?id=55804#c48 such that the copy/paste operations will also preserve the mtime. I have no idea why I chose to only not to send the modified meta-data for COPY requests. There really is no good reason for it and it is inconsistent with what kio_file does for local file copy operations. This addresses bug 55804. http://bugs.kde.org/show_bug.cgi?id=55804 Diffs - kio/kio/job.cpp 8ff088c Diff: http://git.reviewboard.kde.org/r/112528/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 112529: By default hide SMB shares that end with $
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112529/ --- Review request for KDE Runtime. Description --- The attached patch marks all files, directories, shares etc as hidden by default. That what the GUI applications can correctly hide them and only make them visible when the Show Hidden Files option is checked. This addresses bug 197903. http://bugs.kde.org/show_bug.cgi?id=197903 Diffs - Diff: http://git.reviewboard.kde.org/r/112529/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 112467: Use cached login information when authenticating in kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112467/ --- Review request for KDE Runtime. Description --- Use checkCachedAuthentication to check for and use cached authentication before prompting the user for password in kio_smb. This addresses bug 314571. http://bugs.kde.org/show_bug.cgi?id=314571 Diffs - kioslave/smb/kio_smb_auth.cpp 4d236b4 Diff: http://git.reviewboard.kde.org/r/112467/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112467: Use cached login information when authenticating in kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112467/ --- (Updated Sept. 2, 2013, 8:59 p.m.) Review request for KDE Runtime. Changes --- Updated bugs list. Description --- Use checkCachedAuthentication to check for and use cached authentication before prompting the user for password in kio_smb. This addresses bugs 299605 and 314571. http://bugs.kde.org/show_bug.cgi?id=299605 http://bugs.kde.org/show_bug.cgi?id=314571 Diffs - kioslave/smb/kio_smb_auth.cpp 4d236b4 Diff: http://git.reviewboard.kde.org/r/112467/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112467: Use cached login information when authenticating in kio_smb
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112467/ --- (Updated Sept. 2, 2013, 9:14 p.m.) Status -- This change has been discarded. Review request for KDE Runtime. Description --- Use checkCachedAuthentication to check for and use cached authentication before prompting the user for password in kio_smb. This addresses bugs 299605 and 314571. http://bugs.kde.org/show_bug.cgi?id=299605 http://bugs.kde.org/show_bug.cgi?id=314571 Diffs - kioslave/smb/kio_smb_auth.cpp 4d236b4 Diff: http://git.reviewboard.kde.org/r/112467/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 112173: Do not use QFileInfo to obtain the size of a symlink in kio_trash
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112173/ --- Review request for KDE Runtime, David Faure and Tobias Koenig. Description --- The attached patch fixes a bug where deleting a symlink to a very large file causes a trash has reached its limit error. This happens because the code that is used to determine the amount of space available in the trashcan uses QFileInfo::size to determine the size of a file. This will not work because QFileInfo::size returns the size of the actual file the symlink points to and not the size of the symlink itself. See the documentation for QFileInfo. This addresses bug 253776. http://bugs.kde.org/show_bug.cgi?id=253776 Diffs - kioslave/trash/discspaceutil.cpp 8e76ece Diff: http://git.reviewboard.kde.org/r/112173/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 112173: Do not use QFileInfo to obtain the size of a symlink in kio_trash
On Aug. 20, 2013, 1:29 p.m., David Faure wrote: QFileInfo is more portable. Why not just write || info.isSymlink() in the first if? I know QFileInfo is more portable. I use it exclusively whenever I can, but the logic it uses to calculate the size of a symlink will not work for this use case ; so I fail to see how simply adding || info.isSymlink() to the first if statement solves this problem. That is why I said see the documentation for QFileInfo. Simply put info.size() returns the size of the actual file the symlink points to instead of the size of the symlink itself. You do not want that in this case because it will trigger an incorrect error message. There is a very simple test for this. Create a symlink to a file that is larger than the size of your trashcan. Make sure your trashcan is empty. Now delete the symlink by clicking on Move to Trash in Dolphin and Konqueror and see what you get. On Aug. 20, 2013, 1:29 p.m., David Faure wrote: kioslave/trash/discspaceutil.cpp, line 43 http://git.reviewboard.kde.org/r/112173/diff/1/?file=183601#file183601line43 And if you really want to use stat, then this is a bit convoluted. No need to write a perfect isDir(), you're only using it in an else branch, i.e. you know that isFile() returned false already. IMHO just else if (S_ISDIR(buff.st_mode)) would be simpler and easier to read. (and folding isFile, too) But anyway, QFileInfo can do the job just fine IMHO. I will simplify the patch and only handle the case of a symlink by itself and leave everything else alone. Perhaps that would make the problem as well as the fix more clearer. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112173/#review38210 --- On Aug. 20, 2013, 1:22 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112173/ --- (Updated Aug. 20, 2013, 1:22 p.m.) Review request for KDE Runtime, David Faure and Tobias Koenig. Description --- The attached patch fixes a bug where deleting a symlink to a very large file causes a trash has reached its limit error. This happens because the code that is used to determine the amount of space available in the trashcan uses QFileInfo::size to determine the size of a file. This will not work because QFileInfo::size returns the size of the actual file the symlink points to and not the size of the symlink itself. See the documentation for QFileInfo. This addresses bug 253776. http://bugs.kde.org/show_bug.cgi?id=253776 Diffs - kioslave/trash/discspaceutil.cpp 8e76ece Diff: http://git.reviewboard.kde.org/r/112173/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 111883: Fix runtime regression caused by the fix for #134960
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111883/ --- Review request for kdelibs, Albert Astals Cid and David Faure. Description --- Attached patch fixes runtime regression discussed here: http://lists.kde.org/?t=13755400313r=1w=2. Basically, it restores the ability of unit tests to run without enabling GUI in QApplication. Diffs - kio/kio/clipboardupdater.cpp 821ef1e kio/kio/clipboardupdater_p.h e3d32bd kio/kio/copyjob.cpp fbe3cc7 kio/kio/deletejob.cpp a822373 kio/kio/fileundomanager.cpp f0f4168 kio/kio/job.cpp c7c354f kio/kio/paste.cpp 62490bb Diff: http://git.reviewboard.kde.org/r/111883/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 111883: Fix runtime regression caused by the fix for #134960
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111883/ --- (Updated Aug. 5, 2013, 11:55 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs, Albert Astals Cid and David Faure. Description --- Attached patch fixes runtime regression discussed here: http://lists.kde.org/?t=13755400313r=1w=2. Basically, it restores the ability of unit tests to run without enabling GUI in QApplication. Diffs - kio/kio/clipboardupdater.cpp 821ef1e kio/kio/clipboardupdater_p.h e3d32bd kio/kio/copyjob.cpp fbe3cc7 kio/kio/deletejob.cpp a822373 kio/kio/fileundomanager.cpp f0f4168 kio/kio/job.cpp c7c354f kio/kio/paste.cpp 62490bb Diff: http://git.reviewboard.kde.org/r/111883/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 111883: Fix runtime regression caused by the fix for #134960
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111883/ --- (Updated Aug. 5, 2013, 11:58 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs, Albert Astals Cid and David Faure. Description --- Attached patch fixes runtime regression discussed here: http://lists.kde.org/?t=13755400313r=1w=2. Basically, it restores the ability of unit tests to run without enabling GUI in QApplication. Diffs - kio/kio/clipboardupdater.cpp 821ef1e kio/kio/clipboardupdater_p.h e3d32bd kio/kio/copyjob.cpp fbe3cc7 kio/kio/deletejob.cpp a822373 kio/kio/fileundomanager.cpp f0f4168 kio/kio/job.cpp c7c354f kio/kio/paste.cpp 62490bb Diff: http://git.reviewboard.kde.org/r/111883/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 111776: Update URLs copied to clipboard when they change due to KIO operations
On Aug. 2, 2013, 12:53 p.m., David Faure wrote: kio/kio/clipboardupdater.cpp, line 108 http://git.reviewboard.kde.org/r/111776/diff/2/?file=175012#file175012line108 The name sounds like it will clear the clipboard. removeUrlsFromClipboard would be better Well this followed the naming convention used for the other statuc functions. If this one should be renamed, then perhaps it would make sense to rename the others as well, no? Perhaps overwriteUrlsInClipboard and updatedUrlsInClipboard? That way they the naming convention used will be consistent. On Aug. 2, 2013, 12:53 p.m., David Faure wrote: kio/kio/copyjob.cpp, line 2190 http://git.reviewboard.kde.org/r/111776/diff/2/?file=175014#file175014line2190 Where's this instance deleted? Should there be a deleteLater() in all code paths of slotResult()? It should be automatically deleted when the job is gone because ClipboardUpdater sets the job as its parent. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111776/#review36984 --- On Aug. 2, 2013, 12:37 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111776/ --- (Updated Aug. 2, 2013, 12:37 p.m.) Review request for kdelibs and David Faure. Description --- This patch is an improvement of https://git.reviewboard.kde.org/r/111585/ such that KIO operations also update URLs in the clipboard. As such, all KIO operations that rename, move or delete a file will always update the contents of the clipboard. A couple of notes about this patch: - KIO::trash was left out from this patch because I am unsure whether it should be treated like a delete or move operation. - Move, rename and delete operations performed outside of KIO are not covered by this patch and as such will not update URLs in the clipboard. Dealing with non KIO modifications is outside the scope of this patch since it needs to be dealt with outside of KIO. This addresses bug 134960. http://bugs.kde.org/show_bug.cgi?id=134960 Diffs - kio/kio/clipboardupdater.cpp 8ab9210 kio/kio/clipboardupdater_p.h b07c320 kio/kio/copyjob.cpp da19de5 kio/kio/deletejob.cpp 7178424 kio/kio/job.cpp 05d0ba2 kio/kio/paste.cpp b4372ab kio/tests/CMakeLists.txt b570aac kio/tests/clipboardupdatertest.h PRE-CREATION kio/tests/clipboardupdatertest.cpp PRE-CREATION kio/tests/fileundomanagertest.h e909bb7 kio/tests/fileundomanagertest.cpp 709938d Diff: http://git.reviewboard.kde.org/r/111776/diff/ Testing --- Unite tests. Thanks, Dawit Alemayehu
Re: Review Request 111776: Update URLs copied to clipboard when they change due to KIO operations
On Aug. 2, 2013, 12:53 p.m., David Faure wrote: kio/kio/copyjob.cpp, line 2190 http://git.reviewboard.kde.org/r/111776/diff/2/?file=175014#file175014line2190 Where's this instance deleted? Should there be a deleteLater() in all code paths of slotResult()? Dawit Alemayehu wrote: It should be automatically deleted when the job is gone because ClipboardUpdater sets the job as its parent. David Faure wrote: Ah doh, I think I got surprised by this twice already (i.e. you told me once before already). So either I'm stupid (senile?) or this needs more explicit documentation. Maybe in the docs for the ClipboardUpdater constructor. If there was a QObject* parent it would be obvious, but since it's a KIO::Job* job parameter, it's not that obvious that it will become the parent object. The docu should say so. Thanks. Documentation makes perfect sense. I should have done that to avoid all the confusion in the first place. Done. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111776/#review36984 --- On Aug. 2, 2013, 12:37 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111776/ --- (Updated Aug. 2, 2013, 12:37 p.m.) Review request for kdelibs and David Faure. Description --- This patch is an improvement of https://git.reviewboard.kde.org/r/111585/ such that KIO operations also update URLs in the clipboard. As such, all KIO operations that rename, move or delete a file will always update the contents of the clipboard. A couple of notes about this patch: - KIO::trash was left out from this patch because I am unsure whether it should be treated like a delete or move operation. - Move, rename and delete operations performed outside of KIO are not covered by this patch and as such will not update URLs in the clipboard. Dealing with non KIO modifications is outside the scope of this patch since it needs to be dealt with outside of KIO. This addresses bug 134960. http://bugs.kde.org/show_bug.cgi?id=134960 Diffs - kio/kio/clipboardupdater.cpp 8ab9210 kio/kio/clipboardupdater_p.h b07c320 kio/kio/copyjob.cpp da19de5 kio/kio/deletejob.cpp 7178424 kio/kio/job.cpp 05d0ba2 kio/kio/paste.cpp b4372ab kio/tests/CMakeLists.txt b570aac kio/tests/clipboardupdatertest.h PRE-CREATION kio/tests/clipboardupdatertest.cpp PRE-CREATION kio/tests/fileundomanagertest.h e909bb7 kio/tests/fileundomanagertest.cpp 709938d Diff: http://git.reviewboard.kde.org/r/111776/diff/ Testing --- Unite tests. Thanks, Dawit Alemayehu
Review Request 111776: Update URLs copied to clipboard when they change due to KIO operations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111776/ --- Review request for kdelibs and David Faure. Description --- This patch is an improvement of https://git.reviewboard.kde.org/r/111585/ such that KIO operations also update URLs in the clipboard. As such, all KIO operations that rename, move or delete a file will always update the contents of the clipboard. A couple of notes about this patch: - KIO::trash was left out from this patch because I am unsure whether it should be treated like a delete or move operation. - Move, rename and delete operations performed outside of KIO are not covered by this patch and as such will not update URLs in the clipboard. Dealing with non KIO modifications is outside the scope of this patch since it needs to be dealt with outside of KIO. This addresses bug 134960. http://bugs.kde.org/show_bug.cgi?id=134960 Diffs - kio/kio/clipboardupdater.cpp 8ab9210 kio/kio/clipboardupdater_p.h b07c320 kio/kio/copyjob.cpp da19de5 kio/kio/deletejob.cpp 7178424 kio/kio/job.cpp 05d0ba2 kio/kio/paste.cpp b4372ab kio/tests/CMakeLists.txt b570aac kio/tests/clipboardupdatertest.h PRE-CREATION kio/tests/clipboardupdatertest.cpp PRE-CREATION kio/tests/fileundomanagertest.h e909bb7 kio/tests/fileundomanagertest.cpp 709938d Diff: http://git.reviewboard.kde.org/r/111776/diff/ Testing --- Unite tests. Thanks, Dawit Alemayehu
Re: Review Request 111462: Avoid unnecessary stat of destination directory during copy/move in KDirWatch
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111462/ --- (Updated July 23, 2013, 3:47 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch and description of the fix. Description (updated) --- The patch has been modified to only avoid unnecessary stat of the destination directory during copy/move operations in KDirWatch. This way when copying a file A to directory B does not result in directory B being stat'ed everytime file A changes. Diffs (updated) - kdecore/io/kdirwatch.cpp a325f0f Diff: http://git.reviewboard.kde.org/r/111462/diff/ Testing --- - strace an instance of Konqueror or Dolphin. - copy a file from a remote location (ftp,sftp, smb) to your local file system. - check if there is a flood of stat calls on both the file being copied and its destination directory. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 21, 2013, 6:05 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs (updated) - kio/CMakeLists.txt f7a3767 kio/kio/clipboardupdater.cpp PRE-CREATION kio/kio/clipboardupdater_p.h PRE-CREATION kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/tests/fileundomanagertest.h ebd02fa kio/tests/fileundomanagertest.cpp 7c1352c Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 20, 2013, 2:06 p.m.) Review request for kdelibs and David Faure. Changes --- Uploaded the correct patch. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs (updated) - kio/CMakeLists.txt f7a3767 kio/kio/clipboardupdater.cpp PRE-CREATION kio/kio/clipboardupdater_p.h PRE-CREATION kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/tests/fileundomanagertest.h ebd02fa kio/tests/fileundomanagertest.cpp 7c1352c Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
On July 20, 2013, 9:46 p.m., David Faure wrote:
kio/kio/clipboardupdater_p.h, line 34
http://git.reviewboard.kde.org/r/111585/diff/3/?file=172590#file172590line34
I'm confused. If the user presses Undo, the whole copy/move operation
is undone, not just one URL out of many URLs, right?
So I don't see why OverwriteContent doesn't work too, for the undo case?
You cut three files A,B,C from dir1, and paste them into dir2.
OverwriteContent updates the clipboard to dir2/{A,B,C}. If you then undo,
the 3 files are moved from dir2 back to dir1, and therefore
OverwriteContent can be used to update the clipboard from dir2/{A,B,C} to
dir1/{A,B,C}. What am I missing?
Sounds to me like UpdateContent is there for the purpose of the other
cases you mention in the description (e.g. #134960), but that's not tested
yet.
Well I should have probably used a better example or elaborated more in the
documentation. The update mode is the one that should be chosen unless you are
100% certain the user did perform a cut/copy and paste operation. Currently,
the only place I know where that is the case is when KIO::pasteClipboard is
called for a move (cut+paste) operation. Otherwise, you should always use the
update mode because you do not want to inadvertently overwrite the contents of
the clipboard. I will see if I can clarify the documentation a little bit.
- Dawit
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111585/#review36226
---
On July 20, 2013, 2:06 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111585/
---
(Updated July 20, 2013, 2:06 p.m.)
Review request for kdelibs and David Faure.
Description
---
The attached patch fixes a bug where the contents of the clipboard are
prematurely updated during a cut and paste operation. In the process I also
discovered that undoing the operation does not update the clipboard either.
Hence that too is fixed by this patch.
Please note that this patch does not address all the cases where the content
of the clipboard is not updated after a KIO operation. More specifically the
clipboard content will be out of sync if the user performs the following
operations:
- copy/cut a file or a directory and rename it
- copy/cut a file or a directory and move it
- copy/cut a file or a directory and delete it.
In fact there is a ticket for the copy/cut and rename file/directory scenario
(bug# 134960). However, addressing these issues require a careful
consideration of how to do it since delete/rename/move operations can be done
outside of KDE's control. Do we simply fix the KIO jobs to handle this or do
we address it the KDirWatch level so we catch all the scenarios? Probably the
latter. Anyhow, that can wait until for the 134960 fix.
This addresses bug 318757.
http://bugs.kde.org/show_bug.cgi?id=318757
Diffs
-
kio/CMakeLists.txt f7a3767
kio/kio/clipboardupdater.cpp PRE-CREATION
kio/kio/clipboardupdater_p.h PRE-CREATION
kio/kio/fileundomanager.cpp 9f76fef
kio/kio/paste.cpp ca451fb
kio/tests/fileundomanagertest.h ebd02fa
kio/tests/fileundomanagertest.cpp 7c1352c
Diff: http://git.reviewboard.kde.org/r/111585/diff/
Testing
---
Unit and manual tests.
Thanks,
Dawit Alemayehu
Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- Review request for kdelibs and David Faure. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs - kio/CMakeLists.txt f7a3767 kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/kio/updateclipboard.cpp PRE-CREATION kio/kio/updateclipboard_p.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 19, 2013, 11:56 p.m.) Review request for kdelibs and David Faure. Changes --- Updated patch. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs (updated) - kio/CMakeLists.txt f7a3767 kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/kio/updateclipboard.cpp PRE-CREATION kio/kio/updateclipboard_p.h PRE-CREATION kio/tests/fileundomanagertest.h ebd02fa kio/tests/fileundomanagertest.cpp 7c1352c Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
On July 19, 2013, 6:25 p.m., David Faure wrote: You mention running tests, but it doesn't really count since no test is testing this clipboard stuff at the moment :-) Sorry. I attached an earlier version of the patch by mistake. :( I will attach the right one after making the changes you suggested. On July 19, 2013, 6:25 p.m., David Faure wrote: kio/kio/updateclipboard_p.h, line 30 http://git.reviewboard.kde.org/r/111585/diff/1/?file=172221#file172221line30 The class name is a verb, this is a bit unusual. Maybe call it ClipboardUpdater, rather? Could you document what the class does? OverwriteContent replaces source urls with destination urls after a copy job is done. What does UpdateContent do? What about the case where CopyJob needed some user intervention? E.g. if you get an overwrite/rename dialog, you can choose a different destination than initially planned. Does this update the desturl, i.e. the clipboard updater will do the right thing? UpdateContent preserves the urls in the clipboard and only updates those that changed where as OverwriteContent simply overwrites what is in the clipboard with a new set of urls. Anyhow, I have added documentation with specific use case examples and simplified the code a bit to make even more clearer. As far as the updater doing the right thing when a user intervention is required, it will do the right if the job itself gets updated with the changed destination url. Since we connect to the result signal and carry out the change only upon successful completion, the changes always reflect the state of the job itself when the result signal is emitted. Is it an incorrect assumption that the job will reflect the changed destination url? - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/#review36168 --- On July 19, 2013, 1:17 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 19, 2013, 1:17 p.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs - kio/CMakeLists.txt f7a3767 kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/kio/updateclipboard.cpp PRE-CREATION kio/kio/updateclipboard_p.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111462: Prevent a flood of file dirty signals from causing tons of stat calls in KDirLister
On July 9, 2013, 1:38 p.m., David Faure wrote:
To save 2 stat() per second, you want the user to not be notified anymore
that his download is going well and the local file is growing? I don't
think this is a good compromise at all. How is 2 stat calls per second a
flood ?
(I'm thinking of the use case of downloading one very big file; copying
tons of small files is a different issue)
Dawit Alemayehu wrote:
To me the number stats, which btw are 4 and not 2 because of the
additional destination directory stat, simply seem to be an overkill. If you
copy just 4 files that is 16 stats every second until these downloads
complete. How is this any different in terms of impact to users with their
home partition on nfs/smb mounts? In fact this code is exactly in the same
path as the stat problem I fixed recently with
https://git.reviewboard.kde.org/r/110834/. Granted this current problem only
applies if the destination a local file.
Anyhow, I understand the implication of my patch and have said as much
when I posted it. It would mean the user would not see the size increase by
simply looking at the size column in Konqueror/Dolphin details view. Having
said that, would it really matter to the user if the update happened every
second instead of every half second? That by itself would cut down the # of
stats by half. And if I could figure out the source of the continuous
destination directory stat and stop that, we would have reduced the number of
stats by 3/4 without really impacting the user's ability to check on the
progress of file downloads.
Well. I found out the source that continually stats the destination directory.
It is KDirWatch. Basically, here is what happens on my machine (Linux box):
1. KDirWatch calls KDirWatchPrivate::inotifyEventReceived when it receives an
event from inotify.
2. KDirWatchPrivate::inotifyEventReceived determines the event type
(modification of a file), appends the changed filename to the
m_pendingFileChanges list and starts the rescan timer (500 msec).
3. The rescan timer fires and invokes KDirWatchPrivate::slotRescan.
4. KDirWatchPrivate::slotRescan marks all entries with m_mode set to
INotifyMode (inotify) or QFSWatchMode as dirty and then iterates through
these entries to deal with any changes. Inside of this iterator, it calls
KDirWatchPrivate::scanEntry to find out what changed about a given entry.
5. KDirWatchPrivate::scanEntry uses an entries m_mode settings to determine
what to do. In case of inotify it does the following:
if (e-m_mode == FAMMode || e-m_mode == INotifyMode) {
// we know nothing has changed, no need to stat
if(!e-dirty) return NoChange;
e-dirty = false;
}
That is a problem because all the entries were already dirtied in step #4. Even
though inotify informed us explicitly that a file we are downloading that
changed, we have marked the destination directory as dirty. Hence, scanEntry
will end up doing a stat call on it over and over again until the file(s) we
are downloading are done. The question I have here is why? Is this intentional?
Or are we doing something wrong in either slotRescan or scanEntry?
- Dawit
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111462/#review35788
---
On July 9, 2013, 1:05 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111462/
---
(Updated July 9, 2013, 1:05 p.m.)
Review request for kdelibs and David Faure.
Description
---
The attached patch changes the logic that attempts to deal with a flood of
the dirty signals received by KDirLister when existing files are change. The
current code simply puts the file in an update pending list and starts a
delayed timer (500 ms) to process the request. As a result, every half a
second or so the pending update request is processed. This would have been
fine were it not for the fact that the processing results in a call to
KFileItem::refersh which does a stat the local file. Hence, a stat is done on
the file being downloaded every 500 ms. Additionally, some other code is also
doing a stat every half second on the download destination directory as well.
This patch attempts to prevent the flood of stat calls by restarting the
update timer if we get another dirty signal on a file that is already in the
update pending list. IOW, we only do the last stat call. The downside of
doing that is the information about the file being download will be stale
until the download finishes. Unfortunately I still have not been able to
figure out what is doing the stat on the destination directory itself
Re: Review Request 111462: Avoid unnecessary stat of destination directory during copy/move in KDirWatch
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111462/ --- (Updated July 14, 2013, 11:57 p.m.) Review request for kdelibs and David Faure. Changes --- Changed the patch to only prevent unnecessary stat of the destination directory during copy/move operations. Summary (updated) - Avoid unnecessary stat of destination directory during copy/move in KDirWatch Description --- The attached patch changes the logic that attempts to deal with a flood of the dirty signals received by KDirLister when existing files are change. The current code simply puts the file in an update pending list and starts a delayed timer (500 ms) to process the request. As a result, every half a second or so the pending update request is processed. This would have been fine were it not for the fact that the processing results in a call to KFileItem::refersh which does a stat the local file. Hence, a stat is done on the file being downloaded every 500 ms. Additionally, some other code is also doing a stat every half second on the download destination directory as well. This patch attempts to prevent the flood of stat calls by restarting the update timer if we get another dirty signal on a file that is already in the update pending list. IOW, we only do the last stat call. The downside of doing that is the information about the file being download will be stale until the download finishes. Unfortunately I still have not been able to figure out what is doing the stat on the destination directory itself and hence do not have a solution for that yet. Any ideas? Diffs (updated) - kdecore/io/kdirwatch.cpp a325f0f Diff: http://git.reviewboard.kde.org/r/111462/diff/ Testing --- - strace an instance of Konqueror or Dolphin. - copy a file from a remote location (ftp,sftp, smb) to your local file system. - check if there is a flood of stat calls on both the file being copied and its destination directory. Thanks, Dawit Alemayehu
Re: Review Request 111335: Fix for one of the oldest KIO bugs: multiple dialogs when KIO encounters SSL errors
On July 13, 2013, 6:32 a.m., David Faure wrote: Almost there, thanks. I'm surprised by the dispatch (to call one of the many uidelegate methods), followed by an un-dispatch (what's the term?), i.e. these many methods all end up calling the same method anyway I can see how it makes the uidelegate api look nicer, but this is all internal, nobody else than the slave should end up calling these methods (if apps need messageboxes they can just use KMessageBox directly). I'm not vetoing this solution, maybe you can leave it, I'm just wondering what's the purpose. Well to be honest I just simply hate marking publicly declared APIs as internal. That is why I did what I did. In hindsight though I should resisted my own dislike and and simply created the public API and marked it internal. You are right. The rolling and unrolling (making up words here) of the dispatch call make little sense, especially when it is done to avoid ones own pet peeve. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111335/#review35907 --- On July 13, 2013, 1:30 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111335/ --- (Updated July 13, 2013, 1:30 a.m.) Review request for kdelibs. Description --- The attached patch addresses one of the oldest bugs in KIO. Due to the muti-process nature of KIO, if any of the ioslaves encounter something that requires user input, the user might end up getting prompted multiple times. The best example of this is SSL error warnings sent to the client by kio_http. The patch completely resolves this problem using the same approach as kpasswdserver, but without the need for an additional kded process. This addresses bugs 154100 and 265228. http://bugs.kde.org/show_bug.cgi?id=154100 http://bugs.kde.org/show_bug.cgi?id=265228 Diffs - kio/CMakeLists.txt 4854829 kio/kio/job_p.h 0c1fd64 kio/kio/jobuidelegate.h 25e0728 kio/kio/jobuidelegate.cpp 85679c2 kio/kio/scheduler.cpp 802f8b8 kio/kio/slaveinterface.h 4bfcec8 kio/kio/slaveinterface.cpp aa0fc44 kio/kio/slaveinterface_p.h e31ec5e kio/kio/usernotificationhandler.cpp PRE-CREATION kio/kio/usernotificationhandler_p.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111335/diff/ Testing --- Visit a site that throws up SSL warnings and causes KIO to show more than one error dialog. Thanks, Dawit Alemayehu
Re: Review Request 111335: Fix for one of the oldest KIO bugs: multiple dialogs when KIO encounters SSL errors
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111335/ --- (Updated July 13, 2013, 7:22 a.m.) Review request for kdelibs. Changes --- Changed the JobUiDelegate APIs back to a single API that is marked as internal and corrected other minor issues. Description --- The attached patch addresses one of the oldest bugs in KIO. Due to the muti-process nature of KIO, if any of the ioslaves encounter something that requires user input, the user might end up getting prompted multiple times. The best example of this is SSL error warnings sent to the client by kio_http. The patch completely resolves this problem using the same approach as kpasswdserver, but without the need for an additional kded process. This addresses bugs 154100 and 265228. http://bugs.kde.org/show_bug.cgi?id=154100 http://bugs.kde.org/show_bug.cgi?id=265228 Diffs (updated) - kio/CMakeLists.txt 4854829 kio/kio/job.cpp 096a7d7 kio/kio/job_p.h 0c1fd64 kio/kio/jobuidelegate.h 25e0728 kio/kio/jobuidelegate.cpp 85679c2 kio/kio/scheduler.cpp 802f8b8 kio/kio/slaveinterface.h 4bfcec8 kio/kio/slaveinterface.cpp aa0fc44 kio/kio/slaveinterface_p.h e31ec5e kio/kio/usernotificationhandler.cpp PRE-CREATION kio/kio/usernotificationhandler_p.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111335/diff/ Testing --- Visit a site that throws up SSL warnings and causes KIO to show more than one error dialog. Thanks, Dawit Alemayehu
Re: Review Request 111335: Fix for one of the oldest KIO bugs: multiple dialogs when KIO encounters SSL errors
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111335/ --- (Updated July 13, 2013, 1:30 a.m.) Review request for kdelibs. Changes --- - Removed the new public from SimpleJob. Added it to SimpleJobPrivate instead. - Changed the JobUiDelegate API into multiple simple APIs that directly correspond to the KMessageBox API. Description --- The attached patch addresses one of the oldest bugs in KIO. Due to the muti-process nature of KIO, if any of the ioslaves encounter something that requires user input, the user might end up getting prompted multiple times. The best example of this is SSL error warnings sent to the client by kio_http. The patch completely resolves this problem using the same approach as kpasswdserver, but without the need for an additional kded process. This addresses bugs 154100 and 265228. http://bugs.kde.org/show_bug.cgi?id=154100 http://bugs.kde.org/show_bug.cgi?id=265228 Diffs (updated) - kio/CMakeLists.txt 4854829 kio/kio/job_p.h 0c1fd64 kio/kio/jobuidelegate.h 25e0728 kio/kio/jobuidelegate.cpp 85679c2 kio/kio/scheduler.cpp 802f8b8 kio/kio/slaveinterface.h 4bfcec8 kio/kio/slaveinterface.cpp aa0fc44 kio/kio/slaveinterface_p.h e31ec5e kio/kio/usernotificationhandler.cpp PRE-CREATION kio/kio/usernotificationhandler_p.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111335/diff/ Testing --- Visit a site that throws up SSL warnings and causes KIO to show more than one error dialog. Thanks, Dawit Alemayehu
Re: Review Request 111335: Fix for one of the oldest KIO bugs: multiple dialogs when KIO encounters SSL errors
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111335/ --- (Updated July 11, 2013, 11:37 p.m.) Review request for kdelibs. Changes --- Made first iteration of the changes as discussed. I am not entirely sure whether or not SimpleJob::requestMessageBox and JobUiDelegate::requestMessageBox should be more generic or very specific with many parameters as I have them now. Description --- The attached patch addresses one of the oldest bugs in KIO. Due to the muti-process nature of KIO, if any of the ioslaves encounter something that requires user input, the user might end up getting prompted multiple times. The best example of this is SSL error warnings sent to the client by kio_http. The patch completely resolves this problem using the same approach as kpasswdserver, but without the need for an additional kded process. This addresses bugs 154100 and 265228. http://bugs.kde.org/show_bug.cgi?id=154100 http://bugs.kde.org/show_bug.cgi?id=265228 Diffs (updated) - kio/CMakeLists.txt 4854829 kio/kio/job.cpp 096a7d7 kio/kio/jobclasses.h d771cfe kio/kio/jobuidelegate.h 25e0728 kio/kio/jobuidelegate.cpp 85679c2 kio/kio/slaveinterface.h 4bfcec8 kio/kio/slaveinterface.cpp aa0fc44 kio/kio/slaveinterface_p.h e31ec5e kio/kio/usernotificationhandler.cpp PRE-CREATION kio/kio/usernotificationhandler_p.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111335/diff/ Testing --- Visit a site that throws up SSL warnings and causes KIO to show more than one error dialog. Thanks, Dawit Alemayehu
Re: Review Request 111335: Fix for one of the oldest KIO bugs: multiple dialogs when KIO encounters SSL errors
On July 5, 2013, 2:23 p.m., David Faure wrote: kio/kio/scheduler.cpp, line 766 http://git.reviewboard.kde.org/r/111335/diff/2/?file=167766#file167766line766 Please don't put this code in scheduler.cpp I'm trying to properly split core and gui aspects of KIO in frameworks, and scheduler is definitely core, while kmessagebox is definitely not. Please find a way to separate the two. Dawit Alemayehu wrote: So how were you planning to separate the core and gui aspects in frameworks? Without this patch KIO::Scheduler will still be linking against gui libraries because of its use of KIO::Slave. Perhaps if I know how you were planning to perform the split, I could follow the same approach to resolve this issue and it would be one less thing you have to deal with. David Faure wrote: I'm separating core/gui stuff for jobs using delegates and delegate extensions and factories. (the trick is that the kiowidgets library can register stuff using code that runs automatically, just by linking to the library, see Q_CONSTRUCTOR_FUNCTION in kio/jobuidelegate.cpp) But nothing is done yet for the messagebox stuff in Slave. So this is good timing, let's come up with a solution (which we could probably apply to both branches). Context: SlaveInterface::messageBox() is called by kioslaves, in the application process. Let's brainstorm. I can think of 3 solutions on top of my head: 1) Defining an interface in kiocore and implementing it in both libs. The core implementation would have to return Cancel every time, for lack of a possibility to interact with the user. Apps could still reimplement that interface to give predefined answers. 2) Propagating the call up to the job, which can then use the delegate mechanism for showing the messagebox (with again a canned reply for core-only code) 3) Delegating the messagebox to a separate process, e.g. kuiserver. This is the KDE3 solution, actually. The commit that removed that (8d6f7d340e0) says that modal messagebox were blocking kuiserver. But we could use non-modal messageboxes instead. Either with a blocking dbus call (using dbus transactions in kuiserver), or with real async everywhere. Problem: what if kuiserver isn't available. Or what if you wanted a core-only command-line tool which would not interact with the user. Any other ideas? Any input on the above possibilities? I kind of like number 2, to reduce the number of interfaces being used to call gui stuff from kiocore. Dawit Alemayehu wrote: I do not have any additional ideas than the one you presented here. I like #2 as well. I am not fond of the separate process solution at all. One cannot really associate the dialog with the client (read: window). We have hacks that attempt to simulate that, but they are still hacks ; so I prefer solution #2 as well. David Faure wrote: OK then, let's go for that. Do you plan on implementing it, or should I put it on my own TODO list? Dawit Alemayehu wrote: I can do it since I want to resolve this bug and have already started the process so to speak. But I need your help to get started. What should I look at to understand how the separation of concerns (core/gui) work in frameworks? I see how jobuidelegate works in KIO under KDE 4 and seems to be a straight forward step for me to move the current code there, but I suspect that class has changed some in frameworks to do the split, correct? David Faure wrote: In frameworks, KIO's JobUiDelegate still exists, but implements new interfaces (now it derives from both KDialogJobUiDelegate and JobUiDelegateExtension) so that its kio-specific API (askFileRename/askSkip/...) can be called from within core-only KIO jobs. So if you need to add new API to KIO::JobUiDelegate in kde4 that's fine, I'll just add it to JobUiDelegateExtension in frameworks so that it can still be called. Dawit Alemayehu wrote: Hmm... I can only move the GUI related portion of the code I wrote out of the scheduler. Everything else has to remain there in order for this to work since the message box is much like the password server. In fact, if this separation works, we should also switch the password server to this model in frameworks. Anyhow, this is my current plan: 1.) Move the messagebox portion of the current code, effectively the else statement in UserNotificationHandler::processRequest, to jobuidelegate. 2.) Move the code from step one to KIO::JobUiDelete::askUser(...)??? 2.) Change the else statement in UserNotificationHandler::processRequest to call either r-slave-job()-requestMessageBox or r-slave-job()-ui()-askUser. Probably the former one which itself will end up calling the latter one. Does this seem reasonable to you? Any objections
Review Request 111462: Prevent a flood of file dirty signals from causing tons of stat calls in KDirLister
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111462/ --- Review request for kdelibs and David Faure. Description --- The attached patch changes the logic that attempts to deal with a flood of the dirty signals received by KDirLister when existing files are change. The current code simply puts the file in an update pending list and starts a delayed timer (500 ms) to process the request. As a result, every half a second or so the pending update request is processed. This would have been fine were it not for the fact that the processing results in a call to KFileItem::refersh which does a stat the local file. Hence, a stat is done on the file being downloaded every 500 ms. Additionally, some other code is also doing a stat every half second on the download destination directory as well. This patch attempts to prevent the flood of stat calls by restarting the update timer if we get another dirty signal on a file that is already in the update pending list. IOW, we only do the last stat call. The downside of doing that is the information about the file being download will be stale until the download finishes. Unfortunately I still have not been able to figure out what is doing the stat on the destination directory itself and hence do not have a solution for that yet. Any ideas? Diffs - kio/kio/kdirlister.cpp aad6893 Diff: http://git.reviewboard.kde.org/r/111462/diff/ Testing --- - strace an instance of Konqueror or Dolphin. - copy a file from a remote location (ftp,sftp, smb) to your local file system. - check if there is a flood of stat calls on both the file being copied and its destination directory. Thanks, Dawit Alemayehu
Re: Review Request 111335: Fix for one of the oldest KIO bugs: multiple dialogs when KIO encounters SSL errors
On July 5, 2013, 2:23 p.m., David Faure wrote: kio/kio/scheduler.cpp, line 766 http://git.reviewboard.kde.org/r/111335/diff/2/?file=167766#file167766line766 Please don't put this code in scheduler.cpp I'm trying to properly split core and gui aspects of KIO in frameworks, and scheduler is definitely core, while kmessagebox is definitely not. Please find a way to separate the two. Dawit Alemayehu wrote: So how were you planning to separate the core and gui aspects in frameworks? Without this patch KIO::Scheduler will still be linking against gui libraries because of its use of KIO::Slave. Perhaps if I know how you were planning to perform the split, I could follow the same approach to resolve this issue and it would be one less thing you have to deal with. David Faure wrote: I'm separating core/gui stuff for jobs using delegates and delegate extensions and factories. (the trick is that the kiowidgets library can register stuff using code that runs automatically, just by linking to the library, see Q_CONSTRUCTOR_FUNCTION in kio/jobuidelegate.cpp) But nothing is done yet for the messagebox stuff in Slave. So this is good timing, let's come up with a solution (which we could probably apply to both branches). Context: SlaveInterface::messageBox() is called by kioslaves, in the application process. Let's brainstorm. I can think of 3 solutions on top of my head: 1) Defining an interface in kiocore and implementing it in both libs. The core implementation would have to return Cancel every time, for lack of a possibility to interact with the user. Apps could still reimplement that interface to give predefined answers. 2) Propagating the call up to the job, which can then use the delegate mechanism for showing the messagebox (with again a canned reply for core-only code) 3) Delegating the messagebox to a separate process, e.g. kuiserver. This is the KDE3 solution, actually. The commit that removed that (8d6f7d340e0) says that modal messagebox were blocking kuiserver. But we could use non-modal messageboxes instead. Either with a blocking dbus call (using dbus transactions in kuiserver), or with real async everywhere. Problem: what if kuiserver isn't available. Or what if you wanted a core-only command-line tool which would not interact with the user. Any other ideas? Any input on the above possibilities? I kind of like number 2, to reduce the number of interfaces being used to call gui stuff from kiocore. Dawit Alemayehu wrote: I do not have any additional ideas than the one you presented here. I like #2 as well. I am not fond of the separate process solution at all. One cannot really associate the dialog with the client (read: window). We have hacks that attempt to simulate that, but they are still hacks ; so I prefer solution #2 as well. David Faure wrote: OK then, let's go for that. Do you plan on implementing it, or should I put it on my own TODO list? Dawit Alemayehu wrote: I can do it since I want to resolve this bug and have already started the process so to speak. But I need your help to get started. What should I look at to understand how the separation of concerns (core/gui) work in frameworks? I see how jobuidelegate works in KIO under KDE 4 and seems to be a straight forward step for me to move the current code there, but I suspect that class has changed some in frameworks to do the split, correct? David Faure wrote: In frameworks, KIO's JobUiDelegate still exists, but implements new interfaces (now it derives from both KDialogJobUiDelegate and JobUiDelegateExtension) so that its kio-specific API (askFileRename/askSkip/...) can be called from within core-only KIO jobs. So if you need to add new API to KIO::JobUiDelegate in kde4 that's fine, I'll just add it to JobUiDelegateExtension in frameworks so that it can still be called. Hmm... I can only move the GUI related portion of the code I wrote out of the scheduler. Everything else has to remain there in order for this to work since the message box is much like the password server. In fact, if this separation works, we should also switch the password server to this model in frameworks. Anyhow, this is my current plan: 1.) Move the messagebox portion of the current code, effectively the else statement in UserNotificationHandler::processRequest, to jobuidelegate. 2.) Move the code from step one to KIO::JobUiDelete::askUser(...)??? 2.) Change the else statement in UserNotificationHandler::processRequest to call either r-slave-job()-requestMessageBox or r-slave-job()-ui()-askUser. Probably the former one which itself will end up calling the latter one. Does this seem reasonable to you? Any objections/concerns? - Dawit
Review Request 111419: Use case-insensitive URL scheme comparisons in KUrl::isLocalFile
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111419/ --- Review request for kdelibs and David Faure. Description --- This patch fixes KUrl::isLocalFile to properly deal with upper case URL scheme. This addresses bug 295371. http://bugs.kde.org/show_bug.cgi?id=295371 Diffs - kdecore/services/kservice.h 3843bad kdecore/services/kservice.cpp e2cc86f kio/kfile/kopenwithdialog.cpp 84465cd Diff: http://git.reviewboard.kde.org/r/111419/diff/ Testing --- kioclient exec FILE:/// Thanks, Dawit Alemayehu
Re: Review Request 111419: Use case-insensitive URL scheme comparisons in KUrl::isLocalFile
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111419/ --- (Updated July 6, 2013, 4:55 p.m.) Review request for kdelibs and David Faure. Changes --- Uploaded the correct patch. Description --- This patch fixes KUrl::isLocalFile to properly deal with upper case URL scheme. This addresses bug 295371. http://bugs.kde.org/show_bug.cgi?id=295371 Diffs (updated) - kdecore/io/kurl.cpp 5ea3e01 kdecore/tests/kurltest.cpp 71770ee Diff: http://git.reviewboard.kde.org/r/111419/diff/ Testing --- kioclient exec FILE:/// Thanks, Dawit Alemayehu
Re: Review Request 111335: Fix for one of the oldest KIO bugs: multiple dialogs when KIO encounters SSL errors
On July 5, 2013, 2:23 p.m., David Faure wrote: kio/kio/scheduler.cpp, line 766 http://git.reviewboard.kde.org/r/111335/diff/2/?file=167766#file167766line766 Please don't put this code in scheduler.cpp I'm trying to properly split core and gui aspects of KIO in frameworks, and scheduler is definitely core, while kmessagebox is definitely not. Please find a way to separate the two. Dawit Alemayehu wrote: So how were you planning to separate the core and gui aspects in frameworks? Without this patch KIO::Scheduler will still be linking against gui libraries because of its use of KIO::Slave. Perhaps if I know how you were planning to perform the split, I could follow the same approach to resolve this issue and it would be one less thing you have to deal with. David Faure wrote: I'm separating core/gui stuff for jobs using delegates and delegate extensions and factories. (the trick is that the kiowidgets library can register stuff using code that runs automatically, just by linking to the library, see Q_CONSTRUCTOR_FUNCTION in kio/jobuidelegate.cpp) But nothing is done yet for the messagebox stuff in Slave. So this is good timing, let's come up with a solution (which we could probably apply to both branches). Context: SlaveInterface::messageBox() is called by kioslaves, in the application process. Let's brainstorm. I can think of 3 solutions on top of my head: 1) Defining an interface in kiocore and implementing it in both libs. The core implementation would have to return Cancel every time, for lack of a possibility to interact with the user. Apps could still reimplement that interface to give predefined answers. 2) Propagating the call up to the job, which can then use the delegate mechanism for showing the messagebox (with again a canned reply for core-only code) 3) Delegating the messagebox to a separate process, e.g. kuiserver. This is the KDE3 solution, actually. The commit that removed that (8d6f7d340e0) says that modal messagebox were blocking kuiserver. But we could use non-modal messageboxes instead. Either with a blocking dbus call (using dbus transactions in kuiserver), or with real async everywhere. Problem: what if kuiserver isn't available. Or what if you wanted a core-only command-line tool which would not interact with the user. Any other ideas? Any input on the above possibilities? I kind of like number 2, to reduce the number of interfaces being used to call gui stuff from kiocore. I do not have any additional ideas than the one you presented here. I like #2 as well. I am not fond of the separate process solution at all. One cannot really associate the dialog with the client (read: window). We have hacks that attempt to simulate that, but they are still hacks ; so I prefer solution #2 as well. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111335/#review35634 --- On July 3, 2013, 12:19 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111335/ --- (Updated July 3, 2013, 12:19 p.m.) Review request for kdelibs. Description --- The attached patch addresses one of the oldest bugs in KIO. Due to the muti-process nature of KIO, if any of the ioslaves encounter something that requires user input, the user might end up getting prompted multiple times. The best example of this is SSL error warnings sent to the client by kio_http. The patch completely resolves this problem using the same approach as kpasswdserver, but without the need for an additional kded process. This addresses bugs 154100 and 265228. http://bugs.kde.org/show_bug.cgi?id=154100 http://bugs.kde.org/show_bug.cgi?id=265228 Diffs - kio/kio/scheduler.h 04edb40 kio/kio/scheduler.cpp 802f8b8 kio/kio/scheduler_p.h d68f645 kio/kio/slaveinterface.h 4bfcec8 kio/kio/slaveinterface.cpp aa0fc44 kio/kio/slaveinterface_p.h e31ec5e Diff: http://git.reviewboard.kde.org/r/111335/diff/ Testing --- Visit a site that throws up SSL warnings and causes KIO to show more than one error dialog. Thanks, Dawit Alemayehu
Re: Review Request 111335: Fix for one of the oldest KIO bugs: multiple dialogs when KIO encounters SSL errors
On July 5, 2013, 2:23 p.m., David Faure wrote: kio/kio/scheduler.cpp, line 766 http://git.reviewboard.kde.org/r/111335/diff/2/?file=167766#file167766line766 Please don't put this code in scheduler.cpp I'm trying to properly split core and gui aspects of KIO in frameworks, and scheduler is definitely core, while kmessagebox is definitely not. Please find a way to separate the two. Dawit Alemayehu wrote: So how were you planning to separate the core and gui aspects in frameworks? Without this patch KIO::Scheduler will still be linking against gui libraries because of its use of KIO::Slave. Perhaps if I know how you were planning to perform the split, I could follow the same approach to resolve this issue and it would be one less thing you have to deal with. David Faure wrote: I'm separating core/gui stuff for jobs using delegates and delegate extensions and factories. (the trick is that the kiowidgets library can register stuff using code that runs automatically, just by linking to the library, see Q_CONSTRUCTOR_FUNCTION in kio/jobuidelegate.cpp) But nothing is done yet for the messagebox stuff in Slave. So this is good timing, let's come up with a solution (which we could probably apply to both branches). Context: SlaveInterface::messageBox() is called by kioslaves, in the application process. Let's brainstorm. I can think of 3 solutions on top of my head: 1) Defining an interface in kiocore and implementing it in both libs. The core implementation would have to return Cancel every time, for lack of a possibility to interact with the user. Apps could still reimplement that interface to give predefined answers. 2) Propagating the call up to the job, which can then use the delegate mechanism for showing the messagebox (with again a canned reply for core-only code) 3) Delegating the messagebox to a separate process, e.g. kuiserver. This is the KDE3 solution, actually. The commit that removed that (8d6f7d340e0) says that modal messagebox were blocking kuiserver. But we could use non-modal messageboxes instead. Either with a blocking dbus call (using dbus transactions in kuiserver), or with real async everywhere. Problem: what if kuiserver isn't available. Or what if you wanted a core-only command-line tool which would not interact with the user. Any other ideas? Any input on the above possibilities? I kind of like number 2, to reduce the number of interfaces being used to call gui stuff from kiocore. Dawit Alemayehu wrote: I do not have any additional ideas than the one you presented here. I like #2 as well. I am not fond of the separate process solution at all. One cannot really associate the dialog with the client (read: window). We have hacks that attempt to simulate that, but they are still hacks ; so I prefer solution #2 as well. David Faure wrote: OK then, let's go for that. Do you plan on implementing it, or should I put it on my own TODO list? I can do it since I want to resolve this bug and have already started the process so to speak. But I need your help to get started. What should I look at to understand how the separation of concerns (core/gui) work in frameworks? I see how jobuidelegate works in KIO under KDE 4 and seems to be a straight forward step for me to move the current code there, but I suspect that class has changed some in frameworks to do the split, correct? - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111335/#review35634 --- On July 3, 2013, 12:19 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111335/ --- (Updated July 3, 2013, 12:19 p.m.) Review request for kdelibs. Description --- The attached patch addresses one of the oldest bugs in KIO. Due to the muti-process nature of KIO, if any of the ioslaves encounter something that requires user input, the user might end up getting prompted multiple times. The best example of this is SSL error warnings sent to the client by kio_http. The patch completely resolves this problem using the same approach as kpasswdserver, but without the need for an additional kded process. This addresses bugs 154100 and 265228. http://bugs.kde.org/show_bug.cgi?id=154100 http://bugs.kde.org/show_bug.cgi?id=265228 Diffs - kio/kio/scheduler.h 04edb40 kio/kio/scheduler.cpp 802f8b8 kio/kio/scheduler_p.h d68f645 kio/kio/slaveinterface.h 4bfcec8 kio/kio/slaveinterface.cpp aa0fc44 kio/kio
