Last Comment Bug 518597 - [Faceted Search] GLODA search results should contain matched message fragment
: [Faceted Search] GLODA search results should contain matched message fragment
Status: RESOLVED FIXED
[no l10n impact]
:
Product: Thunderbird
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Thunderbird 12.0
Assigned To: Florian Quèze [:florian] [:flo]
:
Mentors:
Depends on: 754824
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-24 10:19 PDT by Shawn Wilsher :sdwilsh
Modified: 2012-05-16 05:03 PDT (History)
7 users (show)
dmose: blocking‑thunderbird3-
dmose: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (4.88 KB, patch)
2012-01-18 17:34 PST, Florian Quèze [:florian] [:flo]
bugmail: review-
Details | Diff | Splinter Review
Patch v2 (5.12 KB, patch)
2012-01-20 03:00 PST, Florian Quèze [:florian] [:flo]
bwinton: ui‑review-
Details | Diff | Splinter Review
Screenshot (77.15 KB, image/png)
2012-01-20 03:03 PST, Florian Quèze [:florian] [:flo]
no flags Details
Patch v3 (6.02 KB, patch)
2012-01-20 09:31 PST, Florian Quèze [:florian] [:flo]
bugmail: review+
bwinton: ui‑review+
Details | Diff | Splinter Review

Description Shawn Wilsher :sdwilsh 2009-09-24 10:19:18 PDT
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) 2009-10-05 11:39:38 PDT
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 (:dmose) 2009-10-07 14:52:58 PDT
Given comment 1, I'm removing this from the blocking list.
Comment 3 Florian Quèze [:florian] [:flo] 2012-01-18 17:34:23 PST
Created attachment 589724 [details] [diff] [review]
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] [:flo] 2012-01-19 02:42:21 PST
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] 2012-01-19 11:26:16 PST
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] [:flo] 2012-01-20 03:00:43 PST
Created attachment 590160 [details] [diff] [review]
Patch v2
Comment 7 Florian Quèze [:florian] [:flo] 2012-01-20 03:03:43 PST
Created attachment 590161 [details]
Screenshot

Blake, here's a screenshot, just in case it can help you for the ui-review ;).
Comment 8 Jb Piacentino 2012-01-20 05:20:34 PST
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) (:☕️) 2012-01-20 07:58:59 PST
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] [:flo] 2012-01-20 09:31:42 PST
Created attachment 590232 [details] [diff] [review]
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] 2012-01-20 11:10:58 PST
Comment on attachment 590232 [details] [diff] [review]
Patch v3

Woo!
Comment 12 Blake Winton (:bwinton) (:☕️) 2012-01-20 11:17:48 PST
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] [:flo] 2012-01-23 03:10:50 PST
http://hg.mozilla.org/comm-central/rev/916cf00f9afa

Note You need to log in before you can comment on or make changes to this bug.