'Re: [Kde-pim] Review Request: Refactoring handling IMAP responses during login phase in Akonadi'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request: Refactoring handling IMAP responses during login phase in Akonadi
From:       "Kevin Krammer" <kevin.krammer () gmx ! at>
Date:       2012-02-13 21:36:56
Message-ID: 20120213213656.23533.42880 () vidsolbach ! de
[Download RAW message or body]

> On Feb. 12, 2012, 11:17 a.m., Kevin Ottens wrote:
> > kimap/loginjob.cpp, line 272
> > <http://git.reviewboard.kde.org/r/103854/diff/1/?file=48560#file48560line272>
> > 
> > I agree with the other Kevin comment in part, this logic ought to be factored \
> > out. Of course other jobs do their own thing for now, but I clearly see the need \
> > to improve that in a consistent way. 
> > So could you please factor out the logic determining the ResponseCode and the \
> > ResponseCode type itself inside of the Message class?
> 
> Oleg Girko wrote:
> As I already mentioned, refactoring the whole KIMAP module is out of scope of this \
> change and is not my goal at this moment. Here I'm solving a serious regression \
> which happened when KMail switched from kio_imap to Akonadi for handling IMAP \
> protocol. KDE 4.7 and KDE 4.8 were released since then and users are still \
> suffering from this regression. I've made this change to be non-intrusive and \
> trivially backportable to KDE 4.7 and KDE 4.8 because I want to see this change to \
> appear in next bugfix release of KDE 4.7 and KDE 4.8. Hence, this change is exactly \
> whan the description says: refactoring of KIMAP::LoginJob::handleResponse() method \
> and nothing more. There are two patches in \
> https://bugs.kde.org/show_bug.cgi?id=249992 to solve this regression, but the first \
> one makes unreadable code even more unreadable, and the second one does not work \
> for non-PLAIN authentication. My solution is more correct and improves code \
> quality. Improving code quality even more is a good task
 , but it would contradict to my goal to make an isolated change which is trivially \
backportable to KDE 4.7 and KDE 4.8.
> 
> Also, refactoring KIMAP module by adding some kind of response code enum to \
> KIMAP::Message class is a step in wrong direction. This enum would be useful to \
> differentiate response types in current situation when there is one \
> handleResponse() method which handles all response types, but this is bad practice \
> by itself. This leads to code duplication because each overriden handleResponse() \
> method has to differentiate between different response types. This problem is \
> partially hidden by KIMAP::Job::handleErrorReplies() method which handles tagged \
> responses, but this is not a solution, but rather a slight relief which reduces \
> code to be duplicated to something like if (handleErrorReplies(response) == \
> NotHandled ) { ... } in most cases, but not eliminates code duplication completely.
> Also, handleErrorReplies() method name is misleading because it handles not just \
> error responses, but all tagged responses including OK ones. And it's still \
> insufficient in two important cases. 
> To eliminate code duplication, instead of using a single virtual handleResponse() \
>                 method, there should be the following virtual methods instead:
> * handleOkResponse() for handling tagged OK responses,
> * handleNoResponse() for handling tagged NO responses,
> * handleErrResponse() for handling tagged ERR responses,
> * handleBadResponse() for handling tagged BAD responses,
> * handleUntaggedResponse() for handling untagged responses,
> * handleContinuationResponse() for handling continuation responses.
> Jobs should override only those methods where handling provided by default method \
> is not sufficient. There should be a single place in code where the type of \
> response is being determined and the appropriate method called. It could be done in \
> KIMAP::Job::handleResponse() *non-virtual* method or somewhere deeper in \
> KIMAP::SessionPrivate::responseReceived() method. And probably there should be \
> different classes representing different response types instead of single low-level \
> KIMAP::Message struct. 
> This approach eliminates code duplication provided that default methods provide the \
> right handling for common cases. And there is no place for response code enum in \
> this approach. 
> But I want to remind once again that no matter how beneficial this refactoring \
> could be, it is out of scope of this particular change. 
> Kevin Krammer wrote:
> "But I want to remind once again that no matter how beneficial this refactoring \
> could be, it is out of scope of this particular change." 
> As far as I can tell nobody asks you to do any such refactoring.
> 
> Oleg Girko wrote:
> I just want to be clear that even if I propose a direction for further refactoring, \
> this does not mean that I'm going to do it right here and right now. 
> Anyway, you've asked me to move ResponseCode enum to KIMAP::Message class, I've \
> provided justification not to do it. Can I mark this issue as fixed or dropped? 
> What we do next? Can I commit the change or should I wait for formal approval by \
> somebody? 
> Kevin Krammer wrote:
> The suggestion for putting the enum into the message class came from Kevin Ottens \
> (yeah, I know, too many Kevins :)) 
> As for comitting, I'd say you wait until the library's maintainer (which is the \
> other Kevin AFAIK) ticks the "Ship it" box on this review request. 
> Oleg Girko wrote:
> Thank you for your advise.
> 
> Concerning this particular issue opened by you, should I mark it as "fixed" or \
> "dropped"?

You can mark that one as dropped. 

- Kevin

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

On Feb. 13, 2012, 4:40 a.m., Oleg Girko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103854/
> -----------------------------------------------------------
> 
> (Updated Feb. 13, 2012, 4:40 a.m.)
> 
> 
> Review request for KDEPIM-Libraries.
> 
> 
> Description
> -------
> 
> Refactoring KIMAP::LoginJob::handleResponse() method in kimap/loginjob.cpp file to \
> make its logic more readable, straightforward and correct. As a side effect, it \
> fixes https://bugs.kde.org/show_bug.cgi?id=249992 by handling untagged CAPABILITY \
> responses more correct and uniform way. 
> This change is trivially backportable to KDE 4.7 (tested with KDE 4.7.4).
> 
> 
> This addresses bug 249992.
> http://bugs.kde.org/show_bug.cgi?id=249992
> 
> 
> Diffs
> -----
> 
> kimap/loginjob.cpp fad276d957e46fd00efa20a5f235d02a639ab2c4 
> 
> Diff: http://git.reviewboard.kde.org/r/103854/diff/
> 
> 
> Testing
> -------
> 
> Successfully tested with Dovecot IMAP server 2.0.17 using CRAM-MD5 and GSSAPI \
> authentication with unencrypted and SSL connection. Also tested with GMail's own \
> IMAP server using SSL connection and LOGIN authentication. 
> 
> Thanks,
> 
> Oleg Girko
> 
> 

_______________________________________________
KDE PIM mailing list kde-pim@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[prev in list] [next in list] [prev in thread] [next in thread] 