591730 - Gloda search should use FormatDisplayName from bug 474721

Status: NEW

(Thunderbird :: Search, defect)

Description

[Jim Porter (:squib)]

In bug 474721, I added a common FormatDisplayName function for contacts in the message header. For consistency, this should be used wherever possible. This bug is about doing it for Gloda. I'll add a patch when I get the chance, but anyone is willing to preempt me on that.

Comment 1

[Jim Porter (:squib)]

See bug 563062 for the multi-message summary equivalent of this bug.

Comment 2

[Jim Porter (:squib)]

asuth, do you know if it would be hard to do this? I seem to recall Gloda using the address book's display name at the time of indexing as the name of the contact, which I think is bad (the indexer shouldn't care about that). Also it's annoying when you misspell a contact's name and now Gloda constantly reminds you of your shame. :(

If that's right and I'm not imagining things, that should probably be fixed here for consistency with the other places names/addresses get displayed. It does looks like it's easy to make the code use FormatDisplayName, though.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Gloda listens for changes in address book cards and updates its information from them, so it doesn't just assign names when they come in.

For performance reasons, it is important to not access the nsIMsgDBHdr instances backing a message in the faceted display UI.  (In fact, the whole reason we have the UI is because of the performance implications of accessing the headers.)

It appears we also do not access the address book cards during faceted UI display, probably because our indexing of changed AB cards keep us up to date.  But there is also the inefficiency of the current address book code in performing lookups by e-mail address to be concerned about.  Since we appear to be saving off the UUID info, we may be able to perform faster lookups using those.

So, depending on what data the routine needs, you may need to do more or less work.  Anything that involves an nsIMsgDBHdr needs to be done during the indexing stage of operation, probably in fundattr.js.

Comment 4

[Jim Porter (:squib)]

(In reply to comment #3)
> Gloda listens for changes in address book cards and updates its information
> from them, so it doesn't just assign names when they come in.

Does that happen immediately or whenever Gloda isn't busy? Not that it matters for this bug, I'm just curious.

> For performance reasons, it is important to not access the nsIMsgDBHdr
> instances backing a message in the faceted display UI.  (In fact, the whole
> reason we have the UI is because of the performance implications of accessing
> the headers.)

Assuming that the header display name gets indexed by Gloda, I'd never need to touch the headers directly.

> It appears we also do not access the address book cards during faceted UI
> display, probably because our indexing of changed AB cards keep us up to date. 
> But there is also the inefficiency of the current address book code in
> performing lookups by e-mail address to be concerned about.  Since we appear to
> be saving off the UUID info, we may be able to perform faster lookups using
> those.

That sounds like a good idea, and it should be workable once I figure out the API used to look up cards by UUID.

> So, depending on what data the routine needs, you may need to do more or less
> work.  Anything that involves an nsIMsgDBHdr needs to be done during the
> indexing stage of operation, probably in fundattr.js.

FormatDisplayName expects the following things: an email address, the header's display name, and (optionally) a card. If the card is specified, the email address is only used to check if it's one of your identities.

Currently, the function doesn't distinguish between "there is no card" and "please find the card for me", but that shouldn't be too hard to change. It probably should be changed anyway, since I imagine it has performance implications in the message header too.

As an addendum, this change may also have the benefit of simplifying Gloda a bit, since it wouldn't need to listen for address book card changes anymore. Maybe it would still need to listen for card creation/deletion, though...

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #4)
> Does that happen immediately or whenever Gloda isn't busy? Not that it matters
> for this bug, I'm just curious.

Addressbook change notifications are queued for indexing like all other indexing jobs.  Which is to say, gloda is always indexing unless we are offline (used as a poor proxy for being on battery), with the rate of indexing adaptively varied to avoid affecting other programs.
 
> Assuming that the header display name gets indexed by Gloda, I'd never need to
> touch the headers directly.

It does not; you would need to add that.  (The plan for things like facebook/evite was always to use extensions to correctly re-attribute the messages to their actual sender rather than just doing display munging.  The gloda bugzilla plugin, for example, re-attributes messages to the person who cause the change rather than the bugzilla daemon.)
 
> Currently, the function doesn't distinguish between "there is no card" and
> "please find the card for me", but that shouldn't be too hard to change. It
> probably should be changed anyway, since I imagine it has performance
> implications in the message header too.

The performance implication in the gloda case is mainly because we can be dealing with many messages all at once.  Of course, if it's only used for message result display and not to affect faceting, that's less of a concern since we only show a max of 10 (rather than all 400 messages we might have retrieved.)
 
> As an addendum, this change may also have the benefit of simplifying Gloda a
> bit, since it wouldn't need to listen for address book card changes anymore.
> Maybe it would still need to listen for card creation/deletion, though...

Gloda would still need to listen for address book changes.  The autocompletion logic in the global search box needs the contact names to be current.  Also, there is logic to mine additional data related to the contact, although not all of it is really used.  (I had a patch that added the ability to tag contacts which the search box could then autocomplete on; it never landed.  But it could still be exposed by an extension.)

Comment 6

[Jim Porter (:squib)]

(In reply to comment #5)
> (In reply to comment #4)
> > Does that happen immediately or whenever Gloda isn't busy? Not that it matters
> > for this bug, I'm just curious.
> 
> Addressbook change notifications are queued for indexing like all other
> indexing jobs.  Which is to say, gloda is always indexing unless we are offline
> (used as a poor proxy for being on battery), with the rate of indexing
> adaptively varied to avoid affecting other programs.

This is probably where I got confused ages ago. I'm sure I expected immediate results, and Gloda didn't give me that.

> The performance implication in the gloda case is mainly because we can be
> dealing with many messages all at once.  Of course, if it's only used for
> message result display and not to affect faceting, that's less of a concern
> since we only show a max of 10 (rather than all 400 messages we might have
> retrieved.)

It probably should affect the display of the facets, though I'm not sure how much of a performance cost that would be.

Maybe FormatDisplayName should be used in the indexing process, rather than the display process. This might still have problems, though. Isn't Gloda's indexing code in mailnews/? FormatDisplayName is only in mail/, and not (yet) in suite. On the other hand, if an extension can inject itself into the contact updating like the Bugzilla one does, maybe I can do it that way...

Comment 7

[Jim Porter (:squib)]

(In reply to comment #6)
> It probably should affect the display of the facets, though I'm not sure how
> much of a performance cost that would be.

To clarify, I don't know if what you mean by "affect[ing] faceting" includes the list of email contacts you can facet by. It should never affect anything but the display. Maybe that's still a problem though. Nevertheless, I think using FormatDisplayName in gloda's indexer (somehow) is the easiest way.