'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:       "Oleg Girko" <ol+kde () infoserver ! ru>
Date:       2012-02-08 18:22:26
Message-ID: 20120208182226.9000.68989 () vidsolbach ! de
[Download RAW message or body]

> On Feb. 3, 2012, 5:07 p.m., Kevin Krammer wrote:
> > kimap/loginjob.cpp, line 272
> > <http://git.reviewboard.kde.org/r/103854/diff/1/?file=48560#file48560line272>
> > 
> > I am wondering whether this is not something that is useful in more places. My \
> > guess is that other jobs have to check response codes as well. Maybe as a \
> > function in JobPrivate?
> 
> Oleg Girko wrote:
> As far as I understand from reading sources for other jobs, responses are already \
> being handled relatively uniform way either by using \
> KIMAP::Job::handleErrorReplies() method or even by not overriding \
> KIMAP::Job::handleResponse() method at all. Most of the jobs just send a single \
> command and expect "OK" response, so default behaviour is sufficient for them. 
> KIMAP::LoginJob is a special complex case because it implements a finite state \
> machine which traverses through several different states in sequence by sending \
> commands and handling their responses. Hence, default response handling is \
> insufficient for KIMAP::LoginJob, so its overriden handleResponse() method uses \
> more complex algorthm to handle all responses by itself. 
> Of course, some refactoring of other jobs could be beneficial for better \
> correctness and readability, but IMHO those changes are logically independent, so \
> they should be placed in separate commits instead of piling them up in a single \
> large change. 
> Kevin Krammer wrote:
> I only did a quick check but I found several occurences of code like this
> 
> if ( !response.content.isEmpty() && response.content[0].toString() == "+" ) // \
> continuation 
> which seems to be one of the cases handled here as well.
> 
> I am not saying that this has to be changed everywhere in this change set, just \
> that obviously it is useful in other places as well and thus makes little sense to \
> create yet another occurence of the same sharable piece of code. 
> The thing that triggered my interest was the enum declaration inside a method's \
> body and a string to enum value mapping right after. It just screamed "helper \
> function" to me. I only then checked whether it would be useful elsewhere to \
> determine where to put it. 
> Maybe something like
> static ResponseCode responseCode( const Message &response )

There is no need for such enum in other job classes.

There are 29 job classes in KIMAP namespace. All of them obviously inherit from \
KIMAP::Job class. There is the default handleResponse() method in this class which \
contains just one statement:  handleErrorReplies(response);
The KIMAP::Job::handleErrorReplies() method handles tagged responses (both OK and \
error conditions), and emits the result if there are no more pending responses for \
this job. This is sufficient for most jobs.

10 jobs do not override handleResponse() method at all. These jobs are: CloseJob, \
CopyJob, CreateJob, DeleteAclJob, DeleteJob, LogoutJob, RenameJob, SetAclJob, \
SubscribeJob, UnsubscribeJob.

17 jobs override handleResponse() method, but still use \
KIMAP::Job:handleErrorReplies() method to handle tagged responses. These jobs are: \
AppendJob, CapabilitiesJob, ExpungeJob, FetchJob, GetAclJob, GetMetaDataJob, \
GetQuotaJob, GetQuotaRootJob, IdleJob, ListJob, ListRightsJob, MyRightsJob, \
NamespaceJob, SearchJob, SelectJob, SetQuotaJob, StoreJob. Most of these jobs handle \
untagged responses only. Only 3 of them handle continuation responses: AppendJob, \
IdleJob, SearchJob. Comparison like ( response.content[0].toString() == "+" ) is \
sufficient in these 3 cases.

Only 2 jobs handle tagged responses in overridden handleResponse() method by \
themselves: LoginJob and SetMetaDataJob. Besides tagged responses, LoginJob handles \
both untagged and continuation responses, whereas SetMetaDataJob handles just \
continuation responses. Also, SetMetaDataJob handles non-OK tagged responses with \
finer granularity, making difference between "NO" and other error responses.

This means that enum ResponseCode from my change makes sense only in one module: \
LoginJob, where it is already located. There is no use for OK and ERR values outside \
of LoginJob, and continuation responses are expected by very few jobs.

Also, my plans are not as ambitious as refactoring the whole KIMAP module. I just \
want to fix the incorrect behaviour in LoginJob, and refactoring of its \
handleResponse() method is needed to prevent my fix to make its algorithm even more \
difficult to understand.

- Oleg

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

On Feb. 5, 2012, 7:02 p.m., Oleg Girko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103854/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2012, 7:02 p.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/diff
> 
> 
> Testing
> -------
> 
> Successfully tested with Dovecot IMAP server 2.0.17 using CRAM-MD5 and GSSAPI \
> authentication with unencrypted and SSL connection. 
> 
> 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] 