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

[Lamarque V. Souza]

	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.

. 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.

. Use qobject_cast instead of dynamic_cast with QObjects.

. 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.

. 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.

. 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.  

. 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

. 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.

[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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20130324/6f032905/attachment.html>