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

[Lamarque Vieira Souza]

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



applet/nmpopup.cpp
<http://git.reviewboard.kde.org/r/100835/#comment1559>

    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.



applet/nmpopup.cpp
<http://git.reviewboard.kde.org/r/100835/#comment1562>

    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.



applet/nmpopup.cpp
<http://git.reviewboard.kde.org/r/100835/#comment1560>

    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.


- Lamarque Vieira


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/20110310/497a7ef7/attachment-0001.htm