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
_______________________________________________
Kde-hardware-devel mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kde-hardware-devel
