Closed Bug 1003105 Opened 6 years ago Closed 5 years ago

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

Categories

(Thunderbird :: Instant Messaging, defect)

defect
Not set

Tracking

(thunderbird36 affected, thunderbird37 affected, thunderbird38 fixed)

RESOLVED FIXED
Thunderbird 39.0
Tracking Status
thunderbird36 --- affected
thunderbird37 --- affected
thunderbird38 --- fixed

People

(Reporter: aleth, Assigned: aleth)

Details

(Keywords: regression)

Attachments

(1 file, 6 obsolete files)

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.
Attached patch tbfindbar.diff (obsolete) — Splinter Review
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.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attachment #8567500 - Flags: review?(clokep)
Attachment #8567500 - Flags: review?(clokep) → review?(nhnt11)
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.
Attached patch tbfindbar.diff v2 (obsolete) — Splinter Review
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.
Attachment #8567500 - Attachment is obsolete: true
Attachment #8567500 - Flags: review?(nhnt11)
Attachment #8567717 - Flags: review?(nhnt11)
(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...
Attachment #8567717 - Flags: review?(nhnt11)
Attached patch tbfindbar.diff v3 (obsolete) — Splinter Review
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.
Attachment #8567717 - Attachment is obsolete: true
Attachment #8567749 - Flags: review?(nhnt11)
Attached patch tbfindbar.diff v4 (obsolete) — Splinter Review
Sets the focus on the browser after the search to allow convenient scrolling etc. (Else the focus remains on the conv list.)
Attachment #8567749 - Attachment is obsolete: true
Attachment #8567749 - Flags: review?(nhnt11)
Attachment #8567750 - Flags: review?(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.
Attachment #8567750 - Flags: review?(nhnt11) → review-
Attached patch Patch with showLog() (obsolete) — Splinter Review
This patch works for me. Please test.
Attachment #8567750 - Attachment is obsolete: true
Attachment #8567759 - Flags: review?(aleth)
Attachment #8567759 - Attachment description: Patch v2 → Patch with showLog()
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.
Attachment #8567759 - Flags: review?(aleth) → review-
Attached patch tbfindbar.diff v5 (obsolete) — Splinter Review
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...
Attachment #8567759 - Attachment is obsolete: true
Attachment #8567816 - Flags: review?(nhnt11)
Now throws if there's a problem.
Attachment #8567816 - Attachment is obsolete: true
Attachment #8567816 - Flags: review?(nhnt11)
Attachment #8568059 - Flags: review?(nhnt11)
Keywords: regression
Comment on attachment 8568059 [details] [diff] [review]
tbfindbar.diff v6

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

Works well with this patch. Thanks!
Attachment #8568059 - Flags: review?(nhnt11) → review+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0
Target Milestone: Thunderbird 39.0 → Thunderbird 38.0
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
Attachment #8568059 - Flags: approval-comm-aurora?
Target Milestone: Thunderbird 38.0 → Thunderbird 39.0
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?
(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 on attachment 8568059 [details] [diff] [review]
tbfindbar.diff v6

https://hg.mozilla.org/releases/comm-aurora/rev/a62603c77469
Attachment #8568059 - Flags: approval-comm-aurora? → approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.