1633620 - Stop using URLs to search address books

Status: RESOLVED FIXED

(MailNews Core :: Address Book, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Currently we search an address book directory by appending a LISP-style string to its URL, using the address book manager to create another whole directory object, then reading the childCards property. Except if it's an LDAP address book which does things a different way. This process is synchronous, creates a whole bunch of new objects (which are the same as the existing ones), and quite frankly a mess.

In this bug I'll be adding a search function to nsIAbDirectory which takes the LISP string and a nsIDirSearchListener object and converting the directory implementations to work this way. I won't be removing the old system just yet as it's not my ultimate goal right now.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1633620-async-search-1.diff

Okay, this should do for now. I haven't removed the old query-by-URI stuff yet because it's got pieces all over the place and I want to move forward with things. (For the most part it still works too, I think, although I know at least one of the implementations is now broken.)

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1633620-fix-printing-1.diff

This fixes printing of address books which is the only remaining consumer of nsAbView. It doesn't need search capability so I've removed that piece.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9143901 [details] [diff] [review]
1633620-async-search-1.diff

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

::: mail/components/addrbook/test/browser/browser_mailing_lists.js
@@ +285,5 @@
>      `address book ("${inputs.abName}") is displayed in the address book list`
>    );
>  
>    // Expand the tree to reveal the mailing list.
> +  if (global.dirTree.view.rowCount == 4) {

should this check this?and not just do the click if?

::: mailnews/addrbook/content/abView.js
@@ +3,5 @@
> +var { toXPCOMArray } = ChromeUtils.import(
> +  "resource:///modules/iteratorUtils.jsm"
> +);
> +
> +function abView(directory, searchQuery, listener, sortColumn, sortDirection) {

Objects/Classes should really be capitalized so they are not confused with variables. So, AbView or something non-abberviated.

@@ +33,5 @@
> +    Ci.nsIAbView,
> +    Ci.nsIAbDirSearchListener,
> +    Ci.nsIObserver,
> +    Ci.nsISupportsWeakReference,
> +  ]),

nit: please add an empty line

@@ +134,5 @@
> +
> +  // nsIAbDirSearchListener
> +
> +  onSearchFoundCard(card) {
> +    console.log("onSearchFoundCard", card);

please remove

@@ +138,5 @@
> +    console.log("onSearchFoundCard", card);
> +    this._rowMap.push(new abViewCard(card));
> +  },
> +  onSearchFinished(result, errorMsg) {
> +    console.log("onSearchFinished", result, errorMsg);

remove

@@ +175,5 @@
> +        for (let i = this._rowMap.length - 1; i >= 0; i--) {
> +          if (this._rowMap[i].card.equals(subject)) {
> +            this._rowMap.splice(i, 1);
> +          }
> +        }

should this make sure it actually found something?

::: mailnews/addrbook/jsaddrbook/AddrBookDirectory.jsm
@@ +713,5 @@
> +          list.description
> +        ).asCard
> +    ).concat(Array.from(this._cards.values(), card => this._getCard(card)));
> +
> +    if (query) {

bail out early?

::: mailnews/addrbook/public/nsIAbDirectory.idl
@@ +196,5 @@
>  
>    /**
> +   * Searches the directory for cards matching query.
> +   */
> +  void search(in AString query, in nsIAbDirSearchListener listener);

please document what query is

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9eda2be91b4e
Stop using URLs to search address books. r=mkmelin
https://hg.mozilla.org/comm-central/rev/a34c59e873c9
Fix printing of address books. r=mkmelin

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/351f3b3d81d9
follow-up - Fix broken select-all in address book window. rs=me

Comment 6

[[e-mail]]

Comment on attachment 9143901 [details] [diff] [review]
1633620-async-search-1.diff

>+    // For some unknown reason the tree needs this before the changes show up.
Yeah, well if you actually used the tree API like the old C++ code did then things would work...

>   if (searchURI == kAllDirectoryRoot) {
>     searchURI += "?";
>   }
This looks obsolete to me, but I see other references to a `?`-suffixed directory root, and I don't know what they're trying to do.

>   document
>     .getElementById("localResultsOnlyMessage")
>     .setAttribute(
>       "hidden",
>       !gDirectoryTreeView.hasRemoteAB || searchURI != kAllDirectoryRoot + "?"
>     );
This looks out of date.

>+  ).QueryInterface(Ci.nsITreeView);
Probably unnecessary given that this is now a JS rather than an XPConnect object.

>+    // Change the column header's border colour to force it to redraw.
>+    // Otherwise redrawing doesn't happen until something else causes it to.
... that's really odd. These are just DOM nodes, they should just redraw...