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

In bug 1682620 we reported issues with access to the Outlook address book. We decided to limit the scope of that bug by moving the mailing/distribution list issues into a new bug.

See bug 1682620 comment #8 for the first issue we ran into.

We stared at this for a long time wondering why aDirectory->GetURI(uri) didn't work. Finally we noticed that it's not called from a member function. Impossible that that ever worked.

This is pretty self-explanatory. We noticed that the ML had an empty tree row when displaying rows by name. This fixes it. We're not 100% sure whether this is the correct fix, maybe we should set the first name instead, or we're missing something completely.

So far, this is all preliminary stuff. Looks like apart from displaying MLs, nothing works, so add/edit/delete and add/remove card all need to be made to work together with the related notifications. But that's for another day once bug 1682620 has been closed. Add ML has the same issue as add card, we need to create a IPM.DistList message, see:
https://peach.ease.lsoft.com/scripts/wa-PEACH.exe?A2=2012&L=MAPI-L&D=0&P=21023763

These small tweaks can go in already. Dependent on bug 1682620 and bug 1686700.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/36d2fff3464b
Make ExtractDirectoryEntry() a member function so inheritance works. r=darktrojan
https://hg.mozilla.org/comm-central/rev/53047d769ff7
Fix empty mailing list name in tree display when entries are displayed by name. r=darktrojan

We've looked at mailing lists now and found some conceptual issues. We have one Outlook address book (OL AB for short in the future) and one mailing list inside it (ML for short). What we see is that first the AB is initialised via the init() call in createDirectoryObject(), and soon after the ML is initialised via this call here:
https://searchfox.org/comm-central/rev/e5bad7edc0e9cdb1e02f040ac5a2a60b984e9b6f/mailnews/addrbook/modules/AddrBookManager.jsm#236
It's not clear who calls getDirectory() here. It's important that the createDirectoryObject() is made without the store flag, so MLs don't end up in the internal store.
When initialising a ML it gets a UID here:
https://searchfox.org/comm-central/rev/e5bad7edc0e9cdb1e02f040ac5a2a60b984e9b6f/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#639
and stores that against the cards.
Later on we see two more initialisations of the ML, so in total, the ML is initialised three times each time creating a new instance of nsIAbDirectory and never storing that object in the map, and a new UID. And that's just when the system starts up without even having opened the AB window.

When opening the AB window and the OL AB, the ML is initialised two more times. Trying to access a card in the ML fails on this line:
https://searchfox.org/comm-central/rev/e5bad7edc0e9cdb1e02f040ac5a2a60b984e9b6f/mail/components/addrbook/content/abCommon.js#571
That's not a surprise since despite setting the ML's UID on the card, that ML as a directory is not in the map.

That raises two questions:

1. Why are MLs initialised so many times? Is there something missing in the OL code to prevent that?
2. What's the overall design here? If this let directory = MailServices.ab.getDirectoryFromUID(card.directoryUID); is meant to work, how can it work of the directory that corresponds to the ML is not in the map? We must be missing something since non-OL MLs work.

We'd appreciate some insights. We can attach our debug patch and the output, but that's little messy. so we thought we'd describe it in words.

I don't know for sure but think you might be able to join this special case for the right outcome. If you did that currently it looks like you'd end up fatally recurring (because of this getDirectory call) but you could do the instantiation without going back to the manager there. Which makes me wonder if nsAbOutlookDirectory should keep some sort of map of its mailing lists.

If this createDirectoryObject call could be avoided altogether in future that'd be nice.

Thanks for the reply. Unfortunately we don't see how the reply relates to the question. As we said in comment #5, accessing a card from a ML fails here:
https://searchfox.org/comm-central/rev/e5bad7edc0e9cdb1e02f040ac5a2a60b984e9b6f/mail/components/addrbook/content/abCommon.js#570
on let directory = MailServices.ab.getDirectoryFromUID(card.directoryUID);. That's because the card carries a UID for a ML/directory which is not in the AB manager's map (store variable). Why would running the OL AB scheme through this code:
https://searchfox.org/comm-central/rev/e5bad7edc0e9cdb1e02f040ac5a2a60b984e9b6f/mailnews/addrbook/modules/AddrBookManager.jsm#217-234
change that situation?

We looked at the code a bit, fileName is empty and tail is the (hex) entry ID of the OL AB directory. So something like this:

+      dump(`=== |${scheme}| |${fileName}| |${tail}|\n`);
+      if ((scheme == "jsaddrbook" || scheme == "moz-aboutlookdirectory") && tail.startsWith("/")) {
+        let parent;
+        if (scheme == "jsaddrbook") {
+          parent = this.getDirectory(`${scheme}://${fileName}`);
+        } else {
+          parent = this.getDirectory(`${scheme}://${tail}`);
+        }

causes an endless loop right there, regardless of the C++ code. Or what did you have in mind? We don't even know that fileName and tail are meant to mean.

What I'm trying to suggest is in answer to your question 1: prevent calling createDirectoryObject by instead iterating over the directory's childNodes and finding the one you want. That will prevent more initialisations of the mailing list object.

Regarding directoryUID, it should not point to a mailing list. In Thunderbird's world at least, all cards belong to the directory. If this isn't the case in nsAbOutlook* then it needs to be fixed.

Perhaps getDirectoryFromUID should also be capable of getting lists, but to date there's been no need for that, so it isn't.

Thanks for the clarification. The answer for question 1 is clearer now.

For question 2, unless we mis-read the code, the card is populated with the UID of the owner, the ML, and the ML is also a directory, is it not? A ML is treated as a directory when its cards are retrieved. nsAbOutlookDirectory::Init() is called for MLs and then it calls into here:
https://searchfox.org/comm-central/rev/e5bad7edc0e9cdb1e02f040ac5a2a60b984e9b6f/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#866
So for a ML-owned card, we need to get the grandfather.

Yes, I mean the top-most directory. Mailing lists also implementing directory things makes sense but it is really annoying.

We started a new thread on the MAPI forum to see how we can climb up from the ML directory to the owning top-most directory:
https://peach.ease.lsoft.com/scripts/wa-PEACH.exe?A2=MAPI-L;86d97824.2101&S=
Looking with MFCMapi, there doesn't appear to be a "parent" (back-pointer) attribute.

That said, when the mailing list "card" is first retrieved from the top-most directory, we might keep our own parent pointer or look into your idea of some sort of map of [...] mailing lists.

Off topic: At least in TB, a mailing list can contain references to other mailing lists, bug 1287726.

This makes the OL AB play nicely with the AddrBookManager. The patch does the following:

• Change the ML URI from moz-aboutlookdirectory://<ML entry ID> to moz-aboutlookdirectory://<parent entry ID>/<ML entry ID> so we can get from the ML to the parent which is needed in AddrBookManager.
• Only initialise everything once instead of many times. This makes the UI in our debug build considerably faster. And it's the pre-condition to make it work, since repeated MAPI queries of the AB return the same ML with different entry IDs. Note that two entry IDs can point to the same MAPI object, they can't be compared directly, one needs to use IMAPISession::CompareEntryIDs() which obviously the AddrBookManager can't do when comparing the URIs which are essentially entry IDs.
• Make GetChildNodes() and GetChildCards() return the data from the internal data structures like the Mac OS does.
• For a card in a ML, set the UID of the top-most directory, not the UID of the ML (directory). Now double-clicking on a card in an AB opens the card and it can be edited.

We'll request review after some more testing (getting late here).

Linted/clang-format.

A few more tweaks, should be good to go now.

Sorry, additional comment.

Thanks for the review. Updated variable name to dirURI.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/c4760f64129b
Integrate Outlook AB into AddrBookManager. r=darktrojan

We noticed that editing a card in a ML triggered this line:
https://searchfox.org/comm-central/rev/f6018345454b019bd41c50da80c2bb34f042db86/mailnews/addrbook/src/nsAbWinHelper.cpp#1004
since we passed the ML entry ID into the function, but that MAPI object doesn't have the PR_STORE_ENTRYID property, the top-level directory has it. So that's what we need to pass in. Handy that the top-level directory entry ID is now stored in the URI.

Sorry about some bulk renaming of variables, but mapiData for an entry ID has been one of our pet hates from day one. Strings and entry IDs where also mixed up, so this should be clearer now.

Sorry, more renaming. Should be good now.

More renaming, sorry about the noise, we should have checked better.

Remaining now:

• Next we'll be looking into deleting a card from a ML. The existing code doesn't work. You can't open a ML as container and then delete from it.
• Deleting will most likely be related to adding a card to a ML.
• Currently you can edit the ML name only, not the members.
• Delete ML doesn't work.
• Add ML doesn't work, this is similar to add card and we already have advice from the MAPI mailing list. But it needs to be integrated with adding references to the member cards.

So we expect another 4-5 patches here.

You can't open a ML as container and then delete from it.

Well, according to https://docs.microsoft.com/en-us/office/client-developer/outlook/mapi/idistlistimapicontainer you can, but we observed that the "open" fails here:
https://searchfox.org/comm-central/rev/f6018345454b019bd41c50da80c2bb34f042db86/mailnews/addrbook/src/nsAbWinHelper.cpp#503
with error 0x80004002 = MAPI_E_INTERFACE_NOT_SUPPORTED. We'll work it out.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/dd600d2842ec
Fix edit card in ML: Pass top-level directory to property getter/setter. r=darktrojan

(In reply to IT Support from comment #22)

• Next we'll be looking into deleting a card from a ML. The existing code doesn't work. You can't open a ML as container and then delete from it.

That statement was correct after all. This is the answer:
https://peach.ease.lsoft.com/scripts/wa-PEACH.exe?A2=2101&L=MAPI-L&D=0&P=21038451
The card entry ID needs to be removed from the DL's PT_MV_BINARY property.

Some progress.

This is working, but there are two MAPI issues to be resolved, we're waiting on a MAPI developers' mailing list (no pun intended) reply for those:
https://peach.ease.lsoft.com/scripts/wa-PEACH.exe?A2=2101&L=MAPI-L&D=0&P=21045150

Note that for notifications the card needs to use the ML's UID, not its own directory UID which is the top level directory's UID, see: https://hg.mozilla.org/comm-central/rev/c4760f64129b#l2.221

We'll submit the final patch once the MAPI issues are resolved.

Almost ready.

Comment on attachment 9202291 [details] [diff] [review]
Part 5: 1685166-del-card-from-ML.patch

Quite a bit of new C++ code was necessary here. Please note the last part of comment #28 re. the UID.

Thanks for the review. Addressed the issues as follows:
Comment:

  // For the notification we need to notify the directory the card is contained
  // in. We can't use `card->GetDirectoryUID(dirUID);` since for cards in a
  // mailing list the card's UID is the top level UID.

Bad parameter: Added if (aEntryIDs == NULL) return FALSE; at the top.

Comment on attachment 9202533 [details] [diff] [review]
Part 5: 1685166-del-card-from-ML.patch

Turned out that the "one off" property also needs to be updated:
https://peach.ease.lsoft.com/scripts/wa-PEACH.exe?A2=2101&L=MAPI-L&D=0&P=21051684
We'll add some more code to do that.

This also adjusts the "one offs" list. Still we can't delete the last card.

Now deleting the properties when the last DL member is removed, see:
https://peach.ease.lsoft.com/scripts/wa-PEACH.exe?A2=2101&L=MAPI-L&D=0&P=21058165

There is still an issue that after successful deletion of a card, the next deletion fails:
Cannot open MAPI message store 8000ffff.

0x8000ffff is the "oh my God" error, so something terrible went wrong, likely something was left open.

OK, we got this working finally. Changes from the previously reviewed version:

• We need to remove entries from two MV BIN properties, one is the DL members list, the other the so-called "one off" list where Outlook "caches" some information. Lovely :-(
• You can't delete the last entry of a MV BIM property, it needs to be deleted
• Some "boy scout" action making parameter names more consistent and killing one default parameter. Also changed foo* aParam to foo aParam[] in a few places to emphasize the "array-ness" of the parameter.
• And finally we got to the bottom of the "can't delete a second time". Looks like MAPI rendered our stuff stale, so we need to rescan the distribution list (DL).

The patch looks bigger than it is. We can spare Geoff the pain of looking at it again.

We noticed that double-clicking a ML on the right side of the panel where the cards are shown sometimes didn't work. After some investigation, we found this:

When the AB is initialised, we scan for nodes (MLs) and cards here:
https://searchfox.org/comm-central/rev/995d3d7bff5601325f9d7f05d83f256495d07f14/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#834
Turns out that in the second scan the "ML card" is returned with a different entry ID. So it sets a different directory URI that isn't the the list in the AddrBookManager.

This patch aligns the URI with what was previously found. Sometimes, not always, you see the related PRINT: Entry ID for DL replaced: Was: %s Now: %s

An alternative approach would have been to introduce a nsIAbDirectory.compareURIs() function. Please let us know if you prefer this approach.

This is a WIP:

Edit ML name mostly works, however, editing in the tree (F2 or double-click) on the left doesn't propagate the name change into the ML card on the right when clicking on the top level AB on the left to expand the content on the right:
Left:
OL concacts
ML
F2 or double-click on ML to edit name, then single click on OL contacts to see it expanded on the right, ML isn't reflecting the new name.

Delete ML mostly works, the ML is in fact deleted from the tree on the right, but not removed on the left, similar issue as above.

Geoff, if you can spot what's missing, let us know.

Thanks for the review, yes, you found something the needed fixing. We added this at the start of the function:

+  // Initialise output arrays.
+  for (ULONG i = 0; i < aNbProperties; i++) {
+    aEntryIDs[i] = NULL;
+    aNbElements[i] = 0;
+  }

And this at the end:

+  if (!success) {
+    for (ULONG i = 0; i < aNbProperties; i++) {
+      if (aNbElements[i] > 0) delete[] aEntryIDs[i];
+      aEntryIDs[i] = NULL;
+      aNbElements[i] = 0;
+    }
+  }

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/766ef6369225
Fix deletion of card from mailing list. r=darktrojan,benc

Comment on attachment 9203143 [details] [diff] [review]
Part 7: 1685166-fix-ML-fix-edit-delete.patch, WIP

We looking into it further and will submit a new version soon.

Repeating from the relevant bits from comment #39: This is a WIP.

Edit ML name mostly works, however, editing on the left(tree) (F2 or double-click) doesn't propagate the name change into the ML card on the right(list) when clicking on the top level AB on the left(tree) to expand the content on the right(list). Sounds complicated, here a "picture" of the left(tree) side of the panel:

OL concacts
  contact 1
  ML

F2 or double-click on ML to edit name, not displayed on the right(list) at this point (expands the ML members on the right). Then single click on OL contacts to see it expanded on the right(list), ML isn't showing the new name. We noticed that there is no activity in abView.js. What's the mechanism of propagating changes on the left(tree) into the list that may be displayed on the right(list) later?

Editing a ML name on the right(list) updates the name both on the right(list) and on the left(tree).

Delete ML mostly works: When deleting from the right(list) everything works, list removed right and left. When deleting from the left(tree) the ML is deleted from the left(tree), but not removed on the right(list), similar issue as above.

Geoff, if you can spot what's missing, please let us know.

Comment on attachment 9203298 [details] [diff] [review]
Part 7: 1685166-fix-ML-fix-edit-delete.patch, WIP

Looks like the changed ML name doesn't get promoted from the left(tree) to the right(list). When selecting a new directory on the left, the right side is just rebuilt from the .childCards. We need to update the our copy which is stale. We'll resubmit a new patch soon.

Comment on attachment 9203116 [details] [diff] [review]
Part 6: 1685166-align-DL-uri.patch

We'll merge this into the the "fix edit" part.

Let's close this bug after the "fix edit" part, it's becoming too long. We filed bug 1693154 for the remaining work.

This fixes all aspects of editing the name or deleting a mailing list. For the remaining problems, see bug 1693154 (please assign to us).

Please note this change in abCommon.js

-    return abURIArr[0] + "/" + abURIArr[1] + "/" + abURIArr[2];
+    return abURIArr[0] + "//" + abURIArr[2];

According to the comment above, abURIArr[1] is empty, so we see no point in concatenating it. We can revert this change if we missed something.

In general, handling of mailing lists is rather twisted. Mostly the methods are called on the parent with the mailing list as argument, except for edit where the edit function is called directly on the list (and we need to get the parent for the UID of the notification). That makes for some rather confusing code.

Also a bit frustrating if the nomenclature in Thunderbird. A "mailing list" is generally this: https://en.wikipedia.org/wiki/Mailing_list.
Outlook calls groups of contacts, well, you guessed, "contact group", or deeper inside "distribution list". Operating in two worlds of different nomenclature adds additional confusion.

Thanks for the review. We addressed the comments as follows:

you're making it worse by using "dl"

Removed that, called it list in most places now. However, from previous patches we already have DeleteEntryfromDL and GetDlMembersTag. This should be OK since these function are provided by the helper and there we're really in Outlook land.

Perhaps you should check here that this is being called on a mailing list

The function has been removed, see below.

There's a void version of NS_ENSURE_SUCCESS, NS_ENSURE_SUCCESS_VOID.

Done.

Just pass dirUID to this function, ...

Good point, done. The "get parent" function was made unnecessary by passing the dirUID to the notification function, we inlined the code were it was used.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/548837991008
Fix edit/deletion of Outlook ML and enable notifications. r=darktrojan

Comment on attachment 9203800 [details] [diff] [review]
Part 6: 1685166-fix-ML-fix-edit-delete.patch (landed)

Linted and landed.

Thank you. We linted it at some stage, maybe not the final version, sorry about that.