[Kde-pim] Review Request 110591: Add vcard based ContactGroup2 format
Kevin Krammer
krammer at kde.org
Thu Sep 19 13:32:41 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).
>
>
>
> Kevin Krammer wrote:
> 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)
>
> Christian Mollekopf wrote:
> I suppose our gut feelings differ then ;-)
>
> Adjusting ContactGroup to ContactGroup2 results in approximately 300 actual hits that may need adjustment, most of which I already adjusted in my patches. The only real advantage that extending ContactGroup indeed has is when ContactGroupJob is used to expand the group (so the akonadi item id <-> uid change is transparent to the client code -> ~30 instances).
>
> I actually started with doing pretty much what you're suggesting (adding the possibility to set an actual UID in ContactReference and handling that case in the group expand job), but eventually dismissed the approach as it was also a lot of work but resulted in IMO unclear semantics. I.e. you can have groups that freely mix references by akonadi item id and uid. Also, there is no clear migration path, respectively each piece of software is forced to handle both cases gracefully. With ContactGroup2 any application can simply decide to only support that, dismissing any legacy groups.
>
> E.g. in the kolabproxy I don't want to have anything to do with ContactGroup as I know it's utterly broken in that scenario:
> * contacts changing akonadi item id when being edited => contact group reference broken
> * contact references support a preferred email address and custom values which is not mappable to vcard
> So from a ContactGroup consumer perspective I definitely prefer to only have to deal with ContactGroup2, and possibly add a small compatibility layer (that can be provided by payload conversion) to support ContactGroup.
>
> IMO we will have a much easier time in the future if we can build new software on top of ContactGroup2 instead of handling all possible usage scenarios of an extended ContactGroup.
>
> It may be a bit more effort in the beginning, but I think it will pay off (and it isn't that much more really).
>
> Thanks for taking the time discussing this =)
>
> Kevin Krammer wrote:
> > Adjusting ContactGroup to ContactGroup2 results in approximately 300 actual hits that may need adjustment
> [...]
> > so the akonadi item id <-> uid change is transparent to the client code -> ~30 instances
>
> I am not sure I can follow you here. Are you saying 300 changes all over the place are less effort and less invasive than 30 changes inside library internals?
>
> > I.e. you can have groups that freely mix references by akonadi item id and uid.
>
> Very unlikely, one would have to specifically create those.
>
> > With ContactGroup2 any application can simply decide to only support that, dismissing any legacy groups.
>
> I don't see how that would be possible.
> Let say you switch KMail from ContactGroup to ContactGroup2. All contact groups sofar created by the user would no longer be accessible.
> Even in the case that an applications gains support for both, it would be very difficult UI wise. Currently it can offer to create a contact group and then let the user select a collection to safe it to. With two different incompatible types it first needs to ask for the target collection and then, based on collection content MIME types, decide which group to create.
>
> Compare that to always using ContactGroup: there is no change for applications receiving a contact group, the expand job takes care of that. There is no change for applications that create contact groups through the standard UI, the standard editor takes care of that.
>
> That leaves 5 classes outside of kdepimlibs that access ContactReference themselves: two in the contact kresource bridges (already deprecated), one in the Kolab resource (likely a candidate for expand job usage), and two in KMail, one in composer and one in mobile (which again looks like an expand job candidate).
>
> As far as I can tell that leaves your two issues:
> the first is (a) a bug in whatever edits the item (item id should never change on payload change) and (b) not an issue with expand job, right?
> the second seems to also be handled by expand job on first look, but that can probably be handled differently as well, e.g. using ContactGroup::Data instead or duplicating the contact and adjusting email/customs
>
> Christian Mollekopf wrote:
> > I am not sure I can follow you here. Are you saying 300 changes all over the place are less effort and less invasive than 30 changes inside library internals?
>
> I'm saying by drawing a clear line between new and old we'll have an easier time getting rid of the old.
> Most of the porting effort can be done by s/ContactGroup/ContactGroup2 (thanks to ContactGroupExpandJob ;-)
>
> > Very unlikely, one would have to specifically create those.
>
> That's possible. Depends on how member editing is implemented, but I guess you're right for i.e. KAddressbook.
>
> > I don't see how that would be possible.
>
> * It's directly possible for e.g. the kolab proxy
> * All applications can be migrated straight to ContactGroup2 (the payload conversion takes care of legacy groups). There's a performance penalty when using legacy groups, but the new codepath using ContactGroup2 remains clean of handling legacy data.
> * I'll port the standard resources that can hold contact groups.
> * All new groups are stored in ContactGroup2 format. This is indeed a potential problem for third party resources (that I don't migrate), the same is true however if there is suddenly a new UID which they don't persist (This will break just as badly).
>
> What I prefer about this approach is that ContactGroup along with the payload conversion code can simply be dropped for KDE5, without touching anything else of the codebase. That means I'm doing the porting effort now upfront, instead of having to visit the 300 places later on to clean up akonadi item id based references (Assuming that we want to get rid of those eventually).
>
> I fear that by extending ContactGroup with yet another possibility to create links we don't migrate, but simply add another possibility without dropping the old one. This leads to more complexity overall in the long run and having to convert back and forth between the two versions forever. With ContactGroup2 we have a clear transition, can add compatibility for the old (instead of the new), and the porting effort looks IMO bigger than it really is.
>
> I think I understand your intentions (it was also my first approach to the problem), but I'm not convinced that ContactGroup2 is nonsense.
> Does what I write make any sense to you?
>
> Kevin Krammer wrote:
> Lets assume a base scenario for the goal of using GID references (the vCard import/export is independent of that anyway):
>
> two addressbooks, each having 5 contact groups, say a personal contacts resource and a kolab proxy. KAddressBook used for viewing/creating, KMail used for completion.
>
> Option 1: ContactGroup2
> - resources: replace ContactGroup with ContactGroup2 in retrieveItems() and retrieveItem() code paths and saving code paths. Loading code paths need both types, so do itemXXX() code paths
> - applications: monitors/models need to handle both MIME types, any direct access to payload types needs at least a code path that converts read-only
> - KAddressBook: only creates ContactGroup2, maybe also convert on edit/change. target folder selection needs to use new MIME type
> - KMail: same as KAddressBook for creation, probably no change for completion
>
> Option 2: ContactGroup with GID references
> - resources: no change in personal contacts, it uses native format for serialization. Kolab Proxy already expands, could do that with the expand job instead
> - applications: no change in general, unless direct access to ContactReference
>
> Either option will need explicit data migration before the respective deprecated type can be dropped.
>
> IMHO the second option scales a lot better, since there is almost no change for applications.
> Resources either using native serialization or expand job don't have to change Akonadi -> Backend paths, switch Backend -> Akonadi paths plus read-only conversion of Akonadi ID -> GID.
> Resources expanding manually need to add fetch by GID for Akonadi -> Backend paths
>
> Christian Mollekopf wrote:
> > Option 1: ContactGroup2
> > - resources: replace ContactGroup with ContactGroup2 in retrieveItems() and
> > retrieveItem() code paths
>
> This would be handled by the mimetype. If the mimetype is vcard and it's a group => ContactGroup2 with the existing mimetype => ContactGroup.
> In the ContactGroup case applications could still get a ContactGroup2 payload through payload conversion.
>
> > and saving code paths.
>
> Saving codepaths would just save according to the payload type. If all applications have been ported that's only ContactGroup2 indeed.
>
> > Loading code paths need
> > both types, so do itemXXX() code paths
>
> > - applications: monitors/models need
> > to handle both MIME types, any direct access to payload types needs at
> > least a code path that converts read-only
>
> This is all transparently handled by payload conversion. Applications are simply ported to item.payload<ContactGroup2>(), if the payload is actually ContactGroup the payload conversion provides inline conversion between the two types.
> That is of course suboptimal, but as all data gets migrated to ContactGroup2 that codepath simply stops being used. The ContactGroup2 codepath would perform ideal from the start.
>
> > - KAddressBook: only creates
> > ContactGroup2, maybe also convert on edit/change. target folder selection
> > needs to use new MIME type
>
> Yes, any edit/creation would result in ContactGroup2 being stored instead of ContactGroup resulting in a gradual migration of all contacts.
> The migration process could of course be helped by a migrator. We just can't do that directly because we have no way for blocking migrations
>
> > - KMail: same as KAddressBook for creation,
> > probably no change for completion
> >
>
> Yes.
>
> > Option 2: ContactGroup with GID references
> > - resources: no change in personal contacts, it uses native format for
> > serialization. Kolab Proxy already expands, could do that with the expand
> > job instead
>
> The native format would have to be extended with a GID, but otherwise correct.
> The kolabproxy would resolve akonadi item id's upon write and transform them to GID references.
>
> > - applications: no change in general, unless direct access to
> > ContactReference
> >
>
> Yes.
>
> > Either option will need explicit data migration before the respective
> > deprecated type can be dropped.
> >
> > IMHO the second option scales a lot better, since there is almost no change
> > for applications. Resources either using native serialization or expand job
> > don't have to change Akonadi -> Backend paths, switch Backend -> Akonadi
> > paths plus read-only conversion of Akonadi ID -> GID. Resources expanding
> > manually need to add fetch by GID for Akonadi -> Backend paths
> >
>
> I think the difference is that I consider references by akonadi item id's a bug in contact groups and wanted to fix that. It's IMO a layer violation that is just exposed by the problems in the kolab proxy:
>
> * A contact is not guaranteed to be tied to a specific item (id), and I don't think that should be a requirement. Indeed an implementation detail happens to change the item id in the kolab proxy on every edit. You could call that a bug, but I think references by GID are just the overall better concept so I figured why not go with that right away. I should be able to work around that problem now with the support of GID lookups.
>
> * That the akonadi item id's don't make any sense outside of an akonadi instance is obvious, so every resource needs to have a translation layer. The GID solution would fix that problem at it's root instead of relying on each resource to do that (I realize that mapping akonadi item id's to something that suits the backend is a core responsibility of a resource, but IMO the GID solution is much more elegant).
>
> So by introducing a new type I wanted to drop the concept of id based references from contact groups (together with some other cleanups to better map to vcard). It is a bit more work, but would allow to deprecate more code (which is a good thing IMO).
> I'm fully aware that ContactGroup2 requires more immediate changes than just adding a GID member to ContactGroup, but I figured it would be worth it.
>
> I'll just add a GID member to ContactGroup and see how that goes and bring up dropping support for akonadi item id based links and the native format for KDE5 again.
> Otherwise I think we're running in circles =)
> If all applications have been ported that's only ContactGroup2 indeed.
That is independent of applications, the need is given as long as there are items of the old type
> Applications are simply ported to item.payload<ContactGroup2>()
How would that work? Converting a ContactGroup into a ContactGroup2 is an asynchronous operation.
> We just can't do that directly because we have no way for blocking migrations
Why would it need to be blocking?
> I think the difference is that I consider references by akonadi item id's a bug in contact groups and wanted to fix that.
I wouldn't consider it a bug. It is the ideal reference in an Akonadi system. Is is unaffected by any payload change or item operation.
The storing of the IDs on disk, e.g. through the contacts resource, could be considered a bug though.
> Indeed an implementation detail happens to change the item id in the kolab proxy on every edit.
That sounds like a serious bug to me. Synching such a resource would trigger tons of notifcations for no gain.
Models would discard perfectly valid data, refetch and re-insert it all over.
>From an application's point of view that is the worst possible situation.
> The GID solution would fix that problem at it's root instead of relying on each resource to do that
Sure, I agree on that, at least for externilization. It also requires the server to be able to create GIDs on item copy operations or be able to deal with non-unique GIDs.
I haven't followed the GID server changes closely enough, did you use the former or the latter?
- 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