Re: Review Request: Make it possible to login to 2 different sites from two separate instance of same application

[Commit Hook]

---
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

[Commit Hook]

---
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

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103226/#review8619
---



kdeui/jobs/kdialogjobuidelegate.cpp


Can't see the point in going up to the mainwindow here.

In scheduler.cpp yes, but in the delegate?



kfile/kdiroperator.cpp


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


Same here, "this" will do just fine.



kfile/kfilewidget.cpp


And here too, "this" is a child of "parent" anyway.



kfile/knewfilemenu.cpp


This function is now unused, please remove it.



kio/kio/scheduler.cpp


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

[Thomas Baumgart]

---
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


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: Proper password caching when opening remote directories in KFileDialog

[Dawit Alemayehu]

---
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.


Changes
---

Updated patch based on dfaure's 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)
-

  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: Fix KDateComboBox shrinking in size after a date is entered

[Commit Hook]

---
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

[Andreas Pakulat]

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

[Kåre Särs]

> On Nov. 30, 2011, 6:01 p.m., Thomas Baumgart wrote:
> > trunk/KDE/kdesdk/scripts/createtarball/create_tarball.rb, line 443
> > 
> >
> > 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

[Christoph Feck]

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

[Sebastian Trueg]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103300/#review8628
---



kio/kfile/kfilemetadatareaderprocess.cpp


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

[Andreas Pakulat]

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

[Commit Hook]

---
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
> 
>