Unknown VCard properties are lost



  • rfc2822: of course you would rather see this as a feature request, or nice to have, as opposed to consider this a case of data-loss, because that would be bad.

    I’m not really interested to go into the small print of rfc’s, but I can dig and perhaps I can find one or two lines that underline the main idea I’m trying to convey.

    That’s not the point however. If you are a CardDAV client, and the first thing you do after connecting to a server is to DELETE every vcard that’s already on the system… technically you are still working within the constraints of the protocol and are standards-compliant.

    @chrysn in this regard has it right. This all depends on a users’ expectation. Based on how you choose to proceed in this matter, I would indeed at the very least always warn the user that some data may be lost after every save operation.

    As for PATCH. It’s hard! And it will not solve your problem. Take this (part of a) hypothetical vcard:

    EMAIL;TYPE=WORK;PREF=1:foo@bar.com
    EMAIL;TYPE=HOME;PREF=2:foo@bar.com
    

    In a PATCH, we’d like to update one of these. How can we easily distinguish the two? The answer that really comes down to the following:

    The only way you can semantically patch one of those lines, is if you’re updating exact byte indexes, the literal order of properties in a vcard (assuming they don’t change), or you include the exact line you’re patching, such as:

    -EMAIL;TYPE=WORK;PREF=1:foo@bar.com
    +EMAIL;TYPE=WORK;PREF=1:foo@example.com
    

    Semantic patching is, afaik, very hard… the way the format works is that you can’t really say something like “update property X and set this value” or “find property X with parameter Y and update this value” accurately.

    So as far as I can tell, the only patch mechanisms actually possible, rely on the caller (you) to have a knowledge of the exact structure of the original, which defeats the point of what you’re trying to achieve (because you discard the stuff you don’t support).

    Note that your workaround of putting properties aside you don’t support goes a long way, but only solves part of the problem, as it doesn’t cover parameters, grouping and the other items on this list.

    So yea, I’d be happy to have a chat about the details of the rfc’s and what their intended meaning is, but in the context of this ticket this is meaningless, because step 1 is to accept that it’s not about being compliant to the letter of the specs, it’s admitting you have a problem.


  • developer

    @jkufner:

    I would not remove attachments by default, just wait whether they cause problems first.

    They have caused problems already. Handling the images in the VCards requires special attention and still doesn’t work with large images flawlessly.

    @chrysn:

    i’m not really familiar with android development, so i don’t know what you mean by column.

    The contact information is not stored by DAVdroid itself, but by the Android contacts provider (“the address book”) which itself uses an SQLite database to store the information. The tables and columns must be used exactly by the Android specification so the contacts information is exchangeable between all Android apps. There are some fields/record types that allow sync adapters to store “custom” data – I’d probably use such one for the original VCard.

    i’d be happy with attachments or overly large images being dropped if there is some kind of confirmation if it actually happens (“Uploading these changes will drop an attachment to this contact (pictures-of-my-cats.ppt, 20MB). [Save anyway] [Abort editing]”).

    Normally, a sync adapter like DAVdroid (basically, just a service) should not need a (blocking) user interface (if that’s even possible, haven’t checked yet). For instance, if one app (let’s say, Samsung Kies or some other remote control app) adds a contact to the contacts provider, DAVdroid should sync it to the server without user interaction.

    @bartv2

    I’m not really interested to go into the small print of rfc’s, but I can dig and perhaps I can find one or two lines that underline the main idea I’m trying to convey.

    I have found this:

    »However, if a client needs to make a change to a vCard, it can only change the entire vCard data via a PUT request. There is no way to incrementally make a change to a set of properties within a vCard object resource. [Yes, we already know this. But now the interesting sentence:] As a result, the client will have to cache the entire set of properties on a resource that is being changed.« — RFC 6352 (CardDAV) 9.1. Restrict the Properties Returned

    This can be most reasonably interpreted as that it’s expected behaviour to retain all properties (even if there’s no MUST).

    As for PATCH. It’s hard!

    As it’s not within the scope of CardDAV, i won’t use it until there’s a) at least a draft specification for usage with CardDAV/VCards, and b) support from at least a few servers.

    So yea, I’d be happy to have a chat about the details of the rfc’s and what their intended meaning is, but in the context of this ticket this is meaningless, because step 1 is to accept that it’s not about being compliant to the letter of the specs, it’s admitting you have a problem.

    I admit that you (not you @bartv2 but all who are affected by this issue) have a problem. Personally, I don’t have one because I have enjoyed the removal of hundreds of useless X-Mozilla-*, X-Evolution-* etc. headers at every sync 🙂 Maybe that’s because I’m a minimalist — I honestly didn’t have the idea that someone will be missing these headers. (And yes, I know that DAVdroid itself uses X-DAVdroid-Starred and this will be lost when updating the VCard by other, equally-behaving clients. I didn’t think that somebody would be missing these headers, too.)

    @All:

    I want other users to have a good experience of DAVdroid. But it’s not the best idea to tell me your feedback like “Look there! A f*cking critical bug, and it deletes all data! How dare you!1! Fix it NOW, you’re my open-source slave and have to work when I say so! And by the way, you’ll have to do X and Y and Z, too, otherwise DAVdroid is crap!” when the behaviour just works fine for me, because then I’ll of course solve the conflict of your interests and my interests by asking an independent judge – namely the specs (even if I could simply ignore your feedback as well as the specs – there’s no obligation that I’ll code for you, and by the way, you could also do it yourself if its that important for you).

    So, after considering RFC 6352 9.1 (see above), I think DAVdroid really SHOULD retain all properties (meaning a higher priority than “nice to have” 😀).



  • Thank you for fixing this.



  • Thank you, now i don’t need to worry that changing contacts on my phone will lose information.


  • developer

    You’re welcome – but don’t forget to clean your VCards from time to time… they can only grow and never shrink.



  • lol



  • thanks a lot for fixing this, it makes davdroid my preferred android cal-/carddav client now.

    the attributes in known fields still do get lost (eg EMAIL;TYPE=WORK;X-EVOLUTION-UI-SLOT=1:attribute-test@example.com -> EMAIL;TYPE=work:attribute-test@example.com), and the contact photos get re-encoded (in my test run, the file grew from 1.2k to 1.5k). can we track this here or shall i open another issue for that?

    by the way, your warnings against vcards got me to think; i might add cleanup features to calypso when i find the time to factor out the import features.


  • developer

    the attributes in known fields still do get lost (eg EMAIL;TYPE=WORK;X-EVOLUTION-UI-SLOT=1:attribute-test@example.com -> EMAIL;TYPE=work:attribute-test@example.com)

    Yeah, they do. Attributes could theoretically be stored in the SYNC1 field of the email address row, but at the moment, there are other priorities. Do you really use these properties on VCards which you edit with DAVdroid or did you just test it?

    and the contact photos get re-encoded (in my test run, the file grew from 1.2k to 1.5k). can we track this here or shall i open another issue for that?

    The VCard must be parsed and then generated (each by ez-vcard), so I think re-encoding is unavoidable (given the current functionality of ez-vcard).



  • Ad attributes: I guess separate issue with low priority would be nice, just to keep the bug known. It will get lost here.

    Ad image reencoding: I did a quick look into ez-vcard and I do not see any image transformations in photo property. Only moving blob around. That should not cause loosing image quality. Maybe I missed something (it was only a brief exploration). Isn’t there some hidden reencoding when storing image into Android database?


  • developer

    Ad attributes: I guess separate issue with low priority would be nice, just to keep the bug known. It will get lost here.

    Ah, you mean a low-priority enhancement. Well why not 😛

    Isn’t there some hidden reencoding when storing image into Android database?

    Ah, that may be the case too. Actually, I didn’t look. In both cases, it can’t be changed by DAVdroid.


Log in to reply
 

Maybe you're interested in these topics?

  • 101
  • 1
  • 2
  • 5