Re: Review Request: Make it possible to login to 2 different sites from two separate instance of same application
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103245/#review8616 --- This review has been submitted with commit d82c98d103ec909a6ad4a1cc8673c58a26a853c1 by Dawit Alemayehu to branch KDE/4.7. - Commit Hook On Nov. 26, 2011, 3:16 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103245/ --- (Updated Nov. 26, 2011, 3:16 p.m.) Review request for KDE Runtime. Description --- KPasswdServer currently processes requests to prompt the user for passwords serially, i.e. one request at a time even if those requests came two totally different applications. Unfortunately that results one application waiting on a completely unrelated application to receive authorization information, which IMHO is unacceptable. The commit that caused this problem was a4cc5583 and its intent was to address the issue of 2 or more password dialogs being shown at the same time. The attached patch modifies that commit so that processRequest will handle requests so long as #1. The request comes from two separate window ids. #2. There is no pending prompt for the requested key (read: host+port). So long as the above two conditions are met, the password prompt will be shown as required. This fixes a bug report, which I cannot find at the moment, where attempting to log into an sftp server in one Konqueror instance prevented the user from logging into a router admin interface in a completely different Konqueror instance. Diffs - kpasswdserver/kpasswdserver.h 351cb7c kpasswdserver/kpasswdserver.cpp cc8ded2 Diff: http://git.reviewboard.kde.org/r/103245/diff/diff Testing --- Attempt to log into two different sites from two different applications of two different instances of the same application. Thanks, Dawit Alemayehu
Re: Review Request: Make it possible to login to 2 different sites from two separate instance of same application
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103245/#review8617 --- This review has been submitted with commit cfd308a5ed59be49655a09db113c6a1a8562e797 by Dawit Alemayehu to branch master. - Commit Hook On Nov. 26, 2011, 3:16 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103245/ --- (Updated Nov. 26, 2011, 3:16 p.m.) Review request for KDE Runtime. Description --- KPasswdServer currently processes requests to prompt the user for passwords serially, i.e. one request at a time even if those requests came two totally different applications. Unfortunately that results one application waiting on a completely unrelated application to receive authorization information, which IMHO is unacceptable. The commit that caused this problem was a4cc5583 and its intent was to address the issue of 2 or more password dialogs being shown at the same time. The attached patch modifies that commit so that processRequest will handle requests so long as #1. The request comes from two separate window ids. #2. There is no pending prompt for the requested key (read: host+port). So long as the above two conditions are met, the password prompt will be shown as required. This fixes a bug report, which I cannot find at the moment, where attempting to log into an sftp server in one Konqueror instance prevented the user from logging into a router admin interface in a completely different Konqueror instance. Diffs - kpasswdserver/kpasswdserver.h 351cb7c kpasswdserver/kpasswdserver.cpp cc8ded2 Diff: http://git.reviewboard.kde.org/r/103245/diff/diff Testing --- Attempt to log into two different sites from two different applications of two different instances of the same application. 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/#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: Add git support to kdesdk: create_tarball.rb
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6842/#review10493 --- Test with the following settings in the config.ini here and ran it with '-a kmymoney' as option: The current stable version [kmymoney] gitModule= yes gitTag = 4.6.1 mainmodule = branches/stable/extragear-kde4 l10nmodule = extragear-office l10npath = branches/stable submodule= office kde_release = no docs = yes docpath = kmymoney/doc translations = yes version = 4.6.1 and the development version [kmymoney] gitModule= yes mainmodule = extragear submodule= office kde_release = no docs = yes translations = yes version = 4.6.90 and it looks good so far (except that the docs won't build) trunk/KDE/kdesdk/scripts/createtarball/create_tarball.rb http://svn.reviewboard.kde.org/r/6842/#comment12416 Is there a specific reason why you don't add the doc subdirectory for stuff fetched from git? - Thomas Baumgart On Nov. 29, 2011, 4:42 p.m., Kåre Särs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6842/ --- (Updated Nov. 29, 2011, 4:42 p.m.) Review request for kdelibs and Release Team. Description --- This patch adds git support to create_tarball.rb in kdesdk. The patch adds two options to the ini file. The first one (gitModule) indicates that the module is located in git and the second optional parameter (gitTag) defines which tag to use for creating the release. (there is no group for kdesdk or extragear) Diffs - trunk/KDE/kdesdk/scripts/createtarball/config.ini 1266277 trunk/KDE/kdesdk/scripts/createtarball/create_tarball.rb 1266277 Diff: http://svn.reviewboard.kde.org/r/6842/diff/diff Testing --- Thanks, Kåre Särs
Re: Review Request: Fix KDateComboBox shrinking in size after a date is entered
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103133/#review8622 --- This review has been submitted with commit b2acdedce156893acb3007d359af4d53983bd351 by David Jarvie to branch KDE/4.7. - Commit Hook On Nov. 14, 2011, 10:58 p.m., David Jarvie wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103133/ --- (Updated Nov. 14, 2011, 10:58 p.m.) Review request for kdelibs and John Layt. Description --- After the user enters a date by means of the date picker or by up/down arrows, the edit field in KDateComboBox shrinks so that it is too small to display all the characters in the date. In addition, when first displayed, the widget visibly resizes, which doesn't look particularly good. This patch fixes these issues. Note that the patch is really a workaround for the issue - I haven't managed to find out exactly why the shrinkage occurs. However, it is a simple fix, so unless somebody else comes up with a better way, I think it's good enough. Diffs - kdeui/widgets/kdatecombobox.cpp fc239bc Diff: http://git.reviewboard.kde.org/r/103133/diff/diff Testing --- Tested successfully in KAlarm's alarm edit dialog. Thanks, David Jarvie
Review Request 101486 (Q/KComboBox related KConfigDialogManager change) breaks apps
Hi, just came around to notice this now, but the mentioned code-change from the review request: https://git.reviewboard.kde.org/r/101486/ actually does break apps. In particular it breaks kdevelop. We have a KComboBox subclass which actually wants to store a custom string-property into the kconfig file and not an index. At the same time the combobox is not editable, but the strings are user-supplied (in a separate widget somewhere else). So storing the string is fine and actually necessary since the order is undefined over restarts. The commit 8edc1932ecc62370d9a31836dfa9b2bd0175a293 and d44186bce4670d2985fb6aba8dba59bbd2c4c77a changed the kconfigdialogmanager to first check for q/kcombobox style and then enforces using the index or the text depending on the editable-flag. The reason for that is that QCombobox in Qt 4.8 changed its behaviour and exposes its currentIndex roperty as a user-proeprty. This leads to storing the index of user-editable comboboxes instead of the text, which is wrong and even dangerous. IMHO it should be reverted in Qt, but I doubt its going to happen and hence not worth spending time on. Since nonetheless there's the case of breaking existing apps with this change, I'd like to check other peoples opinions on adding some more logic to the kconfigdialogmanager's code so that if the user-property comes from a pure Q/KCombobox (i.e. not a subclass) and is currentIndex its ignored, otherwise its taken account. I'm not sending a review-request yet since I don't have a patch and first have to setup a kdelibs dev env anyway. So if there are objections to adding such logic I'd like to spare the time for the setup. For KDevelop we have a workaround now by setting the kcfg_property special property which again overrules the hardcoded handling for Q/KComboBox. Andreas
Re: Review Request: Add git support to kdesdk: create_tarball.rb
On Nov. 30, 2011, 6:01 p.m., Thomas Baumgart wrote: trunk/KDE/kdesdk/scripts/createtarball/create_tarball.rb, line 443 http://svn.reviewboard.kde.org/r/6842/diff/1/?file=47267#file47267line443 Is there a specific reason why you don't add the doc subdirectory for stuff fetched from git? The git modules I have checked, have the non-translated documents in the git repo and the directory already added in the CMakeLists.txt. I take it that, that is the standard :) So the documentation should actually be built. - Kåre --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6842/#review10493 --- On Nov. 29, 2011, 4:42 p.m., Kåre Särs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6842/ --- (Updated Nov. 29, 2011, 4:42 p.m.) Review request for kdelibs and Release Team. Description --- This patch adds git support to create_tarball.rb in kdesdk. The patch adds two options to the ini file. The first one (gitModule) indicates that the module is located in git and the second optional parameter (gitTag) defines which tag to use for creating the release. (there is no group for kdesdk or extragear) Diffs - trunk/KDE/kdesdk/scripts/createtarball/config.ini 1266277 trunk/KDE/kdesdk/scripts/createtarball/create_tarball.rb 1266277 Diff: http://svn.reviewboard.kde.org/r/6842/diff/diff Testing --- Thanks, Kåre Särs
Re: Review Request 101486 (Q/KComboBox related KConfigDialogManager change) breaks apps
On Wednesday 30 November 2011 21:50:41 Andreas Pakulat wrote: Hi, just came around to notice this now, but the mentioned code-change from the review request: https://git.reviewboard.kde.org/r/101486/ actually does break apps. In particular it breaks kdevelop. We have a KComboBox subclass which actually wants to store a custom string-property into the kconfig file and not an index. At the same time the combobox is not editable, but the strings are user-supplied (in a separate widget somewhere else). So storing the string is fine and actually necessary since the order is undefined over restarts. The commit 8edc1932ecc62370d9a31836dfa9b2bd0175a293 and d44186bce4670d2985fb6aba8dba59bbd2c4c77a changed the kconfigdialogmanager to first check for q/kcombobox style and then enforces using the index or the text depending on the editable-flag. The reason for that is that QCombobox in Qt 4.8 changed its behaviour and exposes its currentIndex roperty as a user-proeprty. This leads to storing the index of user-editable comboboxes instead of the text, which is wrong and even dangerous. IMHO it should be reverted in Qt, but I doubt its going to happen and hence not worth spending time on. That problem was previously reported here, thanks for reminding us :) Since nonetheless there's the case of breaking existing apps with this change, I'd like to check other peoples opinions on adding some more logic to the kconfigdialogmanager's code so that if the user-property comes from a pure Q/KCombobox (i.e. not a subclass) and is currentIndex its ignored, otherwise its taken account. I did not yet find a way to see if the user property actually comes from QComboBox, or a subclass. I guess the Qt property system has no way to find that out, but if you find a way, please share a patch. I'm not sending a review-request yet since I don't have a patch and first have to setup a kdelibs dev env anyway. So if there are objections to adding such logic I'd like to spare the time for the setup. For KDevelop we have a workaround now by setting the kcfg_property special property which again overrules the hardcoded handling for Q/KComboBox. Andreas Christoph Feck (kdepepo) KDE Quality Team
Re: Review Request: Fix Date Display in dolphin info panel
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103300/#review8628 --- kio/kfile/kfilemetadatareaderprocess.cpp http://git.reviewboard.kde.org/r/103300/#comment7294 Why do you need to load the kio4 catalog? - Sebastian Trueg On Nov. 30, 2011, 9:17 p.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103300/ --- (Updated Nov. 30, 2011, 9:17 p.m.) Review request for kdelibs and Nepomuk. Description --- The date in dolphin info panel is passed by kfilemetadatareader directly. So all the localization work should be done at the kfilemetadatareader side. There are two problems. 1. date given by nepomuk is stored as UTC time, needed to convert to localtime. 2. date localization will not work without a KComponentData. Diffs - nepomuk/utils/utils.cpp e81615a kio/kfile/kfilemetadatareaderprocess.cpp 03ff887 Diff: http://git.reviewboard.kde.org/r/103300/diff/diff Testing --- Works for me. Thanks, Xuetian Weng
Re: Review Request 101486 (Q/KComboBox related KConfigDialogManager change) breaks apps
On 30.11.11 22:06:12, Christoph Feck wrote:
On Wednesday 30 November 2011 21:50:41 Andreas Pakulat wrote:
Since nonetheless there's the case of breaking existing apps with
this change, I'd like to check other peoples opinions on adding
some more logic to the kconfigdialogmanager's code so that if the
user-property comes from a pure Q/KCombobox (i.e. not a subclass)
and is currentIndex its ignored, otherwise its taken account.
I did not yet find a way to see if the user property actually comes
from QComboBox, or a subclass. I guess the Qt property system has no
way to find that out, but if you find a way, please share a patch.
That should be doable:
int qcombouserpropindex =
QComboBox::staticMetaObject.indexOfProperty(QComboBox::staticMetaObject.userProperty().name());
int curwidgetuserpropindex =
widget-metaObject()-indexOfProperty(widget-metaObject()-userProperty().name());
if( qcombouserpropindex != -1 curwidgetuserpropindex != -1
qcombouserpropindex != curwidgetuserpropindex ) {
// user user-property
} else {
// use the q/kcombobox special code
}
Or am I overlooking something? Obviously that would need a really
verbose comment on why its done :)
Andreas
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
