From kde-hardware-devel Sun Mar 24 23:00:44 2013 From: "Lamarque V. Souza" Date: Sun, 24 Mar 2013 23:00:44 +0000 To: kde-hardware-devel Subject: Re: [Kde-hardware-devel] QtNetworkManager (aka libnm-qt) pending issues Message-Id: <201303242000.44709.lamarque () kde ! org> X-MARC-Message: https://marc.info/?l=kde-hardware-devel&m=136416606130493 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============1448833407114715941==" --===============1448833407114715941== Content-Type: multipart/alternative; boundary="Boundary-01=_cW4TRUy5eeGXxgh" Content-Transfer-Encoding: 7bit --Boundary-01=_cW4TRUy5eeGXxgh Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit 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 --Boundary-01=_cW4TRUy5eeGXxgh Content-Type: text/html; charset="iso-8859-15" Content-Transfer-Encoding: 7bit

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

--Boundary-01=_cW4TRUy5eeGXxgh-- --===============1448833407114715941== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Kde-hardware-devel mailing list Kde-hardware-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-hardware-devel --===============1448833407114715941==--