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

[Lamarque V. Souza]

Em Sunday 24 March 2013, Jan Grulich escreveu:
> Hi,
> 
> thanks for the first round of review. I've made some changes, see below:
> 
> > . 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

	Yes, but you call NetworkManager::Settings::findConnection() for each 
available connection now. We can optimize that.
 
> > . 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, line 54 you use dynamic_cast in a 
WirelessDevice, which derives 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.

	"not so important for now", never say that about API documentation :-P 
If you had not copied then from Plasma NM then I can say you must (not only 
should) fix that :-)
 
> > . 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.

	Yeah, that's not a blocker.
 
> > . 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.

	You are assuming the QVariantMap passed to 
WirelessSecuritySetting::fromMap() is always in good state. I am asking you to 
add the Q_ASSERT() to detect the case when the QVariantMap is not in good 
state. Using Q_ASSERT() ensures that it will not cause runtime overhead if qt 
is compiled in release mode.

> Important issues should be fixed. Could you please do another round of
> review? :)

	Ok, I will do it when I have some more spare time. By the way, since you 
moved the connection-editor from networkmanagement repo to plasma-nm repo that 
means you need to make sure Plasma NM stills works with the settings branch of 
libnm-qt, if anything breaks then that is a showstopper for the settings 
branch. At first I thought you would be updating Plasma NM to work with the 
settings branch, is that still your plans?

-- 
Lamarque V. Souza
KDE's Network Management maintainer
http://planetkde.org/pt-br
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20130324/5a96683d/attachment.html>