[Kde-pim] Review Request: making kmail network-aware

[Martin Bednár]

Le Thursday 01 of September 2011 13:20:46 Andras Mantia a écrit :
> Martin Bednár wrote:
> > Sure, but the point was that when I was using ==Solid::Connected,
> > isOffline returned true for all Solid::Status except Connected, which
> > made me see the "askToGoOnline" window a lot. Now We check for
> > !=Unconnected || !=Disconnecting. Solid apparently being smart, uses
> > the Unknown status when it doesn't know, which I think is why it works
> > on your setup (and mine too). I admit I hadn't realized that when I
> > wrote the second patch. The thing here is now that the window will be
> > shown when KMail tries checking for mail eg the "Check Mail" button
> > when the network is Unconnected || Disconnecting (checked for in
> > isOffline), which is unwanted because kmail can't do anything with the
> > global network state.
> 
> I admit, I'm confused. :)

Sorry, I wrote that a bit in a hurry, trying to get ideas through as they 
came. Will not do that any more...

> 
> You added the check fo ==Solid::Connected originally, which is wrong, as
> Unknown might also mean that it is connected. That's what I'm corrected and
> what you do in line 940 of the patch:
> -  if ( Solid::Networking::status()==Solid::Networking::Connected ) {
> +  if (  ( Solid::Networking::status() != Solid::Networking::Unconnected )
> 
> || ( Solid::Networking::status() != Solid::Networking::Disconnecting ) )
> || {
> 
> This part is fine.
> 
> The other part is not fine:
> -  if ( kmkernel->isOffline() ) {
> +  if ( GlobalSettings::self()->networkState() ==
> GlobalSettings::EnumNetworkState::Offline ) {
>      s_askingToGoOnline = true;
>      int rc =
>      KMessageBox::questionYesNo( KMKernel::self()->mainWin(),
> 
> 
> if you look at the kmkernel->isOffline implementation it looks like this:
> bool KMKernel::isOffline()
> {
>   if ( ( GlobalSettings::self()->networkState() ==
> GlobalSettings::EnumNetworkState::Offline ) ||
>        ( Solid::Networking::status() == Solid::Networking::Unconnected ) ||
> ( Solid::Networking::status() == Solid::Networking::Disconnecting ) )
>     return true;
>   else
>     return false;
> }
> 
> 
> The first part of the if statment is the same as the change you added. So
> when GlobalSettings::self()->networkState() ==
> GlobalSettings::EnumNetworkState::Offline is true, isOffline() returns true,
> WITHOUT checking the solid status (the rest of the if statement is not
> evaluated at all). 

That I understand. And this is a nice performance benefit.

> That one is checked only if GlobalSettings::self()-
> >networkState() returns something else, but Offline.
> 
> That's why I said, your change is not needed, as it doesn't really change
> the behavior of the application. 

Now please stop me if I go wrong somewhere...
I disagree : if  GlobalSettings::self()->networkState() == 
GlobalSettings::EnumNetworkState::Offline returns false, then the rest of the 
isOffline() condition is evaluated. 
And if the rest "( Solid::Networking::status() == 
Solid::Networking::Unconnected ) || ( Solid::Networking::status() == 
Solid::Networking::Disconnecting )" returns true, then isOffline returns true 
and the user gets asked to go online, which is IMO unwanted behavior since 
KMail can't do anything with the system network status. 

I tested this on my setup in the following way : 
I added Solid::Networking::status() == Solid::Networking::Unknown into the 
evaluated expression, knowing that that is the state of the networking at 
startup. The askToGoOnline dialog appeared. (I haven't been able to make Solid 
of my development setup see correctly the networking status). If at KMail 
startup the system status is Unconnected || Disconnecting, the dialog will 
show too.

I hope I explained clearly why I want to make askToGoOnline() only evaluate 
the internal KMail setup to show the dialog, and then evaluate the system 
network status and return based on that. 
This function is indeed used the same way as isOffline() in exactly 2 places 
(lines 483 and 963), so it needs similar funtionality.

> 
> I will commit the first part of the patch.
> If you consider to help KDE/KDEPIM more in the future, you can easily apply
> for a developer account.

I think I will :)

Regards, 

Martin

PS : just FYI, I will be unavailable for the next couple of days. I expect to 
return tuesday.
-------------- 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/20110901/9f23a404/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/