1624207 - Can't export address books with 75.0b1 or 76.0a1

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[WaltS48 [:walts48]]

Attachment: Export Failed.png

Open Address book.
Select an address book to export
Select Tools > Export and get the above error dialog

A dialog window should open so the user can select a location for exporting.

Comment 1

[Geoff Lankow (:darktrojan)]

Hmm… I'd probably better implement it then. Not sure how it got overlooked.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1624207-export-addrbook-1.diff

I've ported the actual export implementation to javascript in AddrBookUtils.jsm and created a test that it produces the same results as 68 does. I've moved the UI part (file picker) into the address book front-end code.

I haven't yet figured out the system character encoding problem (A.K.A. why doesn't Outlook support UTF-8 yet?!). Because we're using javascript we don't have access to NS_CopyUnicodeToNative which deals with the encoding for us in 68. At the moment I've hardwired windows-1252 to the system encoding options. Some possible solutions:

• Just keep using windows-1252. This should work for most people but I'd have to stop using it outside Windows.
• Improve the export UI so that the user can choose an encoding option. Some other improvements are needed anyway IMO and it could be done as a part of that.
• Pass the output across XPCOM to some function that calls NS_CopyUnicodeToNative.
• Read the current character set from the Windows registry and use that. This is feasible but a bit untidy.

Comment 3

[Magnus Melin [:mkmelin]]

I don't think people know what a charset is, what charset to use and so on, so adding UI for it might not be that useful.

Hardcoding it to windows-1252 probably just does the wrong thing for a lot of people. If it's in UTF-8, people can always open it in a text editor and convert to their preferred charset. Frankly, we might just be able to get away with using UTF-8.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9135877 [details] [diff] [review]
1624207-export-addrbook-1.diff

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

::: mail/components/addrbook/content/addressbook.js
@@ +517,5 @@
> +  // LDIF
> +  filterString = bundle.GetStringFromName("LDIFFiles");
> +  filePicker.appendFilter(filterString, "*.ldi; *.ldif"); // 5
> +
> +  filePicker.open(returnValue => {

would just use "rv" like is the standard practice for this

::: mailnews/addrbook/jsaddrbook/AddrBookUtils.jsm
@@ +19,5 @@
>  
> +XPCOMUtils.defineLazyModuleGetters(this, {
> +  MailServices: "resource:///modules/MailServices.jsm",
> +  Services: "resource://gre/modules/Services.jsm",
> +});

Don't see much point in having these being lazy.

@@ +108,5 @@
> +  ["AnniversaryDay", 0],
> +  ["SpouseName", 0],
> +  ["FamilyName", 0],
> +];
> +const lineBreak = AppConstants.platform == "win" ? "\r\n" : "\n";

LINEBREAK

@@ +165,5 @@
> +}
> +
> +function exportDirectoryToLDIF(directory) {
> +  function appendProperty(name, value) {
> +    if (value) {

early return?

@@ +269,5 @@
> +  let output = "";
> +  for (let card of directory.childCards) {
> +    if (!card.isMailList) {
> +      // We don't know how to export mailing lists to vcf.
> +      // Use LDIF for that.

I suppose it should expand the mailing list and put them all in the vcf?
Anyway, if we don't support it, do an early return instead and throw an error. A silent failure is bad for debugging.

Comment 5

[Geoff Lankow (:darktrojan)]

I suppose it should expand the mailing list and put them all in the vcf?

All contacts will be in the exported file anyway, because they have to be in the address book to be in the mailing list.

Anyway, if we don't support it, do an early return instead and throw an
error. A silent failure is bad for debugging.

No, throwing an error is bad. We just skip the mailing list and carry on. This is exactly what the previous implementation did, including the comment.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1624207-export-addrbook-2.diff

I went down the registry key route, so with this patch exporting should be exactly as it was before. (Almost. Outside Windows I dropped the "system charset" options.)

Comment 8

[Jorg K (CEST = GMT+2)]

The trailing spaces in mailnews/addrbook/test/unit/data/export.txt are intentional?

Comment 9

[Geoff Lankow (:darktrojan)]

Yes. (They're tabs, not spaces.) And the \r characters.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9136971 [details] [diff] [review]
1624207-export-addrbook-2.diff

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

Looks good, r=mkmelin

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9136971 [details] [diff] [review]
1624207-export-addrbook-2.diff

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

Looks good, r=mkmelin

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d42df03dfe10
Implement address book export in javascript. r=mkmelin