777308 - searches started from autocomplete popup should include IM results

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[Florian Quèze [:florian]]

Attachment: Patch

Steps to reproduce:
- Focus the gloda search box.
- Type something.
- From the autocomplete popup that appears, select "Messages mentioning: <what you typed>"

Expected result:
If chat is enabled, this should search both emails and IM conversations, like pressing enter in the search box does.

Actual result:
Only emails are searched.

Comment 1

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

Comment on attachment 645711 [details] [diff] [review]
Patch

This needs UX signoff since one could argue that "messages mentioning" could be interpreted to be a way for the user to opt just to search e-mails.  (I think it would be super confusing to not have them be consistent, but that's still a UX call.)

This is a fair amount of boilerplate and it already exists in doSearch.  Just call that method instead of duplicating the boilerplate.

And given the recent TB announcements, it would be great if you could talk to JB or whomever to approve/allocate some time to add some basic mozmill tests for gloda search for messages and instant messages.  As is clear from the multiple patches required for the IM landing, there are enough entry paths that automated tests are really required for confidence about any changes to functionality.  I'm not requiring tests for this patch, however.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 645711 [details] [diff] [review]
Patch

I think people who have enabled IM will expect "messages" to include "instant messages", and making the two searches consistent seems like the right thing to do.

Finally, people can use the Account facet to only search email if that's the behaviour they want.

ui-r=me!

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch v2

Patch updated to call doSearch instead of duplicating code.

Unfortunately, calling doSearch from the autocomplete popup of a gloda facet tab breaks the autocomplete popup, as it closes the tab containing the XUL document owning the popup immediately when receiving the autocomplete-did-enter-text notification, before the input->OnTextEntered call at http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1228

I worked around this in this new patch with a setTimeout.
The only way I see to handle this correctly would be to hack the code in glodaFacetTab.js so that a gloda facet tab can easily be recycled and we can stop closing it in doSearch. This may cause more changes than we are willing to take for Tb15 beta though.

Oh, and I definitely agree that it would make sense to invest time in tests during the next few months (ie after the Tb15 release, but before the governance changes take effect).

Comment 4

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

Comment on attachment 647215 [details] [diff] [review]
Patch v2

I had totally forgotten about the search coming from within the page in certain cases.  It sounds like the use of doSearch is a net improvement because we will now be more consistent about closing the current tab, etc.  Thanks very much for the analysis of what was going wrong and the link.  Observer notifications being synchronous are dangerous, if useful.

setTimeout seems reasonable for this case.  If you could update the comment to be a little more detailed about the breakage, that would be great.  Specifically, given what you have said and your existing comment, I would propose:
// The autocomplete-did-enter-text notification is synchronously
// generated by nsAutoCompleteController which will attempt to
// call ClosePopup after we return and then tell that textbox about
// the text entered.  Since doSearch may close the current tab (that
// owns the popup), input would be destroyed and not get the text if
// we invoked it now, so we defer it to the next turn of the event
// loop by using setTimeout.

If I am misunderstanding things, please feel free to make the comment more correct.

r=asuth.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v2 with a better comment

I updated the comment to make it as clear and accurate as possible.

Comment 6

[Florian Quèze [:florian]]

https://hg.mozilla.org/comm-central/rev/b33977bf7d88

Comment 7

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/comm-aurora/rev/ccc8ae2d6b72
https://hg.mozilla.org/releases/comm-beta/rev/0f13613427fc