Re: Review Request: Proper "All files" option when using mime types as filters for KFileDialog's functions
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101075/
---
(Updated April 20, 2011, 8:50 p.m.)
Review request for kdelibs.
Changes
---
I updated my previous patch in two aspects.
(1) The code formatting has been changed to match the surrounding code.
(2) The documentation for kfiledialog.h has been rewritten to match the
changes. See function setFilter() for the main changes. Across the
documentation, the variants of "mimetype" and "mime-type" have been unified.
Summary
---
When using mimetypes instead of old-fashioned '*.txt|Text file' filters for
KFileDialog functions, there is no mimetype working like '*|All files'.
Mimetypes 'all/all' and 'all/allfiles' have no globs associated and as such do
not match any filenames. The current solution 'application/octet-stream' works
as an all-files filter, but does not behave nicely in KFileFilterCombo:
including this mimetype will render 'all supported files' to 'all files'.
The attached patch improves the situation as follows: if the developer adds a
mimetype 'all/allfiles' to his/her list of mime types for filtering in e.g.
KFileDialog::getOpenUrl's second parameter, function setMimeFilter recognizes
the request for an "all files" filter option and adds it to the list of options
in the combobox. This "all files", however, is not added to the "all support
files" option as the 'application/octet-stream' would have been. Additional
minor changes (moving some lines around "+= delim") were made in the same
function for GUI consistency reasons.
To make the patch and mimetype 'all/allfiles' working (as it has no inherent
glob like '*'), KDirLister has to be modified as well: just like
'application/octet-stream' can act as a wildcard, so does 'all/allfiles' now.
Regarding code consistency, I am not sure why 'application/octet-stream' was
used as a filter; this mimetype stands IMHO for binary bulk data instead of
'any/all files'. Maybe support for application/octet-stream should be marked as
deprecated in this situation.
If this patch does get accepted, the documentation on setMimeFilter needs to be
adopted to explain the use of 'all/allfiles'. Furthermore, I would suggest that
the documentation should suggest to prefer using mimetypes ('text/plain') over
manual globs ('*.txt|Text file') as it offers better consistency ('*.txt|Text
file' vs '*.txt|Plain Text file'), internationalization etc.
Diffs (updated)
-
kfile/kfilefiltercombo.cpp f32a0df
kio/kfile/kfiledialog.h af29a37
kio/kio/kdirlister.cpp df81dc8
Diff: http://git.reviewboard.kde.org/r/101075/diff
Testing
---
Thanks,
Thomas
Re: Review Request: Proper "All files" option when using mime types as filters for KFileDialog's functions
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101075/#review2771
---
This review has been submitted with commit
8ba86c2b6fc6f850cbc93ec59b1de6b31863e60b by David Faure.
- Commit
On April 20, 2011, 8:50 p.m., Thomas Fischer wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101075/
> ---
>
> (Updated April 20, 2011, 8:50 p.m.)
>
>
> Review request for kdelibs.
>
>
> Summary
> ---
>
> When using mimetypes instead of old-fashioned '*.txt|Text file' filters for
> KFileDialog functions, there is no mimetype working like '*|All files'.
> Mimetypes 'all/all' and 'all/allfiles' have no globs associated and as such
> do not match any filenames. The current solution 'application/octet-stream'
> works as an all-files filter, but does not behave nicely in KFileFilterCombo:
> including this mimetype will render 'all supported files' to 'all files'.
>
> The attached patch improves the situation as follows: if the developer adds a
> mimetype 'all/allfiles' to his/her list of mime types for filtering in e.g.
> KFileDialog::getOpenUrl's second parameter, function setMimeFilter recognizes
> the request for an "all files" filter option and adds it to the list of
> options in the combobox. This "all files", however, is not added to the "all
> support files" option as the 'application/octet-stream' would have been.
> Additional minor changes (moving some lines around "+= delim") were made in
> the same function for GUI consistency reasons.
>
> To make the patch and mimetype 'all/allfiles' working (as it has no inherent
> glob like '*'), KDirLister has to be modified as well: just like
> 'application/octet-stream' can act as a wildcard, so does 'all/allfiles' now.
>
> Regarding code consistency, I am not sure why 'application/octet-stream' was
> used as a filter; this mimetype stands IMHO for binary bulk data instead of
> 'any/all files'. Maybe support for application/octet-stream should be marked
> as deprecated in this situation.
>
> If this patch does get accepted, the documentation on setMimeFilter needs to
> be adopted to explain the use of 'all/allfiles'. Furthermore, I would suggest
> that the documentation should suggest to prefer using mimetypes
> ('text/plain') over manual globs ('*.txt|Text file') as it offers better
> consistency ('*.txt|Text file' vs '*.txt|Plain Text file'),
> internationalization etc.
>
>
> Diffs
> -
>
> kfile/kfilefiltercombo.cpp f32a0df
> kio/kfile/kfiledialog.h af29a37
> kio/kio/kdirlister.cpp df81dc8
>
> Diff: http://git.reviewboard.kde.org/r/101075/diff
>
>
> Testing
> ---
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: Add a GUI to configure window's title bar blend colours
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100821/#review2789 --- This review has been submitted with commit b739e3953fd2755fc3a1781479eb0b97b1dd by Jonathan Marten. - Commit On March 8, 2011, 5:23 p.m., Jonathan Marten wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/100821/ > --- > > (Updated March 8, 2011, 5:23 p.m.) > > > Review request for KDE Base Apps and kwin. > > > Summary > --- > > Some of the still available window decorations (KDE2, Keramik, Modern System, > Quartz, > Redmond) use two colours for the title bar, either as a blend or for different > parts of the bar. This patch extends the GUI in System Settings to allow the > blend colour to be configured. > > > This addresses bug 225837. > http://bugs.kde.org/show_bug.cgi?id=225837 > > > Diffs > - > > kcontrol/colors/colorscm.h 7627db8 > kcontrol/colors/colorscm.cpp 571ed86 > kcontrol/colors/colorsettings.ui efd618b > > Diff: http://git.reviewboard.kde.org/r/100821/diff > > > Testing > --- > > Built kde-workspace with these changes, verified operation of systemsettings > and KDE2 window decoration. > > > Screenshots > --- > > kcontrol screenshot > http://git.reviewboard.kde.org/r/100821/s/93/ > > > Thanks, > > Jonathan > >
Re: Review Request: Do not preserve username information on redirection in KIO
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101153/#review2799 --- Ship it! No objections, although I don't have the big picture about this. - David On April 21, 2011, 2:50 a.m., Dawit Alemayehu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/101153/ > --- > > (Updated April 21, 2011, 2:50 a.m.) > > > Review request for kdelibs and David Faure. > > > Summary > --- > > This patch completely removes the automatic restoration of the user name from > the original url to the redirection url at the job level. Even though I tried > to lookup why this was done a long time ago, I have not been able to decipher > the reason behind it. I could only guess that it was probably done as a > workaround for the deficiencies of some unknown ioslave that was not doing > the right thing when constructing the redirection url. > > Unfortunately this has now come in full circle to cause problems when > attempting to fix kio_ftp's login related bug reports. Namely I wanted to fix > the problem where the user typed a username as part of the ftp url and later > on either changed it or choose to login anonymously when prompted with a > password dialog. Right now the client never gets updated even when the user > chose to use a different username in the password dialog. IOW, the username > reflected in the typed-in url is different from the one the user used to > login. You can guess what happens when the user then clicks on a folder or > file after logging in. > > The process I chose to use to update the client is to cause a redirection and > that works fine so long as the redirection url contains a proper username. > Otherwise, the redirection handling slots in KIO::Job automatically re-insert > the username from the old url into the new redirection url. > > > Diffs > - > > kio/kio/job.cpp 004b4c9 > > Diff: http://git.reviewboard.kde.org/r/101153/diff > > > Testing > --- > > > Thanks, > > Dawit > >
libtagaro -> kdereview [-> kdegames]
Hi, what is the technical procedure for moving libtagaro.git to kdereview? I think sysadmins need be informed, and hope that those are reading here. Assuming that this goes well: I hereby propose to move libtagaro to the kdegames module after the usual review period. For the time being, because kdegames has not yet moved from SVN, this would create a module that is split between SVN and Git, but a similar situation exists with kdegraphics, so I hope this is not a problem. In any event, scm-interest is CCd if anyone wants to discuss this. Rationale: libtagaro intends to replace libkdegames in the long run. libkdegames was developed long before such important technologies as QGraphicsView, QML and OCS, which define how our games work now or in the near future. Therefore, the scope of the support library that is libkdegames changes rapidly. Currently, libtagaro contains 1. an advanced version of KGameRenderer from libkdegames, which adds some further optimization opportunities and flexibility 2. a simple sound effects API which, if possible, uses OpenAL to reduce playback latency 3. some basic layouting components for QGraphicsView-based games Not much of this is new in the kdegames codebase. Number 2 is already used by Granatier (by means of a static source copy), number 3 is already used by Kolf (in the same way). Number 1 is, as I said, heavily based on KGameRenderer which is already used by about a dozen games. I want to add more components, especially to accommodate the needs of mobile form factors, but doing so will be much easier when I can rely on the games having a hard dependency on it. Previously, I planned to make libtagaro a private library inside the module, but this approach is unpractical while the rest of kdegames is still in SVN, or when kdegames moves to Git with a split repository layout. I therefore plan to install headers publicly, but explicitly state that there is no API/ABI stability guarantee for the time being, i.e. SO versions will likely be bumped often over the course of the next few 4.x releases. I conclude my explanations with a braindump of what needs to be done: * TagaroAudio falls back to Phonon when OpenAL is not available, but the Phonon backend is not yet implemented. (Mathias Kraus of Granatier helps with that.) * No CMake find-script etc. is installed at the moment. AFAIK it would be best to include this with the rest of the kdegames module. Also, it is probably not state-of-the-art to detect libsndfile through pkgconfig. Granatier has a FindSndfile.cmake which I plan to copy. * I have not checked Krazy output for quite a while, though I think it was empty some two months ago. Greetings Stefan
Re: libtagaro -> kdereview [-> kdegames]
On Thu, Apr 21, 2011 at 11:56 PM, Stefan Majewsky wrote: > Hi, > > what is the technical procedure for moving libtagaro.git to kdereview? > I think sysadmins need be informed, and hope that those are reading > here. Project moved to KDE Review. Regards, Ben Cooksley KDE Sysadmin > > Assuming that this goes well: I hereby propose to move libtagaro to > the kdegames module after the usual review period. For the time being, > because kdegames has not yet moved from SVN, this would create a > module that is split between SVN and Git, but a similar situation > exists with kdegraphics, so I hope this is not a problem. In any > event, scm-interest is CCd if anyone wants to discuss this. > > Rationale: libtagaro intends to replace libkdegames in the long run. > libkdegames was developed long before such important technologies as > QGraphicsView, QML and OCS, which define how our games work now or in > the near future. Therefore, the scope of the support library that is > libkdegames changes rapidly. > > Currently, libtagaro contains > 1. an advanced version of KGameRenderer from libkdegames, which adds > some further optimization opportunities and flexibility > 2. a simple sound effects API which, if possible, uses OpenAL to > reduce playback latency > 3. some basic layouting components for QGraphicsView-based games > > Not much of this is new in the kdegames codebase. Number 2 is already > used by Granatier (by means of a static source copy), number 3 is > already used by Kolf (in the same way). Number 1 is, as I said, > heavily based on KGameRenderer which is already used by about a dozen > games. > > I want to add more components, especially to accommodate the needs of > mobile form factors, but doing so will be much easier when I can rely > on the games having a hard dependency on it. Previously, I planned to > make libtagaro a private library inside the module, but this approach > is unpractical while the rest of kdegames is still in SVN, or when > kdegames moves to Git with a split repository layout. I therefore plan > to install headers publicly, but explicitly state that there is no > API/ABI stability guarantee for the time being, i.e. SO versions will > likely be bumped often over the course of the next few 4.x releases. > > I conclude my explanations with a braindump of what needs to be done: > * TagaroAudio falls back to Phonon when OpenAL is not available, but > the Phonon backend is not yet implemented. (Mathias Kraus of Granatier > helps with that.) > * No CMake find-script etc. is installed at the moment. AFAIK it would > be best to include this with the rest of the kdegames module. Also, it > is probably not state-of-the-art to detect libsndfile through > pkgconfig. Granatier has a FindSndfile.cmake which I plan to copy. > * I have not checked Krazy output for quite a while, though I think it > was empty some two months ago. > > Greetings > Stefan >
Re: libtagaro -> kdereview [-> kdegames]
Hi, On Thursday 21 April 2011, Stefan Majewsky wrote: > Hi, > > what is the technical procedure for moving libtagaro.git to kdereview? > I think sysadmins need be informed, and hope that those are reading > here. Any chance it could get a name which makes you kind of understand what it is ? libkdegames was clear, and since the "it must have a K" is gone, we have more and more stuff with weird names which say absolutely nothing to somebody who is not involved. How about simply libkdegames2 ? (e.g. libpng, libjpeg, libz, libxml2, etc. are also quite "boring" names, but you get an idea what they are good for) ... > I conclude my explanations with a braindump of what needs to be done: > * TagaroAudio falls back to Phonon when OpenAL is not available, but > the Phonon backend is not yet implemented. (Mathias Kraus of Granatier > helps with that.) > * No CMake find-script etc. is installed at the moment. AFAIK it would > be best to include this with the rest of the kdegames module. Also, it > is probably not state-of-the-art to detect libsndfile through > pkgconfig. Granatier has a FindSndfile.cmake which I plan to copy. I'll have a look at this on Monday or so. Alex
Re: Review Request: Do not preserve username information on redirection in KIO
> On April 21, 2011, 8:18 a.m., David Faure wrote: > > No objections, although I don't have the big picture about this. Its main purpose is to make the scenario below possible as part of the ftp login fixes I am working on under https://git.reviewboard.kde.org/r/100873/ 1.) User enters a ftp url, ftp://f...@foobar.com 2.) Gets prompted to enter the password by kio_ftp. 3.) Changes mind and clicks on the "Anonymous" check box and presses OK. Now the user name in the client needs to be updated to blank since the user chose to login anonymously. That is what this patch makes possible. It allows us to send a redirected without any user name, even though the original typed-in url contained one. Hope that clarifies things. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101153/#review2799 --- On April 21, 2011, 2:50 a.m., Dawit Alemayehu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/101153/ > --- > > (Updated April 21, 2011, 2:50 a.m.) > > > Review request for kdelibs and David Faure. > > > Summary > --- > > This patch completely removes the automatic restoration of the user name from > the original url to the redirection url at the job level. Even though I tried > to lookup why this was done a long time ago, I have not been able to decipher > the reason behind it. I could only guess that it was probably done as a > workaround for the deficiencies of some unknown ioslave that was not doing > the right thing when constructing the redirection url. > > Unfortunately this has now come in full circle to cause problems when > attempting to fix kio_ftp's login related bug reports. Namely I wanted to fix > the problem where the user typed a username as part of the ftp url and later > on either changed it or choose to login anonymously when prompted with a > password dialog. Right now the client never gets updated even when the user > chose to use a different username in the password dialog. IOW, the username > reflected in the typed-in url is different from the one the user used to > login. You can guess what happens when the user then clicks on a folder or > file after logging in. > > The process I chose to use to update the client is to cause a redirection and > that works fine so long as the redirection url contains a proper username. > Otherwise, the redirection handling slots in KIO::Job automatically re-insert > the username from the old url into the new redirection url. > > > Diffs > - > > kio/kio/job.cpp 004b4c9 > > Diff: http://git.reviewboard.kde.org/r/101153/diff > > > Testing > --- > > > Thanks, > > Dawit > >
Re: Review Request: Do not preserve username information on redirection in KIO
On Thursday 21 April 2011 16:00:34 Dawit Alemayehu wrote: > Its main purpose is to make the scenario below possible as part of the ftp > login fixes I am working on under > https://git.reviewboard.kde.org/r/100873/ Great, I've been watching you doing much appreciated work on wading through and fixing kio bugs, a module that hasn't seen much love previously. Thanks!
Re: Review Request: Do not preserve username information on redirection in KIO
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101153/#review2805 --- This review has been submitted with commit 98a285a65665d42f45e9460f5b150cc84c0e9b86 by Dawit Alemayehu. - Commit On April 21, 2011, 2:50 a.m., Dawit Alemayehu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/101153/ > --- > > (Updated April 21, 2011, 2:50 a.m.) > > > Review request for kdelibs and David Faure. > > > Summary > --- > > This patch completely removes the automatic restoration of the user name from > the original url to the redirection url at the job level. Even though I tried > to lookup why this was done a long time ago, I have not been able to decipher > the reason behind it. I could only guess that it was probably done as a > workaround for the deficiencies of some unknown ioslave that was not doing > the right thing when constructing the redirection url. > > Unfortunately this has now come in full circle to cause problems when > attempting to fix kio_ftp's login related bug reports. Namely I wanted to fix > the problem where the user typed a username as part of the ftp url and later > on either changed it or choose to login anonymously when prompted with a > password dialog. Right now the client never gets updated even when the user > chose to use a different username in the password dialog. IOW, the username > reflected in the typed-in url is different from the one the user used to > login. You can guess what happens when the user then clicks on a folder or > file after logging in. > > The process I chose to use to update the client is to cause a redirection and > that works fine so long as the redirection url contains a proper username. > Otherwise, the redirection handling slots in KIO::Job automatically re-insert > the username from the old url into the new redirection url. > > > Diffs > - > > kio/kio/job.cpp 004b4c9 > > Diff: http://git.reviewboard.kde.org/r/101153/diff > > > Testing > --- > > > Thanks, > > Dawit > >
Re: libtagaro -> kdereview [-> kdegames]
On Thu, Apr 21, 2011 at 06:56, Stefan Majewsky wrote: > Hi, > > what is the technical procedure for moving libtagaro.git to kdereview? > I think sysadmins need be informed, and hope that those are reading > here. > > Assuming that this goes well: I hereby propose to move libtagaro to > the kdegames module after the usual review period. For the time being, > because kdegames has not yet moved from SVN, this would create a > module that is split between SVN and Git, but a similar situation > exists with kdegraphics, so I hope this is not a problem. In any > event, scm-interest is CCd if anyone wants to discuss this. This doesn't make sense to me. If you want to be part of kdegames, you should join it where it is. If I were you I would just hold off. It's way too confusing to split modules between VCS systems and I don't think it should be allowed (*cough* kdegraphics *cough*). Ian
Review Request: PATCH: Fix most of the login issues with the FTP ioslave...
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101173/ --- Review request for kdelibs. Summary --- The attached patch addresses most of the FTP login related problems and is a replacement for the previous review request https://git.reviewboard.kde.org/r/100873/. Here are all the changes in this patch: - Show the "Remember password" checkbox even after the failure of the first login attempt. [Bug:25] - Always check for cached password before trying to login anonymously unless the "TryAnonymousLoginFirst" flag was set in kio_ftprc. [Bug: 99686, 143488, 124675] - Avoid sending the "anonymous" username so it will not be used in the key used to store the password in kwallet. - When a url contains a username, but the user chooses to login with a different username in the password dialog, then use redirection to update the client of the change. - Store password information in persistent storage if and only if the user checked the "Remember password" checkbox. This addresses bugs 99686, 124675, 143488, and 25. http://bugs.kde.org/show_bug.cgi?id=99686 http://bugs.kde.org/show_bug.cgi?id=124675 http://bugs.kde.org/show_bug.cgi?id=143488 http://bugs.kde.org/show_bug.cgi?id=25 Diffs - kioslave/ftp/ftp.h 4ccdd4c kioslave/ftp/ftp.cpp f7db42b Diff: http://git.reviewboard.kde.org/r/101173/diff Testing --- - Attempt to login with incorrect username and validate the "Remember password" is actually shown again. - Corrected the username information from the password dialog to ensure the client is updated properly about the password change. - Clicked on the "Remember password" to store password in persistent storage and retry logging into the same server at a later point. Thanks, Dawit
Re: Review Request: PATCH: Fix most of the login issues with the FTP ioslave...
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101173/ --- (Updated April 22, 2011, 4:13 a.m.) Review request for kdelibs. Summary --- The attached patch addresses most of the FTP login related problems and is a replacement for the previous review request https://git.reviewboard.kde.org/r/100873/. Here are all the changes in this patch: - Show the "Remember password" checkbox even after the failure of the first login attempt. [Bug:25] - Always check for cached password before trying to login anonymously unless the "TryAnonymousLoginFirst" flag was set in kio_ftprc. [Bug: 99686, 143488, 124675] - Avoid sending the "anonymous" username so it will not be used in the key used to store the password in kwallet. - When a url contains a username, but the user chooses to login with a different username in the password dialog, then use redirection to update the client of the change. - Store password information in persistent storage if and only if the user checked the "Remember password" checkbox. This addresses bugs 99686, 124675, 143488, and 25. http://bugs.kde.org/show_bug.cgi?id=99686 http://bugs.kde.org/show_bug.cgi?id=124675 http://bugs.kde.org/show_bug.cgi?id=143488 http://bugs.kde.org/show_bug.cgi?id=25 Diffs (updated) - kioslave/ftp/ftp.h 4ccdd4c kioslave/ftp/ftp.cpp f7db42b Diff: http://git.reviewboard.kde.org/r/101173/diff Testing --- - Attempt to login with incorrect username and validate the "Remember password" is actually shown again. - Corrected the username information from the password dialog to ensure the client is updated properly about the password change. - Clicked on the "Remember password" to store password in persistent storage and retry logging into the same server at a later point. Thanks, Dawit
Review Request: PATCH: Prevent SlaveBase::openPasswordDialog from automatically storing password information
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101174/ --- Review request for kdelibs. Summary --- The attached patch changes the current behavior of openPasswordDialog such that it will not automatically store the password if the user checked the "Remember password" checkbox. This prevents the problem of storing the password information before the ioslaves had a chance to test whether or not the credentials can be used to successfully authentication against the server. IOW, it avoid the storage of invalid or incorrect password information. Diffs - kio/kio/slavebase.h f8ee99a kio/kio/slavebase.cpp 6432edb Diff: http://git.reviewboard.kde.org/r/101174/diff Testing --- Thanks, Dawit
