Closed Bug 1633620 Opened 1 year ago Closed 1 year ago

Stop using URLs to search address books

Categories

(MailNews Core :: Address Book, enhancement)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 77.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(2 files)

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.

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

Attachment #9143901 - Flags: review?(mkmelin+mozilla)

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.

Attachment #9143903 - Flags: review?(mkmelin+mozilla)
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
Attachment #9143901 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9143903 - Flags: review?(mkmelin+mozilla) → review+

Pushed by geoff@darktrojan.net:
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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 77.0
Blocks: 1633996
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/351f3b3d81d9
follow-up - Fix broken select-all in address book window. rs=me
Regressions: 1639750
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...
Regressions: 1663935
Regressions: 1678555
Regressions: 1693501
You need to log in before you can comment on or make changes to this bug.