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 27, 2011, 7:28 a.m.) Review request for kdelibs and David Faure. Changes --- No need to use m_bUserNameChanged flag in ::setHost anymore. 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
Re: Review Request: First part of fixes for password caching in KIO: SlaveBase::openPaswordDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101174/#review2915 --- This review has been submitted with commit 03f949d61bb556a34de194c431a034d82de95380 by Dawit Alemayehu. - Commit On April 24, 2011, 12:10 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101174/ --- (Updated April 24, 2011, 12:10 a.m.) Review request for kdelibs and David Faure. Summary --- The is the first part of a two part patch set that changes the current behavior of openPasswordDialog such that it will not automatically store the password even when the user checks the Remember password checkbox. The reason behind this change is to make sure incorrect or invalid password information is not stored in the wallet. And that can only be achieved if the ioslaves first have to verify the password information is valid by using it to gain access to the password protected server. Diffs - kio/kio/slavebase.h f8ee99a kio/kio/slavebase.cpp 6432edb Diff: http://git.reviewboard.kde.org/r/101174/diff Testing --- Thanks, Dawit
Re: [Kde-scm-interest] Re: libtagaro - kdereview [- kdegames]
On Fri, Apr 22, 2011 at 3:39 PM, Torgny Nyblom k...@nyblom.org wrote: +1 a module is a unit and should be treated as such even if the different apps are in different gits. So should I move the code to SVN then? This discussion was on kde-games-devel already, and the mass figured it's quite stupid to move code from Git to SVN while everything's generally moving in the opposite direction. Greetings Stefan
Re: Empty merges per release
On Wed, Apr 27, 2011 at 09:40:08AM +0200, Thiago Macieira wrote: I know we decided to use cherry-picks to bring commits from past versions to master, but this has caused that the 4.5 and 4.6 commits are never in the past history of master. which is the state we had with svn. it was considered good enough so far. if you want to make that git feature useful in the kde-ish cherry-pick regime, tag the stable branch points - if you want to know when a commit on a stable branch hit master, you need the extra step of following the cherry-picked from message. this could be actually scripted rather easily. I recommend at least once, after a release, that the tag is merged back into master, using strategy ours if necessary. great idea ... all cherry-picked commits duplicated (oh well, who cares, given all that scripty crap in the history), and still the risk of falsifying the history by claiming that all commits were merged while they might have not been.
Re: Empty merges per release
Em Wednesday, 27 de April de 2011, às 12:46:40, Oswald Buddenhagen escreveu: if you want to make that git feature useful in the kde-ish cherry-pick regime, tag the stable branch points - if you want to know when a commit on a stable branch hit master, you need the extra step of following the cherry-picked from message. this could be actually scripted rather easily. I recommend at least once, after a release, that the tag is merged back into master, using strategy ours if necessary. great idea ... all cherry-picked commits duplicated (oh well, who cares, given all that scripty crap in the history), and still the risk of falsifying the history by claiming that all commits were merged while they might have not been. A shadow tag might be useful then. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Empty merges per release
- Original Message - (oh well, who cares, given all that scripty crap in the history) I consider our translations very important for KDE. Would be nice if people -in general- do not consider translations and the contributors working on that as second class community members. Calling the scripty commits 'crap' is weird, really. Best, -- Tom Albers KDE Sysadmin
Re: Review Request: Allow KWidgetItemDelegate::createItemWidgets to know what index corresponds to the widgets
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101219/#review2924 --- Ship it! That's a no brainer. Please, ship it. As a side note: where you placed the comment for KDE5, please add ### and remove the note from the documentation that is generated (you could add this comment before the implementation on the cpp file). When working on KDE5 I will do a fast grep on ### and fix those issues (mostly public API that needs to be fixed). Thank you very much for the patch. - Rafael Fernández On April 24, 2011, 7:49 a.m., Eli MacKenzie wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101219/ --- (Updated April 24, 2011, 7:49 a.m.) Review request for kdelibs and Rafael Fernández López. Summary --- Ideally KWidgetItemDelegate::createItemWidgets would take a const QModelIndex argument. Unfortunately its public API, so the signature cannot be changed until KDE5. Additionally, its pure virtual so the option of adding a createItemWidgets(const QModelIndex) method won't work either. Instead, I've added a dynamic property called creatingWidgetForIndex to the delegate, that is only present while the widget pool is creating widgets. This means the only change kwidgetitemdelegate.h is documentation. The QModelIndex could be stashed on one of the d-objects and made available through an accessor, but that would mean that d-object would have to carry a QModelIndex that would be invalid almost all of the time. Another benefit is that an application where this feature is desired (for KDE 4.7) only has to include an the new version of kwidgetitemdelegatepool.cpp, which is easy to keep in sync with kdelibs. This change allows you to set a delegate for a particular column in the model, and put a type of widget in each row that works best for the data. Diffs - kdeui/itemviews/kwidgetitemdelegate.h 58dd60868f476d925f3abd53e67b22c1ed7149ac kdeui/itemviews/kwidgetitemdelegatepool.cpp b287584a594e97a091f96447386a588acb08c59e Diff: http://git.reviewboard.kde.org/r/101219/diff Testing --- In my test app I started with a model that only included 5 rows, and would only create a widget for row 2. As long as KWidgetItemDelegate::updateWidgets doesn't assume that the QListQWidget* argument has something in it, all is well. After implementing this change I caused the model to change the type of widget for each row per the index pulled out of the property, again keeping updateWidgets in sync, and have encountered no problems. Screenshots --- with no filtering http://git.reviewboard.kde.org/r/101219/s/136/ With filtering http://git.reviewboard.kde.org/r/101219/s/137/ Thanks, Eli
Re: Empty merges per release
On Wednesday 27 April 2011, John Layt wrote: On Wednesday 27 Apr 2011 17:52:32 Alexander Neundorf wrote: On Wednesday 27 April 2011, Thiago Macieira wrote: I know we decided to use cherry-picks to bring commits from past versions to master, Is this what is described here ? http://techbase.kde.org/Development/Tutorials/Git/Recipes Well, it's a recipe on how to do a cherry-pick, not official policy. I am supposed to be drafting a revision the SVN commit policy to address such issues but 1) I've been otherwise busy and 2) every time the issue of workflow is raised on k-c-d it just gets ignored or goes around in circles. I'd planned to try raise it again this weekend, we need to get a policy sorted. Our git docs are still not in good shape: Yeap, I've slowly been adding stuff to http://techbase.kde.org/Development/Git as I learn it, but help from people who actually understand Git would be welcome. http://techbase.kde.org/Development/Tutorials/Git - remove ? http://techbase.kde.org/Development/Tutorials/Git/Pushing - talks about gitorious, remove ? http://techbase.kde.org/Development/Tutorials/Git/KdeOnGit - same as above, remove ? http://techbase.kde.org/Development/Tutorials/Git/Intermediate - not much there http://techbase.kde.org/Development/Tutorials/Git/Recipes vs. http://techbase.kde.org/Development/Git/Recipes - what to do with these two ? http://techbase.kde.org/Development/Tutorials/Git/git-svn - is this still valid ? http://techbase.kde.org/Development - the git icon is broken The Tutorial is seriously outdated, I haven't had time to eitehr revise it or extract the good bits from it. I'm contemplating just hiding it for now as it would be seriously misleading to any GSoC students who try follow it. I mean, I can help with simple things. If we agree that the following pages should be gone http://techbase.kde.org/Development/Tutorials/Git http://techbase.kde.org/Development/Tutorials/Git/Pushing http://techbase.kde.org/Development/Tutorials/Git/KdeOnGit I can do that. Should I use the Delete link or something else ? I could merge the one item from http://techbase.kde.org/Development/Tutorials/Git/Recipes into http://techbase.kde.org/Development/Git/Recipes and remove the former one. What about the git-svn page ? I guess this one is ok, or ? I can also add the extra links from the original Tutorials/Git/ page to the new starting page, so they don't get lost. So, if you want me to do that, please let me know. Other than that, I don't feel qualified to actually edit content there. Alex
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/#review2926 --- kioslave/ftp/ftp.cpp http://git.reviewboard.kde.org/r/101173/#comment2507 Oh, I thought we could remove the boolean altogether. I don't really understand the way the patch works. The boolean is set in ftpOpenControlConnection and read in ftpOpenConnection? What's the relatoin between these two methods? Can't they communicate without using a more long term (= more possible side effects) boolean? Even a bool as parameter would seem better (even if it reads ugly), because the decision point and the usage point would be clear. I could be wrong because I didn't look into details, but wouldn't it work to do this all in ftpOpenControlConnection maybe? redirect, and return false? - David On April 27, 2011, 7:28 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101173/ --- (Updated April 27, 2011, 7:28 a.m.) Review request for kdelibs and David Faure. 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: Chinese Lunar Calendar Support in Kdelibs
Hi, John and Feng Huang, It's glad to see the interests on this old topic. Sorry, I forgot to update the info about relicense of ccal(at least part for chinese calendar date calculate). My change is in http://code.google.com/p/kcalendar/source/browse/trunk/README Yes, I also updated the link for ccal in it. But that is not a big issue, at least I think at that time. All my code should be redesign or rewrite in a good way, but I am not expert on it. Best Regards, Liang On 10 November 2010 16:31, John Layt johnl...@googlemail.com wrote: Unfortunately, this patch has exactly the same problems as the last one. Firstly, it is not clear that the licensing issues have been resolved. While the required ccal code has been relicensed under the LGPL (I have yet to do a full file-by-file check), it also includes copyright statements for the Lunar Outreach code for which the licensing is unclear. It was for non-commercial use only, Liang Qi said it had been relicensed but the link he gave is now dead. That code seems to just be standard algorithms from Astrinomical Algorithms by Meeus and could probably be easily replaced with something more appropriate from the LGPL libnova (which is a full implementation of Meeus). In fact, I'd be happier if the NOVAS stuff came from libnova as well if possible, but that's more from a style and ongoing maintenance point-of-view. -- http://www.qiliang.net
Re: Review Request: PATCH: Fix most of the login issues with the FTP ioslave...
On April 27, 2011, 7:58 p.m., David Faure wrote: kioslave/ftp/ftp.cpp, line 605 http://git.reviewboard.kde.org/r/101173/diff/3/?file=15471#file15471line605 Oh, I thought we could remove the boolean altogether. I don't really understand the way the patch works. The boolean is set in ftpOpenControlConnection and read in ftpOpenConnection? What's the relatoin between these two methods? Can't they communicate without using a more long term (= more possible side effects) boolean? Even a bool as parameter would seem better (even if it reads ugly), because the decision point and the usage point would be clear. I could be wrong because I didn't look into details, but wouldn't it work to do this all in ftpOpenControlConnection maybe? redirect, and return false? Ahhh m_bUserNameChanged is never set in ftpOpenControlConnection. It does get set to false in ftpCloseControlConnection. Did you mean that ? If so, that is done to reset the flag just like say m_bLoggedOn. More importantly the flag is only used between ftpLogin and ftpOpenConnection ; so it definitely can be changed into a parameter that ftpLogin sets. Come to think of it I did say I was going to remove that flag on irc yesterday didn't I ? Oh well... let me do that then... - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101173/#review2926 --- On April 27, 2011, 7:28 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101173/ --- (Updated April 27, 2011, 7:28 a.m.) Review request for kdelibs and David Faure. 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: Empty merges per release
On Wednesday 27 Apr 2011 19:59:05 Alexander Neundorf wrote: I mean, I can help with simple things. If we agree that the following pages should be gone http://techbase.kde.org/Development/Tutorials/Git http://techbase.kde.org/Development/Tutorials/Git/Pushing http://techbase.kde.org/Development/Tutorials/Git/KdeOnGit I can do that. Should I use the Delete link or something else ? Yes, please delete, just check the Further Resources are on the new page. Also delete: http://techbase.kde.org/Development/Tutorials/Git/BestPractices http://techbase.kde.org/Development/Tutorials/Git/Intermediate http://techbase.kde.org/Development/Tutorials/Git/kde-qt http://techbase.kde.org/Development/Tutorials/Git/decoding-git I could merge the one item from http://techbase.kde.org/Development/Tutorials/Git/Recipes into http://techbase.kde.org/Development/Git/Recipes and remove the former one. Delete that without copying, the workflow still needs to be decided and documented, and cherry-picking and branches already have recipes. What about the git-svn page ? I guess this one is ok, or ? Keep that and the Basics page for now, I'll harvest them later. I can also add the extra links from the original Tutorials/Git/ page to the new starting page, so they don't get lost. That would be good, thanks. So, if you want me to do that, please let me know. Other than that, I don't feel qualified to actually edit content there. Neither do I, but no-one else was doing it, so me it was :-) John.
Re: Review Request: PATCH: Fix most of the login issues with the FTP ioslave...
On April 27, 2011, 11:42 p.m., David Faure wrote: kioslave/ftp/ftp.cpp, line 367 http://git.reviewboard.kde.org/r/101173/diff/4/?file=15510#file15510line367 Sorry, I still have a question. After a username change, setHost is going to be called, right? So it will disconnect kio_ftp from the host? In that case, what's the point in doing the redirection code in this method - I thought the point was to do it after connected(), but that can't be it. So it still seems to me that the redirection code could be done inside of ftpLogin, which would simplify the code quite a lot (no bool *, in particular). But I guess I'm missing something; maybe setHost isn't called, after a redirection, so the app simply reuses the already-connected slave? In that case I don't see a better solution, indeed. #1. After the user name changed redirection, setHost will be called. But why do you think kio_ftp will be disconnected from the server ? If that is because of the closeConnection() call in setHost, then rest assured that it will not be called. Why ? Because m_user has already been updated to the new username and since nothing else will change, the if statement in setHost will never be true. #2. We want the redirection to be the same as any other redirection. The only difference being we only want to update the client with the new user name. As per response in #1, the ioslave already has updated its local copy of the user name (m_user) ; so eveything else is exactly the same as the other redirection. If you attempt to move the redirection to ftpLogin, you have to throw away your successful connection to the server, because you have to return false, i.e. not logged in to ftpOpenConnection. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101173/#review2935 --- On April 27, 2011, 9:27 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101173/ --- (Updated April 27, 2011, 9:27 p.m.) Review request for kdelibs and David Faure. 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
