<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi,<br>
<br>
thanks for the first round of review. I've made some changes, see
below:<br>
<br>
On 03/24/2013 08:04 PM, Lamarque V. Souza wrote:<br>
</div>
<blockquote cite="mid:201303241604.30079.lamarque@kde.org"
type="cite">
<meta name="qrichtext" content="1">
<style type="text/css">
p, li { white-space: pre-wrap; }
</style>
<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>
</blockquote>
FIXED<br>
<br>
<blockquote cite="mid:201303241604.30079.lamarque@kde.org"
type="cite">
<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>
</blockquote>
SOLVED - It's not necessary to create new connections<br>
<blockquote cite="mid:201303241604.30079.lamarque@kde.org"
type="cite">
<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>
</blockquote>
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.<br>
<blockquote cite="mid:201303241604.30079.lamarque@kde.org"
type="cite">
<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; "> <br>
</p>
</blockquote>
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.
<blockquote cite="mid:201303241604.30079.lamarque@kde.org"
type="cite">
<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>
</blockquote>
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.<br>
<blockquote cite="mid:201303241604.30079.lamarque@kde.org"
type="cite">
<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>
</blockquote>
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.<br>
<blockquote cite="mid:201303241604.30079.lamarque@kde.org"
type="cite">
<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;">
<a class="moz-txt-link-freetext" href="https://techbase.kde.org/Policies/Kdelibs_Coding_Style">https://techbase.kde.org/Policies/Kdelibs_Coding_Style</a></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>
</blockquote>
Fixed almost with astyle<br>
<blockquote cite="mid:201303241604.30079.lamarque@kde.org"
type="cite">
<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:
<a class="moz-txt-link-freetext" href="http://techbase.kde.org/Development/Tutorials/Code_Checking">http://techbase.kde.org/Development/Tutorials/Code_Checking</a>.
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>
</blockquote>
DONE<br>
<blockquote cite="mid:201303241604.30079.lamarque@kde.org"
type="cite">
<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]
<a class="moz-txt-link-freetext" href="http://lamarque-lvs.blogspot.com.br/2013/03/what-is-happening-in-qtnetworkmanager.html">http://lamarque-lvs.blogspot.com.br/2013/03/what-is-happening-in-qtnetworkmanager.html</a></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;"><a class="moz-txt-link-freetext" href="http://planetkde.org/pt-br">http://planetkde.org/pt-br</a></p>
</blockquote>
<br>
Important issues should be fixed. Could you please do another round
of review? :)<br>
<pre class="moz-signature" cols="72">--
Jan Grulich
Red Hat Czech, s.r.o
<a class="moz-txt-link-abbreviated" href="mailto:jgrulich@redhat.com">jgrulich@redhat.com</a></pre>
</body>
</html>