[Kde-hardware-devel] QtNetworkManager (aka libnm-qt) pending issues

[Jan Grulich]

Hi,

thanks for the first round of review. I've made some changes, see below:

On 03/24/2013 08:04 PM, Lamarque V. Souza wrote:
>
> Hi,
>
> Jan Grulich is working on implementing settings support in 
> QtNetworkManager [1]. His code is in the settings branch and he asked 
> me to take a look at the code. I am sending this e-mail to everybody 
> because some of the issues spotted below can be usefull for the other 
> people working on Plasma NM/QtNetworkManager/QtModemManager. Here are 
> my findings/comments:
>
> . Do not to initialize QString class members with QString(), this 
> saves a QString() contructor call. Also, if you ever need to empty a 
> QString use
>
> QString::clear() instead of assigning QString() to it for the same reason.
>
FIXED

> . NetworkManager::Device::availableConnections() allocate new objects 
> without parent and without warning the user of that method that they 
> should deallocate the memory used. Preferrably you should use a cache 
> mechanism like the one used by NetworkManager::networkInterfaces() to 
> prevent this memory leak.
>
SOLVED - It's not necessary to create new connections
>
> . Use qobject_cast instead of dynamic_cast with QObjects.
>
There is no dynamic_cast, only static_cast like in Plasma NM. It's not 
possible to use qobject_cast since these classes are not derived from 
QObject.
>
> . In examples/createconnection/main.cpp you can use 'result.ToLower() 
> == "yes"' instead of those three comparisons with "yes", "YES" and 
> "Yes". You could also add more comments describing in details what the 
> examples do. I also see that you use 
> Settings::WirelessSetting::setSsid(ssid.toAscii()) there. Is the 
> 'toAscii()' part required for this to work perperly? If so then that 
> should be added to the setSsid() API doc (in 
> settings/802-11-wireless.h). By the way, all class methods in settings 
> directory lack API doc comments, I know, that is not your fault since 
> you copied the classes from Plasma NM. Yet that is one thing we need 
> to fix before the first release.
>
>
I'm using "toAscii()" only when I need to convert QString to QByteArray. 
It's because from NetworkManager::AccessPoint you get ssid in QString 
and in NetworkManager::Settings::WirelessSetting::setSsid() you have to 
pass QByteArray. I know that documentation is missing, but I think it's 
not so important for now. By the way, I didn't copy these classes from 
Plasma NM.
>
> . in NetworkManager::Settings::WirelessSecuritySetting::fromMap() I 
> think you should add a 'else { nmDebug() << "Unhandled item comment"; 
> }' for each of the if's in there. That would help us detect when 
> NetworkManager adds new possible values in their API.
>
That's a good idea, I think we should add it also into other classes, 
not only into WirelessSecuritySetting. I'll implement it later, I think 
it's not a blocker for this merge.
>
> . also in NetworkManager::Settings::WirelessSecuritySetting::fromMap() 
> you insert some QStringList without checking if they are not empty. 
> Usually NetworkManager refuses settings with empty values, which is 
> the case here. You can add some Q_ASSERT() to check for that. This way 
> the check will only be add to the final binary if Qt is compiled with 
> debug enabled.
>
I think that this problem cannot happen, if you mean QStringLists that 
are created where I put proto, pairwise and group values. I'm checking 
whether these values are empty, if not then there cannot be empty 
QStringList because it's filled with these values.
>
> . Code style:
>
> - use 'foo &bar' instead of 'foo& bar' or 'foo & bar'.
>
> - do not add empty lines without any purpose.
>
> - start comments with capital case and with a period in the end. Also 
> add one space between // and the start of the comment.
>
> - you can use astyle command to do most of those changes. See
>
> https://techbase.kde.org/Policies/Kdelibs_Coding_Style
>
Fixed almost with astyle
>
> . Commit messages should be "git-formatted": one short summary in the 
> first line, followed by an empty line, and then a longer explanation 
> of why the    commit has been made and what it does. Yes, I know, I 
> also need starting doing that myself.
>
> . you should also run krazy2 to detect any other possible issueѕ I 
> have not spotted: 
> http://techbase.kde.org/Development/Tutorials/Code_Checking. Some of 
> the issued reported by krazy2 are KDE specific and does not apply to 
> libnm-qt, which is Qt only library.
>
DONE
>
> [1] 
> http://lamarque-lvs.blogspot.com.br/2013/03/what-is-happening-in-qtnetworkmanager.html
>
> -- 
>
> Lamarque V. Souza
>
> KDE's Network Management maintainer
>
> http://planetkde.org/pt-br
>

Important issues should be fixed. Could you please do another round of 
review? :)

-- 
Jan Grulich
Red Hat Czech, s.r.o
jgrulich at redhat.com

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20130324/59e98cf7/attachment-0001.html>