<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/104349/">http://git.reviewboard.kde.org/r/104349/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 19th, 2012, 8:01 p.m., <b>Lamarque Vieira Souza</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/104349/diff/1/?file=53849#file53849line162" style="color: black; font-weight: bold; text-decoration: underline;">settings/config/mobileconnectionwizard.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void MobileConnectionWizard::initializePage(int id)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">162</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">mPlanComboBox</span><span class="o">-></span><span class="n">insertItems</span><span class="p">(</span><span class="mi">0</span><span class="p">,</span> <span class="n">mApns</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">162</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">foreach</span> <span class="p">(</span><span class="n">QVariantMap</span> <span class="n">apn</span><span class="p">,</span> <span class="n">mApns</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Use "foreach (const QVariantMap &apn, mApns)", it's more efficient.</pre>
</blockquote>
<p>On March 31st, 2012, 4:34 p.m., <b>Swami Dhyan Nataraj [Nikolay Shaplov]</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">"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).</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 19th, 2012, 8:01 p.m., <b>Lamarque Vieira Souza</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/104349/diff/1/?file=53850#file53850line51" style="color: black; font-weight: bold; text-decoration: underline;">settings/config/mobileproviders.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">51</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QVariantMap</span> <span class="n">getApnInfo</span><span class="p">(</span><span class="k">const</span> <span class="n">QDomElement</span> <span class="o">&</span> <span class="n">apn</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On March 31st, 2012, 4:34 p.m., <b>Swami Dhyan Nataraj [Nikolay Shaplov]</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Lamarque Vieira</p>
<br />
<p>On April 1st, 2012, 6:06 a.m., Swami Dhyan Nataraj [Nikolay Shaplov] wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Network Management and Lamarque Vieira Souza.</div>
<div>By Swami Dhyan Nataraj [Nikolay Shaplov].</div>
<p style="color: grey;"><i>Updated April 1, 2012, 6:06 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 :-)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>settings/config/mobileconnectionwizard.h <span style="color: grey">(77fdf56cdf1f9093bcb7052a6a19e6b1bb5ea184)</span></li>
<li>settings/config/mobileconnectionwizard.cpp <span style="color: grey">(1b31c59e0133d931723f0c98902828d98a36aea4)</span></li>
<li>settings/config/mobileproviders.h <span style="color: grey">(72189133fdb6e64edd3ca7736bdc4a2c4954d585)</span></li>
<li>settings/config/mobileproviders.cpp <span style="color: grey">(1ef26fcddeded9e612571a3fd4d1e0771e763bce)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104349/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>