Re: Review Request: Proper password caching when opening remote directories in KFileDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/#review8619 --- kdeui/jobs/kdialogjobuidelegate.cpp http://git.reviewboard.kde.org/r/103226/#comment7280 Can't see the point in going up to the mainwindow here. In scheduler.cpp yes, but in the delegate? kfile/kdiroperator.cpp http://git.reviewboard.kde.org/r/103226/#comment7281 If the scheduler already goes up to the window, why not just use this in all the method calls here? this is a child of parent which is in parent-window, so the loop will find the window just fine, and this removes the need for another member variable here. kfile/kdirselectdialog.cpp http://git.reviewboard.kde.org/r/103226/#comment7282 Same here, this will do just fine. kfile/kfilewidget.cpp http://git.reviewboard.kde.org/r/103226/#comment7283 And here too, this is a child of parent anyway. kfile/knewfilemenu.cpp http://git.reviewboard.kde.org/r/103226/#comment7284 This function is now unused, please remove it. kio/kio/scheduler.cpp http://git.reviewboard.kde.org/r/103226/#comment7285 add comment to explain why we don't use setWindow - David Faure On Nov. 29, 2011, 4:09 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 29, 2011, 4:09 p.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs - kdeui/jobs/kdialogjobuidelegate.cpp fe48f87 kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 kio/kio/scheduler.cpp b4e95a5 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Re: Review Request: Proper password caching when opening remote directories in KFileDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/#review8634 --- This review has been submitted with commit c30ee2b64f82a0cf09f50b765a29908efb8ebf05 by Dawit Alemayehu to branch KDE/4.7. - Commit Hook On Nov. 30, 2011, 6:25 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 30, 2011, 6:25 p.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs - kio/kio/scheduler.cpp b4e95a5 kfile/knewfilemenu.cpp ac54041 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/kdiroperator.cpp 4c93ac9 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Re: Review Request: Proper password caching when opening remote directories in KFileDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/#review8590 --- kfile/kdirselectdialog.cpp http://git.reviewboard.kde.org/r/103226/#comment7272 Is this really necessary? I thought KIO did this internally already, so that when creating a job we could just pass any widget as parameter. kfile/knewfilemenu.cpp http://git.reviewboard.kde.org/r/103226/#comment7273 It would really be much simpler to do this inside of setWindow() or even deeper down... - David Faure On Nov. 24, 2011, 3:16 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 24, 2011, 3:16 p.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs - kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Re: Review Request: Proper password caching when opening remote directories in KFileDialog
On Nov. 29, 2011, 10:40 a.m., David Faure wrote: kfile/kdirselectdialog.cpp, line 273 http://git.reviewboard.kde.org/r/103226/diff/3/?file=41903#file41903line273 Is this really necessary? I thought KIO did this internally already, so that when creating a job we could just pass any widget as parameter. Yes it is. Right now KIO::Scheduler makes a half-hearted attempt to find the top level window by doing widget-window(). Unfortunately that is not enough when using a file dialog because it is considered a top level window, i.e. it is an ancestor widget that has or could have a window system frame. As such, the window id of the file dialog will be used to determine the lifetime of the password instead of the one from the actual application where the file dialog was invoked. On Nov. 29, 2011, 10:40 a.m., David Faure wrote: kfile/knewfilemenu.cpp, line 966 http://git.reviewboard.kde.org/r/103226/diff/3/?file=41905#file41905line966 It would really be much simpler to do this inside of setWindow() or even deeper down... Well I have no real objections to doing that though I have a small concern. If the check for the topLevelWindow is moved to setWindow or even deeper, it would become a tiny bit more expensive for applications that were doing the right thing when setting the top level window in the first place. I am sure the impact won't be much, but nonetheless it would still be more than it is now. If the check should be moved, then it needs to be moved deeper than setWindow() because setWindow() simply does the following: KDialogJobUiDelegate::setWindow(w); KIO::Scheduler::registerWindow(w); In order to ensure the proper logic is used at the lowest possible location, the check IMHO needs to be implemented at the level of those two classes. I will provide an updated patch. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/#review8590 --- On Nov. 24, 2011, 3:16 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 24, 2011, 3:16 p.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs - kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Re: Review Request: Proper password caching when opening remote directories in KFileDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 29, 2011, 4:09 p.m.) Review request for kdelibs and David Faure. Changes --- Updated patch based on feedback. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs (updated) - kdeui/jobs/kdialogjobuidelegate.cpp fe48f87 kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 kio/kio/scheduler.cpp b4e95a5 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Re: Review Request: Proper password caching when opening remote directories in KFileDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/#review8438 --- kfile/kdiroperator.cpp http://git.reviewboard.kde.org/r/103226/#comment7159 how is this different just calling widget-window()? - Aaron J. Seigo On Nov. 24, 2011, 7:37 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 24, 2011, 7:37 a.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs - kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Re: Review Request: Proper password caching when opening remote directories in KFileDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/#review8439 --- kfile/kdirselectdialog.cpp http://git.reviewboard.kde.org/r/103226/#comment7160 this should be a QWeakPointer. QPointer has significant overhead that QWeakPointer avoids. kfile/kfilewidget.cpp http://git.reviewboard.kde.org/r/103226/#comment7161 QWeakPointer kfile/kfilewidget.cpp http://git.reviewboard.kde.org/r/103226/#comment7162 parent-window()? kfile/knewfilemenu.cpp http://git.reviewboard.kde.org/r/103226/#comment7163 QWeakPointer kfile/knewfilemenu.cpp http://git.reviewboard.kde.org/r/103226/#comment7164 d-m_parentWidget-window()? - Aaron J. Seigo On Nov. 24, 2011, 7:37 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 24, 2011, 7:37 a.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs - kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Re: Review Request: Proper password caching when opening remote directories in KFileDialog
On Nov. 24, 2011, 8:37 a.m., Aaron J. Seigo wrote: kfile/kdirselectdialog.cpp, line 102 http://git.reviewboard.kde.org/r/103226/diff/2/?file=41892#file41892line102 this should be a QWeakPointer. QPointer has significant overhead that QWeakPointer avoids. Actually there is no need for either one. A plain old pointer would do just fine in this case. Will change that. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/#review8439 --- On Nov. 24, 2011, 7:37 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 24, 2011, 7:37 a.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs - kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Re: Review Request: Proper password caching when opening remote directories in KFileDialog
On Nov. 24, 2011, 8:35 a.m., Aaron J. Seigo wrote: KDirOperator is called from KFileWidget which is called from KFileDialog. Guess what the parent-window() would give you in this case ? The KFileDialog because it is the next valid ancestor widget that has a window-system frame. If we used that we would improve the current situation from keep the password for a short duration (~10 sec) to keep it until the file dialog is closed, but that is not what we want. We want to keep the password until the application that invoked the KFileDialog is closed. That is why we topLevelWindow to iteration through the parent widgets to find the top most level widget with a window-system frame. Failing that it falls back to parent-window(). On Nov. 24, 2011, 8:35 a.m., Aaron J. Seigo wrote: kfile/kdiroperator.cpp, line 384 http://git.reviewboard.kde.org/r/103226/diff/2/?file=41891#file41891line384 how is this different just calling widget-window()? Thomas Lübking wrote: As far as I can say it scans for the oldest ancestor across window borders and returns the window for this widget. QWidget *w1 = new QWidget; // is implicitly window QWidget *w2 = new QWidget(w1, Qt::Window); // is explicitly window QWidget *w3 = new QWidget(w2); // not a window w3-window() == w2; topLevelWindow(w3) == w1; Just this: return (w ? w-window() : (widget ? widget-window() : 0)); is to me actually return w ? w-window() : 0; // w != 0 || widget == 0 Just this: return (w ? w-window() : (widget ? widget-window() : 0)); is to me actually return w ? w-window() : 0; // w != 0 || widget == 0 Actually that is true since I changed the while loop to (w w-parentWidget()). I forgot to update the return statement. Will fix that as well. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/#review8438 --- On Nov. 24, 2011, 7:37 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 24, 2011, 7:37 a.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs - kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Re: Review Request: Proper password caching when opening remote directories in KFileDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 24, 2011, 3:16 p.m.) Review request for kdelibs and David Faure. Changes --- Updated patch based on feedback. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs (updated) - kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Review Request: Proper password caching when opening remote directories in KFileDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- Review request for kdelibs and David Faure. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs - kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
Re: Review Request: Proper password caching when opening remote directories in KFileDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103226/ --- (Updated Nov. 24, 2011, 7:37 a.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the Remember password checkbox, for the duration of the application instead of just the lifetime of the file dialog. Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again. This addresses bug 179663. http://bugs.kde.org/show_bug.cgi?id=179663 Diffs (updated) - kfile/kdiroperator.cpp 4c93ac9 kfile/kdirselectdialog.cpp 0212e58 kfile/kfilewidget.cpp 09b86d4 kfile/knewfilemenu.cpp ac54041 Diff: http://git.reviewboard.kde.org/r/103226/diff/diff Testing --- Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org. Thanks, Dawit Alemayehu
