1650733 - Further improvements to CardDAV

Status: RESOLVED FIXED

(Thunderbird :: Address Book, task)

Description

[Geoff Lankow (:darktrojan)]

There is more work to do on CardDAV. I think it's worth closing bug 546932 and starting this new one, not least to uplifting patches less confusing.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1650733-carddav-sync-super-1.diff

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1650733-carddav-conflict-1.diff

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1650733-carddav-callbacks-1.diff

This is almost entirely cut-and-paste, not as much real change as it looks.

Using some tricks I've found in other places, I moved the callback functions out into an object, which can be QI'ed to the right interface (satisfying nsIInterfaceRequestor).

I also changed how password failures are handled, see the start of promptAuth. This achieves the same result without storing an extra value.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9161541 [details] [diff] [review]
1650733-carddav-callbacks-1.diff

Review of attachment 9161541 [details] [diff] [review]:
-----------------------------------------------------------------

Do you intend to reuse it, or why not just have the object inline?

::: mailnews/addrbook/jsaddrbook/CardDAVDirectory.jsm
@@ +647,5 @@
> +  asyncOnChannelRedirect(oldChannel, newChannel, flags, callback) {
> +    /**
> +     * Copy the given header from the old channel to the new one, ignoring missing headers
> +     *
> +     * @param {String} aHdr         The header to copy

nit: @param {string} aHdr - The header to copy.

But let's not use the a convention here. So simply hdr

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9161536 [details] [diff] [review]
1650733-carddav-sync-super-1.diff

Review of attachment 9161536 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/jsaddrbook/CardDAVDirectory.jsm
@@ +36,5 @@
>      this._sendCardToServer(newCard);
>      return newCard;
>    }
> +  addMailList() {
> +    throw Components.Exception("", Cr.NS_ERROR_NOT_IMPLEMENTED);

Please add the function name to the messages, so it's easier to know what is not implemented.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/47b40ea44701
Use JS class inheritance to simplify CardDAV card modification. r=mkmelin
https://hg.mozilla.org/comm-central/rev/63669b838a3f
Detect and resolve conflicts in CardDAV cards. r=mkmelin
https://hg.mozilla.org/comm-central/rev/21a41a633ee9
Simplify channel callbacks in CardDAVDirectory. r=mkmelin

Comment 7

[Geoff Lankow (:darktrojan)]

Comment on attachment 9161536 [details] [diff] [review]
1650733-carddav-sync-super-1.diff

One request for all patches. This is all preffed off stuff.

Comment 8

[Wayne Mery (:wsmwk)]

Comment on attachment 9161536 [details] [diff] [review]
1650733-carddav-sync-super-1.diff

[Triage Comment]
Approved for esr78

Comment 9

[Wayne Mery (:wsmwk)]

Comment on attachment 9161538 [details] [diff] [review]
1650733-carddav-conflict-1.diff

[Triage Comment]
Approved for esr78

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9161541 [details] [diff] [review]
1650733-carddav-callbacks-1.diff

[Triage Comment]
Approved for esr78

Comment 11

[Geoff Lankow (:darktrojan)]

Thunderbird 78.2.0:
https://hg.mozilla.org/releases/comm-esr78/rev/82d40fbfa252
https://hg.mozilla.org/releases/comm-esr78/rev/8017dfe81cff
https://hg.mozilla.org/releases/comm-esr78/rev/ef66ee9d2dce

Comment 12

[Lukas.Wringer]

Maybe you could consider reopening this Bug? There are other Improvements to the CardDav feature that you may want to look into - ege.:

• 'Global' Readonly adressbooks: Some adressbooks in groupware solutions are made readonly, often used as a kind of global adressbook. At the moment they would get synced but with a Adressbook with tenthousands of adresses - this may not be ideal... -> Cardbook and SOGo (not sure about TBSync) did allow for a searchable but not syncable adressbook. LDAP is not suitable as areplacement - as for one thing TB crashes / hangs when the server is not reachable but the computer is not offline at the same time - and the ldap port is very offten blocked in public networks.

• Syncronisation of Lists: At the moment the list feature is not available to carddav adressbooks (but the button is shown ;-) ). I am not sure if the VLIST-Format is SOGo specific or if it is a offial draft but you could at least allow the creation of those and let a plugin handle the synchronisation?

The SOGo/inverse team is giving up its own carddav provider (or at least they tend to as the beta is build this way) in their plugin in favour of your implementation but in doing so the loose both of this features that are very important to us. If they are missing people have more valid points in favour of outlook/exchange

Just putting it in here so that you at least know about it, if you say its to SOGo specific or whatever just delete this comment

Comment 13

[thirsch]

I would like to second (+1) the request from @Lukas.Wringer

Comment 14

[Magnus Melin [:mkmelin]]

We usually fix only one thing in a bug. I think you should open a new bug for (each) of those.

Comment 15

[Lukas.Wringer]

Done: #1668788 and #1668791