GNOME Bugzilla – Bug 556061
EContact/EVCard can re-order phone and email attributes
Last modified: 2010-05-19 09:06:26 UTC
If you import a vcard with three EMAIL attributes and then export, you will find that the EMAIL attributes are exported in reverse order. Also, if you create two email entries in the contact editor gui, you will find that they are put into the vcard in reverse order. This may seem trivial, but it causes complications for the addressbook-conduit. If the pda has two emails, in 'slot1' and 'slot2' it is problematic if they get written to 'email1' and 'email2' on the desktop and they are subsequently edited by the gui. The edit can cause them to be reversed, which causes consistency problems when syncing. I'm attaching two patches, one for e-d-s to fix e-contact and e-vcard, and one for the gui editor. I have more extensive patches for the conduit itself so I'll raise that separately.
Created attachment 120476 [details] [review] fix contact/vcard attribute reordering patch adds a 'e_vcard_append_attribute' method to e-vcard. The standard 'e_vcard_add_attribute' prepends. In e-contact, the new 'append' is used in a couple of places to ensure relative ordering of attributes in maintained.
Created attachment 120478 [details] [review] avoid re-ordering phone attributes in contact editor gui fix to contact gui editor to avoid reversing the order of phone attributes.
Matt, isn't append costly? Anyways I understand the problem. Akhil, can you test it in your setup?
Hi Srinivasa, Yes, append is more expensive than prepend. Appending a list of length N to a list of length L is of order LxN whereas prepending is of order N. That said, I doubt the difference will be noticeable in these cases because: o the lists being appended to are not long. They are lists of attributes for a contact, so run to maybe tens of entries, not hundreds or thousands. o The number of times the list is appended to scales with the number of attributes in a contact (actually, less than that because only a few attribute types are order-sensitive). Again, not a scary number. o finding the end of a list of tens of entries is cheap compared to the other work being done when these prepends are being done, such as creating and copying attributes, and removing and destroying attributes. Does Akhil have a capability to measure performance tests? I could take a look at optimising if there were noticeable effects from these changes. Thanks, Matt
Hi, any progress on this? I'm blocked from contributing a patch to fix several data-loss bugs in the addressbook-conduit until this change is available in e-d-s. See http://mail.gnome.org/archives/gnome-pilot-list/2008-October/msg00017.html for details on those issues.
Matt, sorry for the delay. I tried out the patch and it works fine but i am not sure about overhead.
How can we assess the impact of the 'append'? Do you have a test suite that can run through some kind of stress test for the vcard/econtact/addressbook component? My guess is that there are no pathological issues and that the user is not likely to notice any change. I agree that it would be good to get to a more objective analysis! Let me know if I can help. One possibility, I guess is to create a large addressbook and then profile export/import to/from vcard files with and without my patch. Would this be a reasonable test, or do you see any cases likely to stress the machinery more than that?
In the first patch, I do not like the new 'break;' in chunk '@@ -880,10 +881,12 @@', it really shouldn't be there. The 'num_left' is used for MULTI_ELEM_STR_FIELD I do not see any reason to change anything in the e-contact.c, really, it works as expected and every software using EContact counts on the old behavior. Most of the changes you did there are just making it slower (noticeably or not). Except of the last chunk, the one in 'e_contact_set_attributes', that's a good idea to have it there. (or calling a 'reverse' there too.) I was fixing some ordering issues in bug #544383 and in bug #523775, which this one is duplicate of (not by description, but by the patch itself for sure), but I would like to use your change in 'e_contact_set_attributes', it makes sense and is good change (even I would prefer to see that with a reverse call or do you think the addition of the e_vcard_append_attribute function will be good for others?) Please add also a ChangeLog entry to your evolution-data-server patch. Thanks.
Created attachment 122376 [details] [review] updated patch, to include fix for caching bug in e_contact_get_const(). Note - needs test code in the attachment following this one.
Created attachment 122377 [details] test case to demonstrate bugs in the ATTR2_TYPE_STR_FIELD and MULTI_ELEM_STR_FIELD e-contact field types This test case should be placed in the evolution-data-server/addressbook/tests/ebook/ directory. Note previous attachment includes updated Makefile.am to build this test case.
Hi Milan, Sorry for the fairly long reply. You raise three issues, which I'll try and deal with in turn: 1. the use of the 'break' statement 2. e-contact "works as expected" 3. my intentions for the new 'append_attribute' API First, I think you may have misread my use of the 'break'. It exits the for-loop over _parameters_ within an attribute because a matching parameter has been found, and starts working on the next attribute. Without that 'break', the code is fragile as to the ordering of parameters. Say we're looking for PHONE_HOME_2. Compare: Attribute 1: TEL;X-EVOLUTION-UI-SLOT=3;TYPE=HOME,VOICE Attribute 2: TEL;TYPE=HOME,VOICE;X-EVOLUTION-UI-SLOT=3; In the first attribute, the current code will decrement "num_left" once; in the second case it will decrement it twice because "found_needed1" and "found_needed2" are both still true when the 'X-EVOLUTION-UI_SLOT' param is processed. The new 'break;' means that after finding 'TYPE=HOME,VOICE' we go on to the next attribute. I found this bug because, for the pilot addressbook-conduit, I found it necessary to preserve some information from the pilot using a custom X-EVOLUTION- parameter (analogous to the UI-SLOT, actually). This then broke the "ATTR2" retrieval logic. Second, I do not agree that e-contact.c "works as expected". In fact, your changes in bug #544383 were required precisely because e_contact_set_property didn't work as expected! Try the following fun example: 1. set "E_CONTACT_PHONE_HOME" = "123 4567" 2. set "E_CONTACT_PHONE_HOME_2" = "987 6543" Then you will find that if you retrieve "E_CONTACT_PHONE_HOME_2" you get "123 4567". This is just plain bad. It's even worse than that: it is possible for the e_contact_get() and e_contact_get_const() to disagree. Try: 1. set "E_CONTACT_PHONE_HOME" = "123 4567" 2. call e_contact_get_const(contact, E_CONTACT_PHONE_HOME); 3. set "E_CONTACT_PHONE_HOME_2" = "987 6543" 4. now e_contact_get_const(contact, E_CONTACT_PHONE_HOME) and e_contact_get(contact, E_CONTACT_PHONE_HOME) will disagree. My initial patch fixed the first of these two bugs. I've just attached an updated patch that fixes the caching bug, too. I've also attached a test case "test-ordered-phones" for the addressbook/tests/ebook/ directory that demonstrates these bugs. Thirdly, and finally, here's a bit more context for my API changes. I'm the maintainer of gnome-pilot and my motivation for this stuff has been to try and fix several pretty bad data-loss bugs in the addressbook-conduit that makes the conduit unreliable. Once this is done I plan to extend it to include support for the new 'contact' palmos database with birthdays, photos, anniversaries, etc. I was not able to fix the conduit using the current e-contact API, due to the above bugs. The palm conduit contains 5 'slots' that can be filled with work, home, fax, email or other such string-based entries. Ordering is important, and I need to track changes across synchronisations, so I really really want to avoid any re-ordering of e-contact attributes - otherwise you end up duplicating or losing fields when syncing. The contact-editor code gets and sets all EMAIL and PHONE entries as a sequence, so you were able to work around the buggy API by adding ALL attributes of a type in the reverse order. This is not practical in the addressbook-conduit, and doesn't seem to be the right approach anyway. I guess the 'e_contact_set_attributes' could employ a 'reverse' to reverse the list of attributes before prepending them to the vcard. To be honest, though, I'm having a hard time believing that these 'append' operations are going to be remotely user-noticeable. They are not used during bulk vcard import/export operations, as I discovered when trying to profile the operations. I've asked whether there's any test suite available for profiling, but I'm guessing there's none. Are there any examples of bulk operations where these e-contact manipulations could add up? I hope the above explanations, and the attached test code, helps you understand the scale of the problems with the current API. If you think there are other places where 'ordering' workarounds are in place to cope with the current API, I'll be happy to help trying to adapt them to work with the e-contact fixes above - just point me in the right direction. I'm just not willing to try to fix the addressbook-conduit with a basically unusable e-contact API! :) Matt
(In reply to comment #11) > You raise three issues, which I'll try and deal with in turn: > 1. the use of the 'break' statement > ... I see your point here, and you've right. I think there is also other issue, when the TYPE parameter contains something what evo doesn't know, like TYPE=HOME,VOICE,PREF (in this order), then evo will not find this, because the last PREF will make it set 'match=false' even both found_neededX are set to true, when looking for E_CONTACT_PHONE_HOME_2. Am I right? > 2. e-contact "works as expected" > ... Here I meant every application using this structure and depending on the order expects some behaviour, and has some hacks to satisfy its needs. If we will change this behaviour, then all of them should adapt. I know it's a good way of adaptation, but anyway, it's quite much work, isn't it? (I do not know how many applications are using EContact/EVCard, though.) For your conduit, isn't it possible to use some "normal" form for comparisons? Again, I see your point about making it here, I'm just not sure because of the other applications. > 3. my intentions for the new 'append_attribute' API > ... OK, I'm fine with the API extension. ------------------------------------------------------------------------------ After all, I'm sorry to say, but I've such a feeling I'm not the best person to review you effort. I'm afraid I'm not in the position to do such decision(s). Ross, what do you think?
Ross: ping
I know my comment #11 was rather long. I'd just like to highlight one key point for anyone new to this bug (!): ---------------- Try the following fun example: 1. set "E_CONTACT_PHONE_HOME" = "123 4567" 2. set "E_CONTACT_PHONE_HOME_2" = "987 6543" Then you will find that if you retrieve "E_CONTACT_PHONE_HOME_2" you get "123 4567". This is just plain bad. ---------------- This brokenness is the direct cause of data loss bugs in the addressbook conduit that I can't work around. I believe the submitted patch is a clear improvement to the API and am willing to help adapting other code as required to deal with the changes introduced (for example, review changes made in working around the issues in bugs 544383 and 523775).
Ah, a prime example of why EContact should be avoided for any serious use. Using EVCard and treating the list of attributed as an unordered list will fix this and other bugs (i.e. if you are using E_CONTACT_PHONE_HOME_2, what happens if someone has 5 phone numbers?).
Hi Ross, That's an interesting take on the problem! I hadn't realised EContact was deprecated :) Treating all HOME phone numbers as an unordered list won't help me with my problem of writing a robust palm conduit. On palm you have 5 ordered 'slots' for labeled phone/fax/email numbers. That's it. So if you have 4 email addresses and 4 phone numbers on the desktop you cannot store them all on the palm. The important thing is to be consistent with which ones you store and to avoid data loss. That consistency is hard/impossible to achieve with the current bugs, and not really easier if HOME numbers were returned in a random order. I can greatly improve the palm conduit behaviour with the addition of my proposed patch. It's not an impossible problem: Outlook has much the same problems as Evolution in this regard. Sure we could add more E_CONTACT_PHONE_HOME_X slots, but two has served us pretty well over the past number of years. If more were requested, they could be added. I'm still blocked here :(
This is really frustrating. I'm totally blocked on improving gnome-pilot syncing, for no good reason that I can see. I was spurred to ping this bug report when I came across this comment, in a discussion about removing gnome-pilot from Ubuntu default install: "Evolution doesn't sync with Palm anyway, at least not without killing fields and duplicating what it doesn't kill." http://ubuntuforums.org/showthread.php?t=1080350&page=2 That's what this bug means for gnome-pilot: data corruption. Again: I refer new readers to comment #14. I should have mentioned that my patch fixes the broken behaviour described there! I'm finding it really hard to motivate myself to port gnome-pilot conduits away from EContact when I have a patch that fixes EContact to my satisfaction and should be a general improvement! If you consider EContact to be broken, why care about a patch that seems to improve it? I'm willing to improve the patch if necessary to avoid any reordering side effects, but not unless there's some buy-in that this change is likely to be accepted.
(In reply to comment #17) > That's what this bug means for gnome-pilot: data corruption. -> critical > I'm finding it really hard to motivate myself to port gnome-pilot conduits away > from EContact when I have a patch that fixes EContact to my satisfaction and > should be a general improvement! If you consider EContact to be broken, why > care about a patch that seems to improve it? So you ask for having a statement about EContact from Srag/Ross? At least the patch here should get a review soon.
Thanks, Andre.
Firstly, I don't think Ross means to say EContact is deprecated. It would mean that if you want to do extensive attribute operations, use EVCard directly. Ross, can you have a look at the patch? I know you are busy, just find some time in between, thanks :-).
Thanks, that sounds encouraging! I've no problem using EVCard for attribute manipulation as required, but the ordering (and maintaining ordering) is important. The EContact code doesn't respect that ordering at present. Even if the conduit code used EVCard directly, other code using EContact would mess things up between syncs.
Can I PLEASE get a review of this patch? This patch is seven months old last week, and addresses a critical (data loss) bug for PalmOS syncing. It may not be fashionable, but it is still in use by users and part of Evolution.
ping!
I'm inclined to approve and commit these patches if an address book maintainer does not speak up by Monday. Matt has addressed all the concerns raised here and has been waiting far too long already.
The patch is quite sprawling and mixes up several issues: * Adding e_vcard_append_attribute() * Cache fixup in e_contact_set() * Ordering fixes in e_contact_set_property * A test case * Adding new API, e_contact_get_field_and_attribute Adding e_vcard_append_attribute() is fine, but please add documentation to _add_attribute so say that it prepends for speed and if order matters there is _append_. The cache fixups in e_contact_set() should be moved into _set_property because it is possible to set values using g_object_set(). e_contact_get_field_and_attribute() is only used by e_contact_get (which then throws away the attribute). What is this new method call for? Please split the patch up into a series, preferably produced by git so that you'll get your name in the changelog.
Thanks, I'll work on updating the patch and splitting it up as per the above. I'll update it to trunk, too. I have a separate patch for the Evolution gnome-pilot conduits that use e_contact_get_field_and_attribute() to remember a palm-specific ordering across syncs. If the field is modified by Evolution between syncs the attribute will be lost (because the contact editor overwrites all attributes) but that doesn't matter for my use case because the attribute is only required if the desktop record is unmodified between syncs. If there is a better way to remember state per field between syncs, I'd be open to it, but this method worked well for me and I can't see it having any side effects. Thanks Ross, and Matthew.
Matt, any progress in updating the patch?
Matt, I am very interested of all your patches to evolution and gpilot, because I have all the problems you mentioned. I still use a Palm T|X.
I have patched evolution 2.22.3.1 and evolution-data-server 2.22.3.0 with all your patches mentioned here (http://mail.gnome.org/archives/gnome-pilot-list/2008-October/msg00017.html) and from this bug (are the same thing) and I had no problems until today. The synchronisation is smoother as even before. I would be happy if your changes find the way to the official code repository and on this way to any distribution that contains evolution.
*** Bug 599530 has been marked as a duplicate of this bug. ***
Created attachment 161417 [details] [review] eds patch for evolution-data-server; Added new API and few changes within EContact functions, as suggested by Matt, but I didn't take all the things, and I did a bit more things there too. See the changes. I'm committing this patch and closing the bug. If you have more concerns, feel free to reopen and summarize, as this bug got awfully long already.
Created commit a670033 in eds master (2.31.2+)