[Kde-pim] Review Request 110591: Add vcard based ContactGroup2 format

Kevin Krammer krammer at kde.org
Sun Sep 15 19:54:22 BST 2013 


> On Sept. 12, 2013, 12:49 p.m., Kevin Krammer wrote:
> > I thought about this a bit more over lunch. I think the MIME type duality (text/vcard is an alias of text/directory) is something that could be a problem, since we currently don't have a polymorphic contact type that could be either a contact or a group.
> > 
> > When we look at this regarding its goals we can, I think, identify two separate ones:
> > 1) use contact GIDs instead of Akonadi item IDs when referencing contacts
> > 2) be able to serialize groups from/to vcard >= 4.0
> > 
> > (1) could also be solved by making the existing ContactGroup class use the different reference string. we fully control the serialization of that class so we can easily add either a flag to distinguoish between the two types of reference or use a separate entry type. We also have a unique MIME type for it.
> > 
> > (2) should be solvable by adding ContactGroup to the vcard parser/tool just like this patch does for ContactGroup2. Naturally it will fail on entries that use Akonadi IDs, but that could be handled by the application using the parser/tool.
> > 
> > We make sure that all newly created groups use the GID method, maybe also when editing.
> 
> Christian Mollekopf wrote:
>     > When we look at this regarding its goals we can, I think, identify two separate ones:
>     
>     Right
>     
>     > 1) use contact GIDs instead of Akonadi item IDs when referencing
>     
>     That's the motivation for ContactGroup2.
>     
>     > 2) be able to serialize groups from/to vcard >= 4.0
>     
>     I just did that because I anyways required a serialization format for ContactGroup2, and since we have group support in vcard 4.0...
>     
>     > (1) could also be solved by making the existing ContactGroup class use the different reference string. we fully control the serialization of that class so we can easily add either a flag to distinguoish between the two types of reference or use a separate entry type. We also have a unique MIME type for it.
>     
>     The problem is not on the serialization level, it's on the application level. Applications that use a ContactGroup expect to get akonadi item id's as members. ContactGroup2 (or a ContactGroup that supports UID's), explicitly doesn't support that, so all uses of ContactGroup would have to be adjusted. So using a different type gives us a typesafe way to distinguish between the two and a safe migration path. Everything else just get's even messier I think. Here are some options I considered:
>     * determining the UID inline if not available (slow, new error cases, no migration)
>     * allowing for both uid and akonadi item id on ContactGroup (all applications have to handle both cases, no clear transition from old (ContactGroup) to new (ContactGroup2) making it more difficult to get rid of the legacy code eventually (or writing new code that doesn't handle the legacy case).
>     
>     Therefore I still think ContactGroup2 is the way to go, but maybe I missed something.
>     
>     And given ContactGroup2, it just wouldn't add any value to serialize it to the custom format if a standard is available (albeit perfectly possible).
>     
>

I think you are misinterpreting what I have written.
We agree that there are two goals, but I am considering a different way of getting to them.

> Applications that use a ContactGroup expect to get akonadi item id's as members.

No, current users of ContactGroup *can* handle group members that are references by Akonadi item ID (ContactGroup::ContactReference). They already handle another type of member: ContactGroup::Data.
There are two options for also supporting contact references by GID: (1) using ContactGroup::ContactReference but with a type of reference and, (2) a new sub structure for GID references.

> so all uses of ContactGroup would have to be adjusted

That is true for any approach. 

My gut feeling is that adding another top level type is way more work [1] (needs handling of an additional payload type, etc).
Most applications will use the group expand job to fully load a group, adding a third sub type would be transparent to them.
They would not have to change anything about how they handle the job and associated editor widgets since there is no new top level type to handle.

[1] Just compare how often ContactGroup is being used on lxr.kde.org versus how often ContactReference is used
http://lxr.kde.org/ident?i=ContactGroup (539 hits, not all KABC::ContactGroup) vs. http://lxr.kde.org/ident?i=ContactReference (73 hits, most of them inside the ContactGroup code)


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110591/#review39877
-----------------------------------------------------------


On Sept. 11, 2013, 1:38 p.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110591/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2013, 1:38 p.m.)
> 
> 
> Review request for KDEPIM-Libraries, Kevin Krammer and Tobias Koenig.
> 
> 
> Description
> -------
> 
> This patch adds a vcard based replacement for ContactGroup, which uses UID's for references instead of Akonadi::Item::Id's. ContactGroup2 is meant to replace ContactGroup, but we have to wait until KDE5 until we can get rid of ContactGroup and rename ContactGroup2 (for binary compatiblity).
> 
> 
> Diffs
> -----
> 
>   akonadi/contact/abstractcontactgroupformatter.h adaac0fd2063a155509591510bab4584bec0b858 
>   akonadi/contact/abstractcontactgroupformatter.cpp 94b27905e4cc3c07d10502c5c10b68eefb3357fb 
>   akonadi/contact/contactgroupeditor.h bdef7871f760d8e13e9d1bd19a45f9161090f858 
>   akonadi/contact/contactgroupeditor.cpp 1a83b972cc9e0c30208b4cf5a00e32c784524a03 
>   akonadi/contact/contactgroupeditor_p.h 981e1d494ea4aaac295458e417847caded4d287d 
>   akonadi/contact/contactgroupeditordialog.cpp 2cbf0dee4b9efa52d7fb21b3842b4658c62d4968 
>   akonadi/contact/contactgroupexpandjob.h 48914153f71eb9ee651be0b717ab6d26332f0498 
>   akonadi/contact/contactgroupexpandjob.cpp 304b7a42ed0a04caed7cb798695a4afbabcb0641 
>   akonadi/contact/contactgroupmodel.cpp 03875819d5c61724e960c43c5a025a67a776373a 
>   akonadi/contact/contactgroupmodel_p.h 12332ced2ae9bd7d47bd2466fc27eb14c7e51da6 
>   akonadi/contact/contactgroupviewer.cpp 9c04e905d02fecc9bc9aaf0165ff6cb94e3d4370 
>   akonadi/contact/contactstreemodel.cpp b98c867a62aa68a5d67918ad8814b419a164fea9 
>   akonadi/contact/standardcontactactionmanager.cpp 9248b89f211b33a9c5ee3a5bc038140d565bfbc7 
>   includes/KABC/ContactGroup2 PRE-CREATION 
>   kabc/CMakeLists.txt b3907c69758eb4ab7973af60f1d7e4da625fc96b 
>   kabc/addressee.cpp c6c6515d0c207b5af4b264268aab1c30a3a24ad0 
>   kabc/contactgroup2.h PRE-CREATION 
>   kabc/contactgroup2.cpp PRE-CREATION 
>   kabc/tests/CMakeLists.txt 7bc46cb38380f3374bd6c5f023c0f7a5dbfe31dd 
>   kabc/tests/contactgroup2test.cpp PRE-CREATION 
>   kabc/vcardconverter.h e379f7c6248edaf0e313154362ea0ca66b5765dc 
>   kabc/vcardconverter.cpp 9d7556c5efbef18a26db661e0fac82e15124c2fe 
>   kabc/vcardparser/vcard.h 3f714c424ef71a8732f192d65388df2c46843371 
>   kabc/vcardtool.h 6f2b3707755d7889bef5074b2d5f089a9a8349f5 
>   kabc/vcardtool.cpp e99bd9f52fc1f987cf788a7670a93704a37ef123 
> 
> Diff: http://git.reviewboard.kde.org/r/110591/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christian Mollekopf
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

More information about the kde-pim mailing list