Last Comment Bug 754914 - Highlight the search term in IM conversation search results
: Highlight the search term in IM conversation search results
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 15.0
Assigned To: Florian Quèze [:florian] [:flo]
:
Mentors:
Depends on: 759699 763866
Blocks: 743235
  Show dependency treegraph
 
Reported: 2012-05-14 10:38 PDT by Florian Quèze [:florian] [:flo]
Modified: 2012-06-12 02:50 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (12.15 KB, patch)
2012-05-14 10:38 PDT, Florian Quèze [:florian] [:flo]
bwinton: review-
bwinton: ui‑review-
bugmail: feedback+
Details | Diff | Splinter Review
Patch v2 (13.64 KB, patch)
2012-05-18 11:50 PDT, Florian Quèze [:florian] [:flo]
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] 2012-05-14 10:38:57 PDT
Created attachment 623722 [details] [diff] [review]
Patch

(Blake Winton (:bwinton - Thunderbird UX) from bug 743235 comment #6)

> When I click on an IM search result (in my case, Twitter), it would be nice
> to highlight the message that matched somehow.  (Email only shows one
> message, so there's less ambiguity as to which one you should be looking at.)

It seemed easy, but I had to change lots of little details that weren't exactly right but were barely noticeable without this in the chat tab. (We were redisplaying the log content and list of logs way too often; which is now very undesirable as it makes the highlighted result disappear.)

Requesting review from Blake, but I would like a confirmation that Andrew doesn't see anything wrong with changing the parameter of the showConversationInTab method in glodaFacetView.js so I requested feedback from Andrew too.
Comment 1 Florian Quèze [:florian] [:flo] 2012-05-14 10:42:51 PDT
Also, the conversation being displayed asynchronously in chunks makes things more difficult. I had to add setTimeout calls to tell the findbar to find again until the result is found. The only case where these timeouts won't be stopped before the whole conversation has been displayed is if the found search term isn't in the displayed conversation. This shouldn't happen, but it can currently happen because of bug 754824 that I noticed while working on this, and debugged today.
Comment 2 Andrew Sutherland [:asuth] 2012-05-14 13:05:10 PDT
Comment on attachment 623722 [details] [diff] [review]
Patch

The signature change is fine by me.
Comment 3 Blake Winton (:bwinton) (:☕️) 2012-05-17 12:33:52 PDT
Comment on attachment 623722 [details] [diff] [review]
Patch

So, based on the bug of showing the participants in the search results, (as per http://dl.dropbox.com/u/2301433/Screenshots/SearchIrc.png) I'm going to say ui-r-.  Aside from that, it seems okay, though.  I do like how you pre-fill the find box, but I think it should also be focused, so that I can close it with <esc>.

> Also, the conversation being displayed asynchronously in chunks
> makes things more difficult. I had to add setTimeout calls to
> tell the findbar to find again until the result is found.

Instead of this, I think I'ld prefer to see the asynchronous display code send an event that the find code (and testing code!) could listen for to know when something new has been drawn.  Something like 'Services.obs.addObserver(this, "conversation-loaded", false);'?  ;)

>+++ b/mail/base/content/glodaFacetBindings.xml
>@@ -1548,24 +1548,24 @@
>         // -- eventify
>         subject.onclick = function(aEvent) {
>-          FacetContext.showConversationInTab(message,
>+          FacetContext.showConversationInTab(this,
>                                              aEvent.button == 1);

So, on Mac, I think we usually use Cmd-Click to open in the background, instead of, uh, whatever button 1 is…  ;)

>+++ b/mail/base/content/glodaFacetView.js
>@@ -827,33 +827,36 @@ var FacetContext = {
>   /**
>    * Show the conversation in a new glodaList tab.
>    *
>-   * @param {GlodaConversation} aConversation The conversation to show.
>+   * @param {glodaFacetBindings.xml#result-message} aResultMessage The
>+   * result the user wants to see in more details.

Nit: We should indent the second line by four spaces.

>+++ b/mail/components/im/content/chat-messenger-overlay.js
>@@ -441,37 +457,47 @@ var chatHandler = {
>+      if (item.shouldDisplayConversation) {
>+        this._displayedLog = log.path;
>+        this._showLog(conv, item.searchTerm || undefined);

Why do you need that "|| undefined"?

Other than that, I like it, but switching to the listener seems like a large enough change to go for ui-r-.

Thanks,
Blake.
Comment 4 Florian Quèze [:florian] [:flo] 2012-05-18 11:50:49 PDT
Created attachment 625185 [details] [diff] [review]
Patch v2

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

> >+++ b/mail/base/content/glodaFacetBindings.xml
> >@@ -1548,24 +1548,24 @@
> >         // -- eventify
> >         subject.onclick = function(aEvent) {
> >-          FacetContext.showConversationInTab(message,
> >+          FacetContext.showConversationInTab(this,
> >                                              aEvent.button == 1);
> 
> So, on Mac, I think we usually use Cmd-Click to open in the background,
> instead of, uh, whatever button 1 is…  ;)

I haven't changed this behavior, which isn't directly related to highlighting search terms in IM conversations. Fwiw, button == 1 is middle click, which doesn't really apply for people (like me) using mostly the Macbook's touchpad.

> >+++ b/mail/components/im/content/chat-messenger-overlay.js
> >@@ -441,37 +457,47 @@ var chatHandler = {
> >+      if (item.shouldDisplayConversation) {
> >+        this._displayedLog = log.path;
> >+        this._showLog(conv, item.searchTerm || undefined);
> 
> Why do you need that "|| undefined"?

The code in _handleArgs sets item.searchTerm conditionally, so it's possible that item doesn't have a searchTerm property.
The "|| undefined" prevents a "item has no searchTerm property" JS strict warning.

Note: the change to chat/content/convbrowser.xml has r=aleth,clokep over IRC in #instantbird.
Comment 5 Blake Winton (:bwinton) (:☕️) 2012-05-28 08:36:13 PDT
Comment on attachment 625185 [details] [diff] [review]
Patch v2

Okay, I pretty much like the way this works now, and based on the inter-diff inspection, I'm happy with the code changes, too.

r=me, ui-r=me!

Thanks,
Blake.
Comment 6 Florian Quèze [:florian] [:flo] 2012-05-29 08:56:37 PDT
https://hg.mozilla.org/comm-central/rev/9849318c2099

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