Review Request 128724: [WIP] [KContacts] Allways save the upper case version of a custom key of a KContacts::Addressee

[Bernhard Scheirle]

> On Aug. 22, 2016, 11:19 a.m., Daniel Vrátil wrote:
> > 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....
> 
> Bernhard Scheirle wrote:
>     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.
>     
>     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/)
> 
> Daniel Vrátil wrote:
>     Making lookup in KContacts::Addressee::custom() case insensitive is the right way to go then.

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.


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

(ci-search is a case-insensitive search)

// Three Addressee with the same uid (so I can compare them later)
Addressee c;
Addressee a(c);
Addressee b(c);

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

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

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.

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


b.insertCustom("app", "NaMe", "value2");
// 1.    "APP-NAME" : "value2"
// 2. 3. "app-NaMe" : "value2"

a == b; // must be true
// 1. Use QHash::operator==
// 2. 3. Explicit ci-search of the 'a' keys in the keys of 'b'


What are the disadvantages of always uppercase all keys? And do they justify a complex Addressee class?


- Bernhard


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128724/#review98544
-----------------------------------------------------------


On Aug. 20, 2016, 8:59 p.m., Bernhard Scheirle wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128724/
> -----------------------------------------------------------
> 
> (Updated Aug. 20, 2016, 8:59 p.m.)
> 
> 
> Review request for KDEPIM and Laurent Montel.
> 
> 
> Bugs: 317966
>     https://bugs.kde.org/show_bug.cgi?id=317966
> 
> 
> Repository: kcontacts
> 
> 
> Description
> -------
> 
> 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).
> 
> Of cause this change affects a few other applications:
> 
> * https://git.reviewboard.kde.org/r/128723
>     * akonadi-contacts/src/editor/generalinfoeditor/messaging/messagingwidgetlister.cpp
>     * akonadi-contacts/src/standardcontactformatter.cpp
>     * akonadi-contacts/src/editor/customfieldeditor/customfieldslistwidget.cpp
> 
> 
> * https://git.reviewboard.kde.org/r/128722
>     * kdepim-apps-libs/kaddressbookgrantlee/src/printing/contactgrantleeprintobject.cpp
>     * kdepim-apps-libs/kaddressbookgrantlee/src/formatter/grantleecontactformatter.cpp
> 
> 
> * (No review request yet)
>     * kdepim/kmail/src/kmreaderwin.cpp
> 
> 
> * (No review request yet)
>     * kopete/libkopete/kabcpersistence.cpp
> 
> 
> * (No review request yet)
>     * akonadi-google-applets/contacts/src/contactwidgetitem.cpp
> 
> 
> * (No review request yet)
>     * libkgapi/src/contacts/contactsservice.cpp
> 
> 
> * (No review request yet)
>     * messagelib/messageviewer/src/header/contactdisplaymessagememento.cpp
> 
> 
> Since I'm fairly new to KDE development I appreciate any feedback.
> 
> 
> Diffs
> -----
> 
>   src/addressee.cpp 86072c0da026e85fe5cd2a2115a2c47e0f31be74 
> 
> Diff: https://git.reviewboard.kde.org/r/128724/diff/
> 
> 
> Testing
> -------
> 
> ctest fails currently (4 test cases fail).
> I want to wait for your feedback before updating the test cases.
> 
> 
> Thanks,
> 
> Bernhard Scheirle
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20160822/26731da6/attachment.html>