<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/128724/">https://git.reviewboard.kde.org/r/128724/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 22nd, 2016, 11:19 a.m. CEST, <b>Daniel Vrátil</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As you mentioned, the RFC dictates that keys should case insensitive, which means you should fix all the code that performs case-sensitive comparision instead of converting the keys to uppercase....</p></pre>
</blockquote>
<p>On August 22nd, 2016, 11:34 a.m. CEST, <b>Bernhard Scheirle</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It is not possible to write case-insensitive code without modifying the Addressee functions. Most of the time others rely on Addressee::custom to get the right value.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Of cause instead of forcing uppercase we could do a more complex comparison in Addressee::custom via std::find_if (as you mentioned in https://git.reviewboard.kde.org/r/128723/)</p></pre>
</blockquote>
<p>On August 22nd, 2016, 2:19 p.m. CEST, <b>Daniel Vrátil</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Making lookup in KContacts::Addressee::custom() case insensitive is the right way to go then.</p></pre>
</blockquote>
<p>On August 22nd, 2016, 3:40 p.m. CEST, <b>Bernhard Scheirle</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not yet sold on the not uppercase verison.
It is good practice to unify the storage. (And it is allowed by the RFC)
Not unifying the storage makes this class more complex with little to no gain at all.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Lets examine three different methods:
1. Uppercase all keys (as proposed by me)
2. Save keys as they are, if updated stay with the original key
3. Save keys as they are, if updated use the new key</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(ci-search is a case-insensitive search)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">// Three Addressee with the same uid (so I can compare them later)
Addressee c;
Addressee a(c);
Addressee b(c);</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">// Insert the same custom multiple times:
a.insertCustom("app", "name", "value1");
a.insertCustom("app", "NAME", "value2");
// What is the internal state of 'a' now? How to get to this state?
// 1. "APP-NAME" : "value2"; simple hash of the key needed
// 2. "app-name" : "value2"; ci-search for the key, if found update the value
// 3. "app-NAME" : "value2"; ci-search for the key, if found delete it and insert the new one</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">// Output must be independent of the case:
a.custom("app", "name"); // == "value2"
a.custom("app", "NAME"); // == "value2"
// 1. simple hash of the key needed
// 2.3. ci-search for the key needed</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a.customs();
// Output:
// 1. ["APP-NAME:value2"]; Output is independent of the insert order.
// 2. ["app-name:value2"]; Output is dependent of the insert order.
// 3. ["app-NAME:value2"]; Output is dependent of the insert order.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a.setCustoms({"app-name:value1", "APP-NAME:value2"});
// Key duplication:
1. conflict gets resolved automatically via the hash collision
2. 3. Explicit duplicate key detection necessary</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">b.insertCustom("app", "NaMe", "value2");
// 1. "APP-NAME" : "value2"
// 2. 3. "app-NaMe" : "value2"</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a == b; // must be true
// 1. Use QHash::operator==
// 2. 3. Explicit ci-search of the 'a' keys in the keys of 'b'</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What are the disadvantages of always uppercase all keys? And do they justify a complex Addressee class?</p></pre>
</blockquote>
<p>On August 23rd, 2016, 10:07 p.m. CEST, <b>Daniel Vrátil</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Right, converting the keys to all-uppercase makes sense. This however does not affect the argument about Addressee::custom() performing a case-insensitive lookup...</p></pre>
</blockquote>
</blockquote>
<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;">In the current version Addressee::custom() preforms already a case-insensitive lookup.
Internal all keys are uppercase, it calls toUpper on the input and does a case-sensitive lookup.
Therefore it is essentially a case-insensitive lookup.</p></pre>
<br />
<p>- Bernhard</p>
<br />
<p>On August 20th, 2016, 8:59 p.m. CEST, Bernhard Scheirle 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 KDEPIM and Laurent Montel.</div>
<div>By Bernhard Scheirle.</div>
<p style="color: grey;"><i>Updated Aug. 20, 2016, 8:59 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=317966">317966</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kcontacts
</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;">Allways save the upper case version of a custom key. This forces developers to not use case-sensitive keys.
Case-sensitive keys are bad, because:
1. RFC 6350 explicitly says that keys are case-insensitive: https://tools.ietf.org/html/rfc6350#section-3.3
2. Ex- and importing contacts may change the case (e.g. if you use owncloud).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Of cause this change affects a few other applications:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">https://git.reviewboard.kde.org/r/128723</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">akonadi-contacts/src/editor/generalinfoeditor/messaging/messagingwidgetlister.cpp</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">akonadi-contacts/src/standardcontactformatter.cpp</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">akonadi-contacts/src/editor/customfieldeditor/customfieldslistwidget.cpp</li>
</ul>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">https://git.reviewboard.kde.org/r/128722</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">kdepim-apps-libs/kaddressbookgrantlee/src/printing/contactgrantleeprintobject.cpp</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">kdepim-apps-libs/kaddressbookgrantlee/src/formatter/grantleecontactformatter.cpp</li>
</ul>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(No review request yet)</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">kdepim/kmail/src/kmreaderwin.cpp</li>
</ul>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(No review request yet)</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">kopete/libkopete/kabcpersistence.cpp</li>
</ul>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(No review request yet)</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">akonadi-google-applets/contacts/src/contactwidgetitem.cpp</li>
</ul>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(No review request yet)</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">libkgapi/src/contacts/contactsservice.cpp</li>
</ul>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(No review request yet)</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">messagelib/messageviewer/src/header/contactdisplaymessagememento.cpp</li>
</ul>
</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Since I'm fairly new to KDE development I appreciate any feedback.</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;">ctest fails currently (4 test cases fail).
I want to wait for your feedback before updating the test cases.</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>src/addressee.cpp <span style="color: grey">(86072c0da026e85fe5cd2a2115a2c47e0f31be74)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128724/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>