[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-hardware-devel
Subject: [Kde-hardware-devel] QtNetworkManager (aka libnm-qt) pending issues
From: "Lamarque V. Souza" <lamarque () kde ! org>
Date: 2013-03-24 19:04:29
Message-ID: 201303241604.30079.lamarque () kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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
[Attachment #5 (text/html)]
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" \
"http://www.w3.org/TR/REC-html40/strict.dtd"> <html><head><meta name="qrichtext" \
content="1" /><style type="text/css"> p, li { white-space: pre-wrap; }
</style></head><body style=" font-family:'Tahoma'; font-size:12pt; font-weight:400; \
font-style:normal;"> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> Hi,</p> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> 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:</p> <p style="-qt-paragraph-type:empty; margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; "> </p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">. 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</p> \
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">QString::clear() instead of \
assigning QString() to it for the same reason.</p> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">. \
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.</p> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">. Use qobject_cast instead of \
dynamic_cast with QObjects.</p> <p style="-qt-paragraph-type:empty; margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; "> </p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">. 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.</p> <p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> \
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">. 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.</p> <p style="-qt-paragraph-type:empty; margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; "> </p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">. 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. </p> <p style="-qt-paragraph-type:empty; \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; "> </p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">. Code style:</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;"> - use 'foo &bar' instead of 'foo& bar' \
or 'foo & bar'.</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"> - do not add empty lines without any purpose.</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> - start comments with \
capital case and with a period in the end. Also add one space between // and the \
start of the comment.</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"> - you can use astyle command to do most of those changes. See</p> \
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> \
https://techbase.kde.org/Policies/Kdelibs_Coding_Style</p> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">. 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.</p> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">. 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.</p> <p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> \
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">[1] \
http://lamarque-lvs.blogspot.com.br/2013/03/what-is-happening-in-qtnetworkmanager.html</p>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> \
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">-- </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">Lamarque V. Souza</p> <p \
style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">KDE's Network Management \
maintainer</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">http://planetkde.org/pt-br</p></body></html>
_______________________________________________
Kde-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic