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

Discussion:

Lamarque Vieira Souza

2012-03-19 20:00:54 UTC

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

settings/config/mobileconnectionwizard.h
<http://git.reviewboard.kde.org/r/104349/#comment9207>

remove trailing whitespace.

This patch is against nm09 branch. Can you provide a patch against master branch too?

I do not have time to test the patch now. I will do it next weekend.

settings/config/mobileconnectionwizard.cpp
<http://git.reviewboard.kde.org/r/104349/#comment9222>

You removed insertSeparator in the "else" clause, why not here? This can leads to a crash in the wizard.

settings/config/mobileconnectionwizard.cpp
<http://git.reviewboard.kde.org/r/104349/#comment9209>

Use "foreach (const QVariantMap &apn, mApns)", it's more efficient.

settings/config/mobileconnectionwizard.cpp
<http://git.reviewboard.kde.org/r/104349/#comment9216>

Use ')' (single quotes) instead of ")" (double quotes), it's more efficient when handling a single character.

settings/config/mobileconnectionwizard.cpp
<http://git.reviewboard.kde.org/r/104349/#comment9217>

stick to the code style and use four spaces for indentation.

settings/config/mobileconnectionwizard.cpp
<http://git.reviewboard.kde.org/r/104349/#comment9218>

stick to the code style and use "} else {" here.

settings/config/mobileproviders.h
<http://git.reviewboard.kde.org/r/104349/#comment9219>

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.

settings/config/mobileproviders.h
<http://git.reviewboard.kde.org/r/104349/#comment9211>

remote trailing whitespace.

settings/config/mobileproviders.h
<http://git.reviewboard.kde.org/r/104349/#comment9215>

remove trailing whitespace.

settings/config/mobileproviders.cpp
<http://git.reviewboard.kde.org/r/104349/#comment9214>

remove trailing whitespace.

settings/config/mobileproviders.cpp
<http://git.reviewboard.kde.org/r/104349/#comment9212>

remote trailing whitespace.

settings/config/mobileproviders.cpp
<http://git.reviewboard.kde.org/r/104349/#comment9213>

remote trailing writespace.

settings/config/mobileproviders.cpp
<http://git.reviewboard.kde.org/r/104349/#comment9221>

Use single quotes for single characters, it's more efficient.

settings/config/mobileproviders.cpp
<http://git.reviewboard.kde.org/r/104349/#comment9220>

What is the problem here? Stick to the code style, add a space after the comma.

- Lamarque Vieira Souza

-----------------------------------------------------------
http://git.reviewboard.kde.org/r/104349/
-----------------------------------------------------------
(Updated March 19, 2012, 6:55 p.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 3cd801e92f1bf15acf7857192fac052107fdfe52
settings/config/mobileconnectionwizard.cpp b2e72d631b8272c4be5bdb8766a1d214715e2038
settings/config/mobileproviders.h 8df2269b38f7339d328de53e0be4734896bc1595
settings/config/mobileproviders.cpp 24d2c9256ebc87997e8a9f1903faa84900aa4d16
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/20120319/d458d9a2/attachment-0001.html>

Swami Dhyan Nataraj [Nikolay Shaplov]

2012-03-19 18:55:07 UTC

Permalink

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

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 3cd801e92f1bf15acf7857192fac052107fdfe52
settings/config/mobileconnectionwizard.cpp b2e72d631b8272c4be5bdb8766a1d214715e2038
settings/config/mobileproviders.h 8df2269b38f7339d328de53e0be4734896bc1595
settings/config/mobileproviders.cpp 24d2c9256ebc87997e8a9f1903faa84900aa4d16

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/20120319/2cf5583b/attachment.html>

Swami Dhyan Nataraj [Nikolay Shaplov]

2012-04-01 06:06:17 UTC

Permalink

-----------------------------------------------------------
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 (updated)
-----

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/efa642de/attachment.html>

Lamarque Vieira Souza

2012-04-01 14:39:43 UTC

Permalink

Post by Lamarque Vieira Souza
-----------------------------------------------------------
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/ee6909ac/attachment-0001.html>

Swami Dhyan Nataraj [Nikolay Shaplov]

2012-03-31 16:35:00 UTC

Permalink

Post by Lamarque Vieira Souza

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.

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.

Post by Lamarque Vieira Souza

settings/config/mobileconnectionwizard.cpp, line 150
<http://git.reviewboard.kde.org/r/104349/diff/1/?file=53849#file53849line150>
You removed insertSeparator in the "else" clause, why not here? This can leads to a crash in the wizard.

Oups, my mistake

Post by Lamarque Vieira Souza

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.

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?

Post by Lamarque Vieira Souza

settings/config/mobileproviders.cpp, line 276
<http://git.reviewboard.kde.org/r/104349/diff/1/?file=53851#file53851line276>
Use single quotes for single characters, it's more efficient.

It was not me' I've just copied code from function above :-)

- Swami Dhyan Nataraj

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

Post by Lamarque Vieira Souza
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/104349/
-----------------------------------------------------------
(Updated March 19, 2012, 6:55 p.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 3cd801e92f1bf15acf7857192fac052107fdfe52
settings/config/mobileconnectionwizard.cpp b2e72d631b8272c4be5bdb8766a1d214715e2038
settings/config/mobileproviders.h 8df2269b38f7339d328de53e0be4734896bc1595
settings/config/mobileproviders.cpp 24d2c9256ebc87997e8a9f1903faa84900aa4d16
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/20120331/d34f502f/attachment.html>

Lamarque Vieira Souza

2012-04-01 15:37:51 UTC

Permalink

Post by Swami Dhyan Nataraj [Nikolay Shaplov]

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.

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).

Post by Swami Dhyan Nataraj [Nikolay Shaplov]

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
-----------------------------------------------------------

Post by Swami Dhyan Nataraj [Nikolay Shaplov]
-----------------------------------------------------------
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>