[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-hardware-devel
Subject: Re: [Kde-hardware-devel] QtNetworkManager (aka libnm-qt) pending issues
From: Jan Grulich <jgrulich () redhat ! com>
Date: 2013-03-24 22:39:44
Message-ID: 514F80B0.5090104 () redhat ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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
jgrulich@redhat.com
[Attachment #5 (text/html)]
<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-qtnetworkmanage \
r.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>
_______________________________________________
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