Closed Bug 1624207 Opened 2 months ago Closed 2 months ago

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

Categories

(Thunderbird :: Address Book, defect)

x86_64
All
defect
Not set
normal

Tracking

(thunderbird68 unaffected, thunderbird75+ affected)

RESOLVED FIXED
Thunderbird 76.0
Tracking Status
thunderbird68 --- unaffected
thunderbird75 + affected

People

(Reporter: walts48, Assigned: darktrojan)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image 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.

Keywords: regression

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

Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attached patch 1624207-export-addrbook-1.diff (obsolete) — Splinter Review

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.
Attachment #9135877 - Flags: feedback?(mkmelin+mozilla)

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 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.
Attachment #9135877 - Flags: feedback?(mkmelin+mozilla) → feedback+

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.

Duplicate of this bug: 1625146

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.)

Attachment #9135877 - Attachment is obsolete: true
Attachment #9136971 - Flags: review?(mkmelin+mozilla)

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

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

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

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

Looks good, r=mkmelin
Attachment #9136971 - Flags: review?(mkmelin+mozilla) → review-
Comment on attachment 9136971 [details] [diff] [review]
1624207-export-addrbook-2.diff

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

Looks good, r=mkmelin
Attachment #9136971 - Flags: review- → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/d42df03dfe10
Implement address book export in javascript. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 76.0
You need to log in before you can comment on or make changes to this bug.