Review Request: Updated code to fix "Enable ..." checkbox handling and simplify the code.

[Lamarque Vieira Souza]

> On March 10, 2011, 6:44 p.m., Lamarque Vieira Souza wrote:
> > applet/nmpopup.cpp, line 530
> > <http://git.reviewboard.kde.org/r/100835/diff/1/?file=10903#file10903line530>
> >
> >     I did more changes in 591a85e2c62e10f2792517343f6fdd6316a933b6 to do almost all you have done in this review request. But I prefer to keep things simple: m_rfCheckBox and m_wwanCheckBox are visible if there is at least one interface of that type; it does not matter if Networking is enabled or not. m_rfCheckBox and m_wwanCheckBox are enabled (can be checked) if their respective hardware attribute is enabled.
> 
> Jirka Klimes wrote:
>     Here instead of Solid::Control::NetworkManager::isNetworkingEnabled() should probably be 'status != asleep', but Solid doesn't have an ASLEEP state (I think it should be added). But isNetworkingEnabled() is sufficient in the meantime for this, I think. This is basically what nm-applet does. We probably down want to change WiFi and WWAN state when NM is sleeping. 
>     
>     BTW, m_rfCheckBox could be renamed to m_wifiCheckBox or something.

I will rename m_rfCheckBox to m_wifiCheckBox.


> On March 10, 2011, 6:44 p.m., Lamarque Vieira Souza wrote:
> > applet/nmpopup.cpp, line 601
> > <http://git.reviewboard.kde.org/r/100835/diff/1/?file=10903#file10903line601>
> >
> >     I have implemented b1c517b2b42a62990b6f9dd5b96eb79659e9953b to fix bug 259375, so unless you can confirm that bug will not reappear without the readConfig/saveConfig calls I think it is better we keep readConfig/saveConfig. During my tests here even without the redundant DBus calls NM seems to disable wireless when it does to sleep, which is what happens when my notebook is suspending. But NM enables wireless even though that is not what the user wants. NM should keep wireless disabled if it was disabled by the user before the suspend.
> 
> Jirka Klimes wrote:
>     Actually there used to be a bug in NM that it doesn't remember user preference on WiFi state over suspend/resume cycle. But it has been fixed quite a long while ago in both master and NM_0_8 branches upstream. I would rather encourage users to use up-to-date software than doing workarounds.
>     The related commits in NM master are fa70542c618665cf203a2b71fa0e504f759f7902, e86ef05d84749c5a15d7bcf30f78056ca205489c (and maybe others).
>     See https://bugzilla.gnome.org/show_bug.cgi?id=624479. So bug 259375 is fixed (there is probably old NM version involved).
>     What distro and NM version do you have?
>     
>

I use Gentoo with networkmanager-0.8-r1. According the ebuild's changelog that ebuild version was created in 30 Mar 2010, which is older than gnome's bug report 624479. We cannot force distributions to upgrade networkmanager, so I will keep the workaround.


> On March 10, 2011, 6:44 p.m., Lamarque Vieira Souza wrote:
> > applet/nmpopup.cpp, line 620
> > <http://git.reviewboard.kde.org/r/100835/diff/1/?file=10903#file10903line620>
> >
> >     I prefer to do not send DBus calls in the "EnabledChanged" slots instead of disabling signals everytime we need to change this attribute. I really do not like to have to remember to do something before doing other thing. The real problem is not emiting the signal, the problem is calling set{Networking,Wireless,Wwan}Enabled unneedlessly in the checkboxes enabled's slots. I already fixed that in commit 591a85e2c62e10f2792517343f6fdd6316a933b6.
> 
> Jirka Klimes wrote:
>     As for <NM state> != <new checkbox state> condition before doing setWirelessEnabled(), it will do no harm, but is not necessary, because we should call this function just when we toggle the checkbox and in that case the condition is always true (the checkbox is in sync with NM daemon state).
>     Moreover you still contact NM with Solid::Control::NetworkManager::isWirelessEnabled(). So you don't save DBus calls, but add instead.
>     
>     Actually, these are two different approaches, we have.
>     I see it this way, have clearly defined roles:
>     wirelessEnabledToggled() - is a slot for toggled signal. It should only be called when user toggles the box. So just send the flipped condition to NM.
>     managerWirelessEnabledChanged() - this reacts on external changes and should update the box accordingly. But it should *not* call NM and not to call wirelessEnabledToggled(), so that we have clear distinction what each function does. However setChecked() issues 'toggled', so we need to block the signal. The alternative approach to blocking signals could be to use 'clicked' instead of  'toggled' for wirelessEnabledToggled() (not sure if it works for checkboxes).
>

The checkbox must be in sync, that is idea. I meant "do not send set{Networking,Wireless,Wwan}Enabled DBus calls".

The toggled signal is issued everytime the checkbox changes its tick mark, that can happen when 1. someone clicks on it, 2. someones uses the keyboard's space bar when the checkbox has the focus or when 3. another object calls the checkbox's setChecked method.

Your patch covers #1 and #2 and explicit ignores #3. The Toggled slots should react to any changes in the toggle state, that change coming from user or NetworkManager. That is why the managerNetworkingEnabledChanged slot calls m_rfChexkBox->setChecked. What was wrong is calling set{Networking,Wireless,Wwan}Enabled inside the manager{Networking,Wireless,Wwan}EnabledChanged or {networking,wireless,wwan}EnabledToggled slots. The way you did everytime we want to call setChecked we need to disable signals first and re-enabled them afterwards. I think that is error prone if someone someday changes the code and forget to do that. Anyway, it is logical that when managerWirelessEnabledChanged is issued it should be interpreted as like the user has clicked on the checkbox. Your code changes that interpretation.


- Lamarque Vieira


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


On March 10, 2011, 4:10 p.m., Jirka Klimes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100835/
> -----------------------------------------------------------
> 
> (Updated March 10, 2011, 4:10 p.m.)
> 
> 
> Review request for Network Management.
> 
> 
> Summary
> -------
> 
> This patch fixes "Enable ..." checkbox handling and simplify the code.
> 
> There was a bug that when Wifi was rfkilled by a hardware switch,
> knm unchecked the "Enable wireless" checkbox, but it also (erroneously)
> issued 'disable wireless' command, which caused NM to store 'wireless
> disabled' state as user preference. Then after enabling hardware switch
> WiFi stayed disabled.
> This is now fixed and the code is also simplified a bit.
> 
> Would you review please?
> 
> For Lamarque:
> I think that b1c517b2b42a62990b6f9dd5b96eb79659e9953b (saving config) is not necessary and may be contra-productive.
> NM itself stores user preference and there's no need applets do that. If there are more applets they will step on their toes.
> And in NM 0.9 will be perfectly possible to have more applets running.
> Nonetheless, thanks for your work on knm!
> 
> 
> Diffs
> -----
> 
>   applet/nmpopup.cpp 282299b 
> 
> Diff: http://git.reviewboard.kde.org/r/100835/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jirka
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110311/e24e695c/attachment.htm