1003105 - Clicking on an IM search result brings up the findbar with incorrect content

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[aleth [:aleth]]

STR
Carry out a gloda search for string A.
Click on an IM conversation in the search results.
The IM log opens, along with the findbar.
The findbar string is pre-set to a value the source of which is unclear, and which is usually not found in the conversation, causing a beep. It is certainly not A.

Expected behaviour:
Either the findbar should default to empty, or it should be filled with A.

Comment 1

[aleth [:aleth]]

Attachment: tbfindbar.diff

Calling focus() now fills in the findbar search string with data from the selection or clipboard, which we don't want here as the goal is to highlight the first search result.

Ensuring the first search result got highlighted was also pretty broken, so I fixed that too.

Comment 2

[aleth [:aleth]]

Since it seems the description wasn't clear:

The expected behaviour is you click on a search result and it shows you the log with a findbar and the first hit highlighted. Current behaviour is that the findbar is often populated with a completely different string, and/or a result is highlighted for a split second, then disappears again.

The existing code assumed that if browser._messageDisplayPending was false, all the messages would have been added. But as there are multiple interlocking async loops involved, this is not true - the queue in convbrowser can empty multiple times. 

It also assumed that once a hit was highlighed, adding further messages wouldn't remove the highlight, which is wrong. Unfortunately this means we can't do what the existing code tries to do - show a highlight on the first match already while further messages are being added - without causing flicker, which is more annoying than it helps.

Comment 3

[aleth [:aleth]]

Attachment: tbfindbar.diff v2

I removed the convbrowser changes. Since it's not guaranteed that all the initial messages have been added when _messageDisplayPending is false, code shouldn't rely on that flag (it's private to convbrowser anyway).

I tried to find a way to highlight the first match "early" as the existing code intended, but couldn't find a way to make it work reliably.

Other changes are as discussed on IRC.

Comment 4

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #2)
> The existing code assumed that if browser._messageDisplayPending was false,
> all the messages would have been added. But as there are multiple
> interlocking async loops involved, this is not true - the queue in
> convbrowser can empty multiple times. 

It struck me that this shouldn't actually be true in this case, as the messages are added from the log to convbrowser synchronously. So I investigated a bit more and it appears we are displaying the entire log twice, as showLog gets called twice. I'll have to look at this again to fix this properly...

Comment 5

[aleth [:aleth]]

Attachment: tbfindbar.diff v3

Turns out the log being displayed twice was causing the search result hit to disappear, and not merely the fact messages were being added. This was obscured due to the various promises and other async queues.

After fixing that problem, it was possible to keep much closer to the original code and allow to highlight a match early, while messages are still being added.

To ensure the event listener gets removed, the convbrowser change is necessary. This removes a potential leak in the existing code.

Comment 6

[aleth [:aleth]]

Attachment: tbfindbar.diff v4

Sets the focus on the browser after the search to allow convenient scrolling etc. (Else the focus remains on the conv list.)

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8567750 [details] [diff] [review]
tbfindbar.diff v4

Review of attachment 8567750 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't work, probably because you've removed the call to |this._showLog()|.
The log list is populated but no relevant log is loaded - the browser just keeps showing whatever it was already showing. Clicking on items in the log list show the correct logs for the search result.
The rest of the changes look theoretically ok. I'll try to fix this issue myself and upload a patch so that it just might make the merge.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch with showLog()

This patch works for me. Please test.

Comment 9

[aleth [:aleth]]

Comment on attachment 8567759 [details] [diff] [review]
Patch with showLog()

Review of attachment 8567759 [details] [diff] [review]:
-----------------------------------------------------------------

This just puts the bug back. showLogList is supposed to select the given log, which triggers showLog. If that's not happening, you've found *another* bug - an edge case in showLogList.

Comment 10

[aleth [:aleth]]

Attachment: tbfindbar.diff v5

This fixes an edge case - it wouldn't find the log if the log was Today or Yesterday. Check if that's the one you ran into...

Comment 11

[aleth [:aleth]]

Attachment: tbfindbar.diff v6

Now throws if there's a problem.

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8568059 [details] [diff] [review]
tbfindbar.diff v6

Review of attachment 8568059 [details] [diff] [review]:
-----------------------------------------------------------------

Works well with this patch. Thanks!

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/c7bb83ed817a

Comment 14

[aleth [:aleth]]

Comment on attachment 8568059 [details] [diff] [review]
tbfindbar.diff v6

[Approval Request Comment]
Regression caused by (bug #): nhnt11's async logs patch & Bug 1071337 
User impact if declined: Broken feature.
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low

Comment 15

[Kent James (:rkent)]

There is not enough information here to track the status of this in other versions. Please include at least the regressing bug number, and failing that at least set the affected status flags for previous versions.

Is this an issue in TB 37?

Comment 16

[aleth [:aleth]]

(In reply to Kent James (:rkent) from comment #15)
> There is not enough information here to track the status of this in other
> versions. Please include at least the regressing bug number, and failing
> that at least set the affected status flags for previous versions.
> 
> Is this an issue in TB 37?

It's a bit complicated.

The error in the original bug description is a regression from m-c, Bug 326743 (TB30).
Fixing this uncovers a later regression from Bug 955292 (TB33) in conjunction with Bug 1071337 (TB38).
Of course viewing a log via faceted search was completely broken by Bug 955292 (TB33) until Bug 1069845 (TB38), so you wouldn't encounter this bug for that unfortunate reason.

So yes, it's an issue in TB37, but this patch can't be uplifted there without uplifting its dependencies. When nhnt11 fixes the remaining gloda bugs, we should consider uplifting the whole set of fixes to TB36.

Comment 17

[Kent James (:rkent)]

Comment on attachment 8568059 [details] [diff] [review]
tbfindbar.diff v6

https://hg.mozilla.org/releases/comm-aurora/rev/a62603c77469