<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/108064/">http://git.reviewboard.kde.org/r/108064/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 2nd, 2013, 10:11 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/108064/diff/1/?file=103610#file103610line14" style="color: black; font-weight: bold; text-decoration: underline;">libs/internals/settings/ipv6.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; "></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">14</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">mPrivacy</span><span class="p">(</span><span class="mi">0</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;">I think you should create a enum like there Ipv6Setting::EnumMethod instead of using hardcoded integer values. That works like a documentation about what are the possible values for mPrivacy.</pre>
</blockquote>
<p>On January 2nd, 2013, 11:04 p.m., <b>Ralf Jung</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;">Okay, will do.
Would you prefer the enum to be three-state (like in the patch proposed for the Gnome applet [1]), or four-state including "Unknown", like in libnm-util? The behaviour of "Unknown" used to differ from "Disabled", but this was changed [2] and now I can not find any indication for a difference in behaviour in the sources. As I said above, the docs do not mention "Unknown" at all... but it is the default value.
[1] http://bugzilla-attachments.gnome.org/attachment.cgi?id=217377
[2] http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=af0eb9e7adf0732921ebe927408a41a36f56f72e</pre>
</blockquote>
<p>On January 2nd, 2013, 11:20 p.m., <b>Lamarque Vieira Souza</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;">Four-state, we can use "Unkwon" internally.</pre>
</blockquote>
<p>On January 3rd, 2013, 1:06 p.m., <b>Jan Grulich</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;">Take a look at my implementation of IPv6Privacy [1] [2]
[1] http://quickgit.kde.org/?p=libnm-qt.git&a=blob&h=0f7719078556e8654a009803c61cc1904e886990&hb=971ec1ff8dd66a144c79c2ac61e5d483e90b1630&f=settings%2Fipv6.h
[2] http://quickgit.kde.org/?p=libnm-qt.git&a=blob&h=b2568805fc907b605faa34ecb7d4822f393acfe1&hb=971ec1ff8dd66a144c79c2ac61e5d483e90b1630&f=settings%2Fipv6.cpp</pre>
</blockquote>
<p>On January 3rd, 2013, 1:32 p.m., <b>Lamarque Vieira Souza</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 prefer to use Unknown instead of Undefined like in NetworkManager's source code to make it easier to search both source codes. In line 466 of the second link you use the -1 value directly instead of the enumerate. There are some set* methods where the parameters can be const, like in setMethod, setIgnoreAutoRoutes, setIgnoreAutoDns, setNeverDefault, setMayFail and setPrivacy. There are some mismatches in code style (for instance in some places you use "if () {" and in some other places "if () <line break> {". The former is the correct code style. In some places you use "while(){", there should be more white spaces in there, like "while () {". You should follow code style similar to [1]. You can use the astyle command line described in [1] to do most of the changes for you, just keep in mind that astyle breaks the canonical form of the SIGNAL() and SLOT() macros, you should use normalize [2] after using astyle to fix that. The rest of the patches are ok, good work.
[1] http://techbase.kde.org/Policies/Kdelibs_Coding_Style
[2] git://gitorious.org/qt/qtrepotools/util/normalize</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;">The problem with comparison to -1 was fixed after I send these links here. Some problems with coding style was due to fact that I copied this part from networkmanagement and forgot to fix that. Anyway, I just wanted to help to Ralf Jung and my work is not part of this review :). Thanks for the short review</pre>
<br />
<p>- Jan</p>
<br />
<p>On January 1st, 2013, 4:28 p.m., Ralf Jung 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.</div>
<div>By Ralf Jung.</div>
<p style="color: grey;"><i>Updated Jan. 1, 2013, 4:28 p.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 patch adds an option to the IPv6 configuration screen to change the IPv6 Privacy Extension configuration.
The option is implemented as documented at http://projects.gnome.org/NetworkManager/developers/api/09/ref-settings.html . However, that document says the default value is "-1", while it also says that only 0, 1 and 2 are valid for this option. Therefore, I chose 0 to be the default in the Knm::Ipv6Setting class, but the option is always put into the NM config map - even if it is zero.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">I verified that whatever I configure in the applet ends up in the /etc/NetworkManager/system-connections files.
I could not yet test whether the extensions are actually properly enabled and disabled because the network I am currently in does not use IPv6. However, next week I will be at university again where I should be able to test this.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=312305">312305</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>backends/NetworkManager/settings/ipv6dbus.cpp <span style="color: grey">(8c846ab)</span></li>
<li>libs/internals/settings/ipv6.h <span style="color: grey">(2eadf69)</span></li>
<li>libs/internals/settings/ipv6.cpp <span style="color: grey">(590274b)</span></li>
<li>libs/ui/ipv6.ui <span style="color: grey">(56a1248)</span></li>
<li>libs/ui/ipv6widget.cpp <span style="color: grey">(36b6885)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108064/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>