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