[Kde-pim] Proposal for a solution for bug 77682

[Thomas McGuire]

Hi,

Thank you for your patch.

I think you mean bug 77862 instead (kio_imap hangs when connection status 
changes, one of the most popular KMail bugs)

On Friday 07 May 2010 18:27:53 Guy Maurel wrote:
> The bug reports that the problem occurs as the network changes.
> No part of imap within kmail takes care of this.
> 
> Use the NetworkManager. If your distribution hasn't it, have a look at:
>   git clone git://git.freedesktop.org/git/NetworkManager/NetworkManager.git
> 
> I propose to connect to the "Solid::Networking::notifier",
> which sends informations about the network as described at:
>   kdelibs/solid/solid/networking.h:42
>     /**
>      * Describes the state of the networking system
>      */
>     enum Status {
>         Unknown, /**< the networking system is not active or unable to
>                                 report its status - proceed with caution */
>         Unconnected,/**< the system is not connected to any network */
>         Disconnecting, /**< the system is breaking the connection */
>         Connecting, /**< the system is not connected to any network */
>         Connected /**< the system is currently connected to a network */
>     };
> 
> Create a new slot within "kmkernel".
> Make the connect at "void KMKernel::init()".
> 
> The new slot
>   "void KMKernel::slotNetworkStatusHasChanged( Solid::Networking::Status
> status)" gets the changes of the network.
> 
> If "Solid::Networking::Unconnected" occurs, we check all the accounts for
> "KAccount::Imap".
> 
> Get the slave address
>   lmSlave = imapAcct->slave();
> and the pid of the slave
>   pid = lmSlave->slave_pid();
> prepare the kill-command
>   killCommand = "kill "+ s_pid;
> and let it works
>   system( killCommand.toAscii());
> 
> Test and what appens:
> After having a connection to the imap-server we have the processes:
>   3996 19:00 pts/1 00:00:06 kmail --nofork
>   4001 19:00 ?     00:00:00 kdeinit4: kio_imap4 [kdeinit] imap
> local:/tmp/ksocket-guy-kde
> 
> as we unplug (to test it) the ethernet (after a maximum of 60 seconds) we
> get kmail(3996) KMKernel::slotNetworkStatusHasChanged: Networking is now:
> Unconnected kmail(3996) KMKernel::slotNetworkStatusHasChanged:
> imapAcct->slave: KIO::Slave(0xbc3960) kmail(3996)
> KMKernel::slotNetworkStatusHasChanged: slave_pid: 4001 kmail(3996)
> KMKernel::slotNetworkStatusHasChanged: kill-command "kill 4001"
> 
> as the process is killed, the slave get an input
>   kmail(3996)/kio (Slave) KIO::Slave::gotInput: slave died pid =  4001
>   kmail(3996)/kio (Scheduler) KIO::SchedulerPrivate::disconnectSlave:
> _disconnectSlave(  KIO::Slave(0xbc3960) ) kmail(3996)/kdeui
> (KNotification) KNotificationManager::notificationClosed: 234
> 
> kmail is now able to go on for a new imap-connection after the network is
> on again.

The general idea of using Solid is good.
KMail already has some online/offline status handling, and I think Solid 
should be integrated with that.
See the onlineStatusChanged() signal of KMKernel. The main widget connects to 
that and changes the state of the "Work online/offline" action accordingly.

So I think the stopNetworkJobs()/resumeNetworkJobs() should be called when the 
Solid network status changes.

Right now, stopNetworkJobs() doesn't actually change the network status. I 
think simply calling AccountManager::cancelMailCheck() might already be enough 
for this.  Can you check if that works?

So I'd like the patch to be changed to take the above into account.
Some other comments on the patch:
- Declare variables as late as possible. In the patch, they are declared at 
  the beginning of the function. It is generally better to only declare them 
  when actually using them. This helps readability and reduces the chance of 
  using uninitialized variables
- Disconnected IMAP is not covered, only online IMAP
- The cases inside the switch miss break statements
- calling system() with a kill command for the slave is not a good idea, that
  is much to brutal. Also, it is not portable, for example it won't work on 
  Mac or Windows. I'd really prefer to use AccountManager::cancelMailCheck 
  instead, if that works.

I'm a bit uneasy about committing such a big patch to the 4.4 branch. However, 
if it fixes the bug and is tested by multiple persons before committing, it 
can go in, as many users are waiting for the fix for this particular bug.

Luckily, this bug does not affect KMail trunk, since we don't use KIO slaves 
there for IMAP :)

So please take the above suggestions into account and improve the patch. 
Thanks.

Regards,
Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20100507/917fe213/attachment.sig>
-------------- next part --------------
_______________________________________________
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/