Closed Bug 1318974 Opened 4 years ago Closed 4 years ago

New messages sometimes not scrolled into view

Categories

(Chat Core :: General, defect)

defect
Not set
major

Tracking

(thunderbird_esr4551+ fixed, thunderbird51 fixed, thunderbird52 fixed, thunderbird53 fixed)

RESOLVED FIXED
Instantbird 53
Tracking Status
thunderbird_esr45 51+ fixed
thunderbird51 --- fixed
thunderbird52 --- fixed
thunderbird53 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file)

This seems to be a regression from bug 1315479, but it actually just uncovered an existing bug. If we interrupt the displayPendingMessages loop before getNextPendingMessage returned null, the _pendingMessages array isn't cleaned up and getPendingMessagesCount returns a bogus value at the next call.
Blocks: 1315479
Attached patch FixSplinter Review
Attachment #8812626 - Flags: review?(aleth)
Comment on attachment 8812626 [details] [diff] [review]
Fix

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

So that means this bug happens whenever we run out of messages to display just as we run out of time for the current cycle.

I suppose the original idea behind this was to be more generous about letting the array keep growing before scrolling to the end.

The real problem here seems to be that there's two counters, this._nextPendingMessageIndex and this._pendingMessagesDisplayed, one being internal to the stuff that can be overridden.
Attachment #8812626 - Flags: review?(aleth) → review+
https://hg.mozilla.org/comm-central/rev/d4e762ef561de865b6e95befb61beafaf914866d
Bug 1318974 - Cleanup the _pendingMessages array even if getNextPendingMessage never returns null, r=aleth. a=aleth CLOSED TREE
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 53
We should probably uplift this!
(In reply to aleth [:aleth] from comment #4)
> We should probably uplift this!

We can for the sake of correctness, but we backed bug 1315479 out of 52, so the bug is likely invisible there. I think it hardly ever manifests itself when loading old messages, and we are very unlikely to have more than 40 new incoming messages to display at once.
Comment on attachment 8812626 [details] [diff] [review]
Fix

[Approval Request Comment]
User impact if declined: Fixes a relatively rare but annoying edge case
Testing completed (on c-c, etc.): yes
Attachment #8812626 - Flags: approval-comm-esr45?
Attachment #8812626 - Flags: approval-comm-beta?
Attachment #8812626 - Flags: approval-comm-aurora?
Attachment #8812626 - Flags: approval-comm-beta?
Attachment #8812626 - Flags: approval-comm-beta+
Attachment #8812626 - Flags: approval-comm-aurora?
Attachment #8812626 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.