(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.

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 on attachment 623722 [details] [diff] [review] Patch The signature change is fine by me.

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.

(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 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.