<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/121047/">https://git.reviewboard.kde.org/r/121047/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2014, 10:58 p.m. CET, <b>David Faure</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="https://git.reviewboard.kde.org/r/121047/diff/1-2/?file=327084#file327084line786" style="color: black; font-weight: bold; text-decoration: underline;">src/kcodecs.h</a>
<span style="font-weight: normal;">
(Diff revisions 1 - 2)
</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; ">protected:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">752</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nl">protected:</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">751</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nl">protected:</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">should be private, maybe? or is it used by subclasses?</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The d pointers are actually accessed by implementations, so it has to be protected.</p></pre>
<br />
<p>- Daniel</p>
<br />
<p>On November 18th, 2014, 2:31 p.m. CET, Daniel Vrátil wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks, KDEPIM-Libraries and David Faure.</div>
<div>By Daniel Vrátil.</div>
<p style="color: grey;"><i>Updated Nov. 18, 2014, 2:31 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kcodecs
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KCodecs currently provides only decodeRFC2047String(), and even that has a comment saying "more robust implementation available in KMime". This patch makes KCodecs::decodeRFC2047String() use the "more robust" implementation from KMime, and adds KCodecs::encodeRFC2047String() counterpart - also using implementation from KMime.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Since KMime also provides codecs implementing various MIME encodings (quoted-printable, base64, uuencode), and the KMmime implementation of decodeRFC2047String() depends on them, I also brought over all the MIME codecs, and make use of them in all the KCodecs static methods. I believe that having a proper and more robust implementation available in a more generic framework like KCodecs is better, than if it was hidden in the KMime framework. As a result, we can make KMime framework to use KCodecs (it already depends on it anyway) for all the encodings, and only leave classes for storing and parsing the "high-level" message/rfc822 messages, and headers. It also reduces code duplication, and makes all applications use the same encode/decode implementations (which is cool, right? :-))</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The main reason for this move is that I'm trying to move over to KCodecs one small class from KPimUtils framework in kdepimlibs (which is our universal dumping ground in kdepimlibs that I'm trying to get rid of) for generic email address validation, but it depends on proper implementations of encode/decodeRFC2047String methods, so I ended up moving all this stuff :-) The tiny Email class is not part of this review though, I'll wait for this to get in, then post another review. I believe that this is a rather useful tool that could find wide adoption outside PIM, therefor I decided not to move it from KPimUtils to KMime, but instead move all the interesting, and multi-purpose parts from KMime into KCodecs.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I also moved over related KMime unit-tests (that's why the review is so big).</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Builds, all unit-tests pass.</p></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>autotests/data/codec_x-kmime-rfc2231/basic-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_x-kmime-rfc2231/null-decode.x-kmime-rfc2231 <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_x-kmime-rfc2231/null-decode.x-kmime-rfc2231.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_x-kmime-rfc2231/null-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_x-kmime-rfc2231/null-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_x-uuencode/basic-decode.x-uuencode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_x-uuencode/basic-decode.x-uuencode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/kcodecstest.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/kcodecstest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/rfc2047test.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/rfc2047test.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(adc0f2a)</span></li>
<li>src/kcodecs.h <span style="color: grey">(48effbb)</span></li>
<li>src/kcodecs.cpp <span style="color: grey">(4fd660d)</span></li>
<li>src/kcodecs_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kcodecsbase64.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kcodecsbase64.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kcodecsidentity.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kcodecsidentity.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kcodecsqp.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kcodecsqp.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kcodecsuuencode.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kcodecsuuencode.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/CMakeLists.txt <span style="color: grey">(4c41ba2)</span></li>
<li>autotests/base64benchmark.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/codectest.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/codectest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/basic-decode.b <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/basic-decode.b.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/basic-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/basic-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/null-decode.b <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/null-decode.b.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/null-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/null-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/padding0-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/padding0-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/padding1-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/padding1-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/padding2-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_b/padding2-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_base64/basic-decode.base64 <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_base64/basic-decode.base64.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_base64/basic-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_base64/basic-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_base64/corrupt.base64 <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_base64/corrupt.base64.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_base64/very_small-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_base64/very_small-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_q/all-encoded-decode.q <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_q/all-encoded-decode.q.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_q/basic-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_q/basic-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_q/null-decode.q <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_q/null-decode.q.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_q/null-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_q/null-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/basic-decode.quoted-printable <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/basic-decode.quoted-printable.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/basic-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/basic-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/corrupt.quoted-printable <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/corrupt.quoted-printable.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/corrupt2.quoted-printable <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/corrupt2.quoted-printable.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/corrupt3.quoted-printable <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/corrupt3.quoted-printable.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/corrupt4.quoted-printable <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/corrupt4.quoted-printable.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/wrap-encode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_quoted-printable/wrap-encode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_x-kmime-rfc2231/all-encoded.x-kmime-rfc2231-decode <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_x-kmime-rfc2231/all-encoded.x-kmime-rfc2231-decode.expected <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/codec_x-kmime-rfc2231/basic-encode <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121047/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>