[Kde-pim] Re: Review Request: KIMAP::Session should timeout when opening a connection

[Kevin Ottens]

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

Ship it!


Very good. That said there's a minor whitespace nitpick to fix please. But no need to go for a second round of review for something that trivial. ;-)


kimap/session.cpp
<http://git.reviewboard.kde.org/r/100999/#comment1976>

    Please add an empty line before that one.


- Kevin


On April 1, 2011, 3:42 a.m., Gregory Schlomoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100999/
> -----------------------------------------------------------
> 
> (Updated April 1, 2011, 3:42 a.m.)
> 
> 
> Review request for KDEPIM-Libraries and Kevin Ottens.
> 
> 
> Summary
> -------
> 
> This patch makes sure the KIMAP::Session will timeout while opening a new connection.
> 
> The timeout could occur in two places:
> 1. While establishing the network connection (ie: takes longer than expected to resolve the host, etc...)
> 2. Once a valid network connection has been established, but the server doesn't sends anything (ie: not an IMAP server, for example)
> 
> === Signaling the timeout ===
> 
> As of now, KIMAP::Session only has the connectionLost() signal to report connection errors. This is both misleading and inconsistent:
> 
> - misleading, because it is also emitted in the event of a connection failure, even when we weren't previously connected, and no "loss" actually occurred.
> - inconsistent, because as it is implemented now, it doesn't always follow this misleading behavior.
> 
> === Solution ===
> 
> We have two options:
> 
> 1. Always use connectionLost() for all errors (connection failed, timeouts, etc..)
> 2. Send connectionFailed() if we couldn't establish a connection to a valid IMAP server, and use stateChanged() to detect a loss of such connection. 
> 
> In scenario 2, the existing connectionLost() behavior doesn't changes, and applications must upgrade to the new API to get the benefits of this patch.
> 
> I propose the latter solution in this review request, which seems better, but I can switch to the former if needed.
> 
> 
> Diffs
> -----
> 
>   kimap/session.h 9f30937 
>   kimap/session.cpp d320b78 
>   kimap/tests/testsession.cpp 743d7d0 
> 
> Diff: http://git.reviewboard.kde.org/r/100999/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gregory
> 
>

_______________________________________________
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/