On Sun, Mar 24, 2013 at 8:04 PM, Lamarque V. Souza <span dir="ltr"><<a href="mailto:lamarque@kde.org" target="_blank">lamarque@kde.org</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<u></u>
<div 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;text-indent:0px"> Hi,</p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> 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="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">. 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;text-indent:0px">QString::clear() instead of assigning QString() to it for the same reason.</p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">. 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="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">. Use qobject_cast instead of dynamic_cast with QObjects.</p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">. 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>
</div></blockquote><div>For the Yes thing, it's better to use .compare(result, Qt::CaseInsensitive). It doesn't require changes in the string. ;)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div 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;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">. 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="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">. 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="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">. Code style:</p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> - 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;text-indent:0px"> - do not add empty lines without any purpose.</p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> - 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;text-indent:0px"> - 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;text-indent:0px"> <a href="https://techbase.kde.org/Policies/Kdelibs_Coding_Style" target="_blank">https://techbase.kde.org/Policies/Kdelibs_Coding_Style</a></p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">. 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="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">. you should also run krazy2 to detect any other possible issueѕ I have not spotted: <a href="http://techbase.kde.org/Development/Tutorials/Code_Checking" target="_blank">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="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">[1] <a href="http://lamarque-lvs.blogspot.com.br/2013/03/what-is-happening-in-qtnetworkmanager.html" target="_blank">http://lamarque-lvs.blogspot.com.br/2013/03/what-is-happening-in-qtnetworkmanager.html</a></p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"> </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">-- </p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">Lamarque V. Souza</p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px">KDE's Network Management maintainer</p>
<p style="margin-top:0px;margin-bottom:0px;margin-left:0px;margin-right:0px;text-indent:0px"><a href="http://planetkde.org/pt-br" target="_blank">http://planetkde.org/pt-br</a></p></div><br>_______________________________________________<br>
Kde-hardware-devel mailing list<br>
<a href="mailto:Kde-hardware-devel@kde.org">Kde-hardware-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/kde-hardware-devel" target="_blank">https://mail.kde.org/mailman/listinfo/kde-hardware-devel</a><br>
<br></blockquote></div><br>