[Kde-hardware-devel] QtNetworkManager (aka libnm-qt) pending issues

[Aleix Pol]

On Sun, Mar 24, 2013 at 8:04 PM, Lamarque V. Souza <lamarque at kde.org> 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.
>
>
>
> . 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.
>
For the Yes thing, it's better to use
.compare(result, Qt::CaseInsensitive). It doesn't require changes in the
string. ;)


>
>
> . 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
>
> _______________________________________________
> Kde-hardware-devel mailing list
> Kde-hardware-devel at kde.org
> https://mail.kde.org/mailman/listinfo/kde-hardware-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20130325/2588becb/attachment-0001.html>