Closed Bug 518597 Opened 11 years ago Closed 9 years ago

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

Categories

(Thunderbird :: Search, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 12.0

People

(Reporter: sdwilsh, Assigned: florian)

References

Details

(Whiteboard: [no l10n impact])

Attachments

(2 files, 2 obsolete files)

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.
Flags: blocking-thunderbird3?
Assignee: nobody → david.ascher
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Whiteboard: [no l10n impact]
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.
Assignee: david.ascher → nobody
Summary: GLODA search results should contain matched message fragment → [Faceted Search] GLODA search results should contain matched message fragment
Given comment 1, I'm removing this from the blocking list.
Flags: blocking-thunderbird3+ → blocking-thunderbird3-
Flags: wanted-thunderbird3+
Target Milestone: Thunderbird 3.0rc1 → ---
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attachment #589724 - Flags: ui-review?(bwinton)
Attachment #589724 - Flags: review?(bugmail)
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 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.
Attachment #589724 - Flags: review?(bugmail) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #589724 - Attachment is obsolete: true
Attachment #590160 - Flags: ui-review?(bwinton)
Attachment #590160 - Flags: review?(bugmail)
Attachment #589724 - Flags: ui-review?(bwinton)
Attached image Screenshot
Blake, here's a screenshot, just in case it can help you for the ui-review ;).
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 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.
Attachment #590160 - Flags: ui-review?(bwinton) → ui-review-
Attached patch Patch v3Splinter Review
(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.
Attachment #590160 - Attachment is obsolete: true
Attachment #590232 - Flags: ui-review?(bwinton)
Attachment #590232 - Flags: review?(bugmail)
Attachment #590160 - Flags: review?(bugmail)
Comment on attachment 590232 [details] [diff] [review]
Patch v3

Woo!
Attachment #590232 - Flags: review?(bugmail) → review+
Comment on attachment 590232 [details] [diff] [review]
Patch v3

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

Later,
Blake.
Attachment #590232 - Flags: ui-review?(bwinton) → ui-review+
http://hg.mozilla.org/comm-central/rev/916cf00f9afa
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
You need to log in before you can comment on or make changes to this bug.