Review Request: Mobile Connection Wizard: A concept of i18n of APN's list

Sun Apr 1 15:37:51 UTC 2012

> On March 19, 2012, 8:01 p.m., Lamarque Vieira Souza wrote:
> > settings/config/mobileconnectionwizard.cpp, line 162
> > <http://git.reviewboard.kde.org/r/104349/diff/1/?file=53849#file53849line162>
> >
> >     Use "foreach (const QVariantMap &apn, mApns)", it's more efficient.
> 
> Swami Dhyan Nataraj [Nikolay Shaplov] wrote:
>     Fixed. But I can't understand the idea. What this will give to us? 
>     const will forbid changing of the apn but how it will help us?

"const" is a hint to the compiler for it to activate certain optimizations that otherwise would be supressed.

Also keep in mind that always using more memory to save CPU time is not a good idea, specially in embedded systems. In Plasma Active for instance using less memory in Plasma NM is more important then saving CPU time since most CPUs nowadays are more than capable of current desktop tasks if the program is properly written. That are the reasons I preferred to re-parse only the node for the specified provider in the .xml instead of caching its data. It has been working good for the one year and a half that the code is in Plasma NM. Since this wizard is rarely used and is a separated process that will release all its memory once finished there is not much of a problem in using more memory here (I guess).

> On March 19, 2012, 8:01 p.m., Lamarque Vieira Souza wrote:
> > settings/config/mobileproviders.h, line 51
> > <http://git.reviewboard.kde.org/r/104349/diff/1/?file=53850#file53850line51>
> >
> >     Forcing everyone to pass a QDomElement to this method is not a good ideia API speaking. This forces everybody to first parse the file before they can get the QDomElement.
> 
> Swami Dhyan Nataraj [Nikolay Shaplov] wrote:
>     I considered it as an internal function that allows to convert xml representation into QVariantMap one. It should not be used outside of the object. 
>     getApns already gives a list of QVariantMaps.
>     
>     May be it would be good to move getApnInfo into private area and change name to apnXml2VariantMan or something like this.

I still prefer that you re-implement a public "QVariantMap getApnInfo(const QString & apn);" and make "QVariantMap getApnInfo(const QDomElement & apn);" private since there is no sense in showing QDomElements in a public API here.

getApns gives a list O QVAriantMaps but its usage is not good, API speaking. Forcing everbody to get a list and then iterate through it to find the desired data is not a good API, at least not in this case. Unfortunately for you, since you are using a QList to store the apn info you will have to iterate through the list to implement the public "QVariantMap getApnInfo(const QString & apn);".

What's the point in having a only private getApnInfo? getApnInfo can be usefull for other classes besides MobileProviders, although there is not such case once your patch removed the only case for it. I prefer to have one public getApnInfo and a private one like a suggested above.

- Lamarque Vieira

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104349/#review11630
-----------------------------------------------------------

On April 1, 2012, 6:06 a.m., Swami Dhyan Nataraj [Nikolay Shaplov] wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104349/
> -----------------------------------------------------------
> 
> (Updated April 1, 2012, 6:06 a.m.)
> 
> 
> Review request for Network Management and Lamarque Vieira Souza.
> 
> 
> Description
> -------
> 
> This is a kind of concept how to internationalize list of APNs in mobile connection wizard.
> 
> This concept uses less CPU and more memory (through this task is not resourse critical). It create a list of apn's QVatiant map once, and then uses it all the time till the end. I thought it is better then reparse XML each time we need an APL info.
> 
> If there are ant comments, I will try to do fixes. If no, and everything is OK, I will remove all commented out code from the patch, and let's commit it :-)
> 
> 
> Diffs
> -----
> 
>   settings/config/mobileconnectionwizard.h 77fdf56cdf1f9093bcb7052a6a19e6b1bb5ea184 
>   settings/config/mobileconnectionwizard.cpp 1b31c59e0133d931723f0c98902828d98a36aea4 
>   settings/config/mobileproviders.h 72189133fdb6e64edd3ca7736bdc4a2c4954d585 
>   settings/config/mobileproviders.cpp 1ef26fcddeded9e612571a3fd4d1e0771e763bce 
> 
> Diff: http://git.reviewboard.kde.org/r/104349/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Swami Dhyan Nataraj [Nikolay Shaplov]
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120401/b32da555/attachment.html>