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