[Kde-pim] Review Request: Refactoring handling IMAP responses during login phase in Akonadi
Oleg Girko
ol+kde at infoserver.ru
Mon Feb 13 20:56:15 GMT 2012
> 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.
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?
- Oleg
-----------------------------------------------------------
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 at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
More information about the kde-pim
mailing list