Review Request 128724: [WIP] [KContacts] Allways save the upper case version of a custom key of a KContacts::Addressee
Bernhard Scheirle
bernhard+kde at scheirle.de
Tue Aug 23 21:39:08 BST 2016
> 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.
>
> Bernhard Scheirle wrote:
> 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?
>
> Daniel Vrátil wrote:
> Right, converting the keys to all-uppercase makes sense. This however does not affect the argument about Addressee::custom() performing a case-insensitive lookup...
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.
- 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/20160823/f1287cfe/attachment.html>
More information about the kde-pim
mailing list