1693154 - Fix remaining issues with the Outlook address book wrt. to mailing/distribution lists: Add new list, edit list members.

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[IT Support]

+++ This bug was initially created as a clone of Bug #1685166 +++
+++ This bug was initially created as a clone of Bug #1682620 +++

Bug 1685166 is becoming too long and twisted. We'll address the remaining issues here:

• Add new mailing list
• Edit mailing list members.

Comment 1

[Geoff Lankow (:darktrojan)]

You should apply for the editbugs permission so you can make these changes yourself.

Comment 2

[IT Support]

Attachment: Part 1: 1693154-remove-CommitAddressList.patch

We're back after a little break. We noticed that editing the mailing list members on the edit mailing list panel involves this code:
https://searchfox.org/comm-central/rev/3dfd71b2072e9f600ee83ea4acf6c632ef01bcc3/mailnews/addrbook/content/abMailListDialog.js#149-164
In other words, removing a card in the edit process calls mailList.deleteCards(cardsToRemove); and that already works like deleting one or more member cards in the tree/list.
Adding a card in the edit process calls cardForEmailAddress(email) followed by mailList.addCard(card);. As we previously stated, that CreateCard() called in AddCard() isn't working for mailing lists, and we'll fix this after implementing cardForEmailAddress().

In a part 1 we can remove CommitAddressList(). Part 2 will implement cardForEmailAddress(), part 3 will fix CreateCard() and finally part 4 will fix AddMailList(). The hard part is part 3 where we will need to add to the two properties PidLidDistributionListMembers and PidLidDistributionListOneOffMembers of the IPM.DistList.

Comment 3

[IT Support]

Attachment: Part 2: 1693154-cardForEmailAddress.patch

Another easy one. Please see previous comment for the "roadmap". BTW, with this patch adding a card to a mailing list during the ML edit operation fails in CreateCard()as predicted. If it ever worked, that code stopped working with the advent of Outlook 2010 or earlier.

Comment 4

[IT Support]

Attachment: Part 3: 1693154-fix-add-card-to-ML.patch - WIP, not working.

This should be the overall structure. Need to implement AddEntryToDL() .

Comment 5

[IT Support]

Attachment: Part 3: 1693154-fix-add-card-to-ML.patch - WIP, somewhat working (can add card to ML)

Comment 6

[IT Support]

Attachment: Part 3: 1693154-fix-add-card-to-ML.patch - WIP, 99% working

Coming along nicely, can add card to ML now, fully reflected in tree/list. Only remaining issue left is that the first name of a new card ML isn't right. We'll look into it.

Comment 7

[IT Support]

Attachment: Part 3: 1693154-fix-add-card-to-ML.patch

Fully working. We've taken care of the first name issue, apparently another Outlook quirk.

Enjoy some more C++ MAPI code. BTW, we pass wchar_t in AddEntryToDL() since wcslen() doesn't work on char16_t.

Comment 8

[IT Support]

Attachment: Part 3: 1693154-fix-add-card-to-ML.patch

We forgot to cover adding to an empty mailing list. That has been added now.

Comment 9

[IT Support]

Attachment: Part 4: 1693154-fix-add-ML.patch, WIP, not working

Comment 10

[IT Support]

Attachment: Part 4: 1693154-fix-add-ML.patch, WIP, 95% working

It's just a matter of getting the UIDs right. Still a JS error in abView.js:268.

Comment 11

[IT Support]

Attachment: Part 4: 1693154-fix-add-ML.patch

This is working now after some more UID and DL name fun. A few comments:

• nsAbOutlookDirectory::AddMailList() needed a complete rewrite.
• nsAbWinHelper::CreateEntry(), renamed to nsAbWinHelper::CreateEntryInternal() was good enough to create DLs as well with some tweaks.
• The DL name needs to be set via a named attribute whose tag we need to fetch.
• CopyEntry() and GetDefaultContainer() are obsolete now.
• Some boy-scout cleanup: Renamed a few badly named variables, inlined UpdateAddressList().

Comment 12

[IT Support]

Attachment: Part 3: 1693154-fix-add-card-to-ML.patch

We decided to inline CreateCard() into AddCard(). It's a complete rewrite anyway. Sorry about the noise.

Comment 13

[IT Support]

We saw another issue. This code here:
https://searchfox.org/comm-central/rev/fad8a1aa15e6571ccee797087eadcaf67d2c4fa3/mailnews/addrbook/content/abView.js#255
retrieves the UID from all cards in the AB. Since OL cards don't natively have a UID, the getter nsAbCardProperty::GetUID() allocates a new UID and then calls the setter nsAbCardProperty::SetUID() which runs a ModifyCard(). That in MAPI land, that's a very expensive operation.

Sorry, we'll rework part 3 a bit more and allocate a UID straight away. That will avoid all the "fake" modifications.

Comment 14

[IT Support]

Attachment: Part 3: 1693154-fix-add-card-to-ML.patch

Sorry again about the noise. This also addresses the issue mentioned in comment #13. It saves a whole lot of "fake" ModifyCard() calls we saw in our debug log.

Comment 15

[IT Support]

Attachment: Part 4: 1693154-fix-add-ML.patch

Rebased to part 3. Please refer to what we wrote in comment #11.

Comment 16

[IT Support]

Attachment: Part 4: 1693154-fix-add-ML.patch

We forgot to set a few properties of the "list card". Now it displays properly in the list on the right and double-click triggers the edit function. We also found and fixed another Outlook quirk leading to an ugly start-up crash. Further comments re. part 4 in comment #11.

Comment 17

[IT Support]

Attachment: Part 4: 1693154-fix-add-ML.patch

Typo in comment. Sorry.

Comment 18

[Ben Campbell]

Comment on attachment 9207264 [details] [diff] [review]
Part 3: 1693154-fix-add-card-to-ML.patch

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

LGTM

(as an aside, I wonder if there's any compile-time checks anywhere to ensure that wchar_t is actually 16 bits? I can't imagine any compiler on windows _not_ having it at 16 bits, but in theory it could be any size. The only reason I mention it is that I vaguely recall getting bitten by non 16-bit wchar_t, but it _probably_ wasn't on windows :- ).

Comment 19

[IT Support]

Thanks for the review. Well, the underlying assumption is that Mozilla wide strings and MS wide or Unicode strings both use UTF-16. char16_t and wchar_t are the underlying raw string types. They a guaranteed to be the same size:
https://searchfox.org/mozilla-central/rev/44695ef057e422a8d6c6056972bdf32766c36187/mfbt/Char16.h#38

We don't quite understand the fine-print here. On one side, it's OK to pass an nsString.get() onto a wchar_t*, on the other side, it's not OK to do a wcslen() on a char16_t, or as we saw in part 4, const_cast<wchar_t*>(aName) also doesn't work if aName is a char16_t, hence the wchar_t* parameter also on CreateEntryInternal(). BTW, Ben, feel free to take a look at part 4. There's nothing new C++ wise, so that's why we didn't request your review.

In the code base there's quite some mixing of these types, for example:
char16_t* pwszStr = (char16_t*)moz_xmalloc((strLen + 1) * sizeof(WCHAR)); here
https://searchfox.org/comm-central/rev/d53f614c98630c0a73a73f376955f79fa3773b36/mailnews/import/src/MapiApi.cpp#919
or LossyCopyUTF16toASCII(nsDependentString(static_cast<wchar_t*>(result)), val); here:
https://searchfox.org/comm-central/rev/d53f614c98630c0a73a73f376955f79fa3773b36/mailnews/import/src/MapiApi.cpp#1154

Note also bug 1686850.

Comment 20

[Geoff Lankow (:darktrojan)]

Comment on attachment 9207264 [details] [diff] [review]
Part 3: 1693154-fix-add-card-to-ML.patch

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

::: mailnews/addrbook/src/nsAbWinHelper.cpp
@@ +854,5 @@
> +  memcpy(dlEntryId->idBytes, contactId, contactIdLength);
> +  dlMembers.mEntries[dlMembers.mNbEntries].Assign(
> +      dlEntryIdLength, reinterpret_cast<LPENTRYID>(dlEntryId));
> +
> +  // Constrruct a one-off entry.

Typo.

Comment 21

[Geoff Lankow (:darktrojan)]

Comment on attachment 9207273 [details] [diff] [review]
Part 4: 1693154-fix-add-ML.patch

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

::: mailnews/addrbook/src/nsAbOutlookDirectory.cpp
@@ +515,5 @@
> +
> +  // Light-weight initialisation. `nsAbOutlookDirectory::Init()` will get
> +  // the object type wrong since we don't have cards yet and scan for cards
> +  // which we don't have yet.
> +  rv = directory->nsAbDirProperty::Init(uri.get());

I can't see anything that sets the name on directory. That might not be necessary but it's in nsAbOutlookDirectory::Init so I'm wondering if you missed it.

@@ +539,4 @@
>  
> +  // This is a bit of a hack. If set the UID of the card before setting its
> +  // directory UID, we can avoid an unwanted `ModifyCard()` call inside
> +  // `nsAbCardProperty::SetUID()`.

Missing a word?

Comment 22

[IT Support]

Attachment: Part 3: 1693154-fix-add-card-to-ML.patch

Thanks for the review.

Fixed "Constrruct" comment and added the missing word "we" to two comments (were missing in part 3 and part 4).

Comment 23

[IT Support]

Attachment: Part 4: 1693154-fix-add-ML.patch

Added the missing word "we" to the comment. There was also a trailing space issue we noticed, trailing space added in part 3, removed in part 4. That's been rectified.

I can't see anything that sets the name on directory.

The name is set with this call: mapiAddBook->CreateDistList(*mDirEntry, newEntry, name.get()).

In fact, if you dig through the implementation, it is set twice in nsAbWinHelper::CreateEntryInternal(), once via this snippet

  if (strcmp(aContactClass, "IPM.DistList") == 0) {
    // Set distribution list name.
    problems = NULL;
    GetDlNameTag(newEntry.Get(), propValue.ulPropTag);
    propValue.Value.lpszW = const_cast<wchar_t*>(aName);
    mLastError = newEntry->SetProps(1, &propValue, &problems);
    if (HR_FAILED(mLastError)) {
      PRINTF(("Cannot set DL name %08lx.\n", mLastError));
      return FALSE;
    }
  }

on the contact object (see comment #11, 3rd bullet point), and a second time here

  // Construct the entry ID of the related address book entry (IMailUser).
  AbEntryId* abEntryId =
      (AbEntryId*)moz_xmalloc(sizeof(AbEntryId) + value->Value.bin.cb);
[snip]
  mLastError = object->SetProps(1, &displayName, &problems);
  if (HR_FAILED(mLastError)) {
    PRINTF(("Cannot set display name %08lx.\n", mLastError));
    return FALSE;
  }

on the IMailUser object.

Comment 24

[IT Support]

Checkin needed for all four parts.

Comment 25

[IT Support]

Oops, we misunderstood, sorry. You were referring to the SetDirName() call. In our part 4 patch this is covered by rv = newList->CopyMailList(aMailList); is it not?
https://searchfox.org/comm-central/rev/c37e8363a890b225bab89cd9d3d2fdd6d98d6e51/mailnews/addrbook/src/nsAbDirProperty.cpp#215

Comment 26

[IT Support]

If you prefer, we can change the newList->CopyMailList(aMailList) to a newList->SetDirName(name) which would make it more obvious. The name was retrieved some lines above. It's a pity that OL doesn't support the other ML properties.

Comment 27

[Geoff Lankow (:darktrojan)]

(In reply to IT Support from comment #25)

Oops, we misunderstood, sorry. You were referring to the SetDirName() call. In our part 4 patch this is covered by rv = newList->CopyMailList(aMailList); is it not?
https://searchfox.org/comm-central/rev/c37e8363a890b225bab89cd9d3d2fdd6d98d6e51/mailnews/addrbook/src/nsAbDirProperty.cpp#215

Yes it is. I just didn't see it (obviously didn't look closely enough) and wanted to check you hadn't made a mistake.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4177843f8984
Remove unneeded nsAbOutlookDirectory::CommitAddressList(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/4d31e736aa7c
Implement nsAbOutlookDirectory::CardForEmailAddress(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/6236bd177fa9
Fix adding card to ML in Outlook AB. r=darktrojan,benc
https://hg.mozilla.org/comm-central/rev/3089309e2ff7
Fix adding a ML to Outlook AB. r=darktrojan

Comment 29

[IT Support]

Comment on attachment 9206743 [details] [diff] [review]
Part 1: 1693154-remove-CommitAddressList.patch

[Approval Request Comment]
Regression caused by (bug #): It never worked, at least not since Outlook 2010.
User impact if declined: TB access to Outlook address book incomplete.
Testing completed (on c-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky):
This is not risky since TB access to Outlook address book is not a "published" feature, one has to set hidden preferences to enable it. We've been wondering whether to ask for beta approval here. If might be good for this bug here to join it's predecessor, bug 1685166, which was completed in TB 87 (the milestone of 86 reflects the first landing there). That way, interested users could try it out early. On the other hand, 12 days to TB 88 beta won't make a lot of difference.

Request applies to all four parts.

Comment 30

[Wayne Mery (:wsmwk)]

Comment on attachment 9206743 [details] [diff] [review]
Part 1: 1693154-remove-CommitAddressList.patch

[Triage Comment]

On the other hand, 12 days to TB 88 beta won't make a lot of difference.

Agree, there's no urgency