518597 - [Faceted Search] GLODA search results should contain matched message fragment

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[Shawn Wilsher :sdwilsh]

It'd be really helpful if the search results that we are presented with contained the part of the message that contains the matched search term[s] instead of just the start of the message.

Comment 1

[David Ascher (:davida)]

While I agree that this bug would be good, I don't think it really blocks, and I'm reluctant to do a quick fix w/o good test coverage.  I know I won't have time for that.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Given comment 1, I'm removing this from the blocking list.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch

This patch:
- ensures the first match of the body text is visible in the displayed snippet
- displays in bold all the matches that are visible in the snippet.

This is important for instant messaging (bug 714733) as IM conversation search results are usually completely meaningless if the 5 first lines of a conversation are displayed.

Comment 4

[Florian Quèze [:florian]]

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

>+            // Unicode characters with a code point >  128 are 2 bytes long.
>+            // Unicode characters with a code point > 2048 are 3 bytes long.

Assume these comments read > 127 and > 2047 (I've fixed that locally already).

Comment 5

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

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

The logic looks good, and it's great to have this feature!

The caveat is that you need to handle the case where we are displaying a gloda query that is not a fulltext query, in which case stashedColumns is not going to be present.  (Its initialization is predicated on query.options.stashColumns being present.)  You can initiate such a query by selecting one of the auto-complete options in the global search box such as a contact or a tag name.

Presumably you would still just want to display the first 5 lines or whatever we do now in such a case?

My only real nit is that the rest of the file uses the Thunderbird idiom of:
  for each (let [, thing] in Iterator(things))
instead of this patch's use of:
  for each (let thing in things)

The reason most Thunderbird code uses the former (or index subscripting) is out of paranoia that some jerk is going to mess with Array.prototype and break that logic.  Since this code lives in its own page, it's not really a concern, but if you could make the change for consistency, that would be great.

The UX review can still happen on this patch unless Blake wants something different for the non-fulltext case too.

Comment 6

[Florian Quèze [:florian]]

Attachment: Patch v2

Comment 7

[Florian Quèze [:florian]]

Attachment: Screenshot

Blake, here's a screenshot, just in case it can help you for the ui-review ;).

Comment 8

[Jb Piacentino]

As far as UI is concerned, may I suggest we look into what FF is doing with in-page search, ie apply a different background/foreground to the found words ? This really stands out

Comment 9

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

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

I agree that we need to make the terms stand out a little more.  I would also like to see the terms highlighted when they appear in the subject or email address.

Finally, one of my matches started off with:
…

This is the *matched* text

and I think we should get rid of the extra newline in that case.

Thanks,
Blake.

Comment 10

[Florian Quèze [:florian]]

Attachment: Patch v3

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #9)

> I agree that we need to make the terms stand out a little more.

This new patch changes the highlights as discussed on IRC.

> I would also like to see the terms highlighted when they appear
> in the subject or email address.

I would prefer if we could handle that in a separate bug.

> Finally, one of my matches started off with:
> …
> 
> This is the *matched* text
> 
> and I think we should get rid of the extra newline in that case.

I added a special case for this.

Comment 11

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

Comment on attachment 590232 [details] [diff] [review]
Patch v3

Woo!

Comment 12

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

Comment on attachment 590232 [details] [diff] [review]
Patch v3

Assuming this matches the last screenshot I saw on IRC, ui-r=me!  :)

Later,
Blake.

Comment 13

[Florian Quèze [:florian]]

http://hg.mozilla.org/comm-central/rev/916cf00f9afa