Closed Bug 1143300 Opened 9 years ago Closed 6 years ago

Batch status updates in the UI

Categories

(Instantbird Graveyard :: Conversation, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: aleth, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

We currently call these methods 2-3 times in a row (in response to notifications) when we really only need to repaint the UI once.
Attached patch batchupdates.diff (obsolete) — Splinter Review
Attachment #8577605 - Flags: review?(clokep)
Do we know how much this is saving? This patch seems focused on the conversations. I would expect the blist changes to be slightly more costly (as I expect many more notifications when the status is changing on several buddies of a contact; although batching with an executeSoon will only help if all the accounts received the connection/disconnection at the exact same time, which isn't really likely :-/).

I wonder if there are risks of race conditions. Is it possible that we destroy the conversations immediately after dispatching a notification, and that the attempt to update the UI 'later' will fail in bizarre ways?
Comment on attachment 8577605 [details] [diff] [review]
batchupdates.diff

Clearly Florian wants this one. :-)
Attachment #8577605 - Flags: review?(clokep) → review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> Do we know how much this is saving? This patch seems focused on the
> conversations.

The goal here is not really to improve performance but to avoid flicker. The change in bug 1143238 removes some fragility (that broke for XMPP) but at the cost of possibly displaying intermediate states like joining -> left -> joined.

> I would expect the blist changes to be slightly more costly
> (as I expect many more notifications when the status is changing on several
> buddies of a contact; although batching with an executeSoon will only help
> if all the accounts received the connection/disconnection at the exact same
> time, which isn't really likely :-/).

Indeed, I tried adding the same to contacts/buddies but multiple notifications are 1) rarer than one might think unless contacts are expanded 2) can't be batched as e.g. the status message arrives noticeably later from the server than the status. I don't think it's worth it.

> I wonder if there are risks of race conditions. Is it possible that we
> destroy the conversations immediately after dispatching a notification, and
> that the attempt to update the UI 'later' will fail in bizarre ways?

Good point, I added overrides to the destructors.
Attachment #8577605 - Attachment is obsolete: true
Attachment #8577605 - Flags: review?(florian)
Attachment #8577633 - Flags: review?(florian)
Comment on attachment 8577633 [details] [diff] [review]
batchupdates.diff v2

The work that happened here is likely relevant to Thunderbird and it would make sense to port this, but the current patch as is doesn't make sense to review anymore.
Attachment #8577633 - Flags: review?(florian)
As far as we know, Aleth is no longer working on this. Clokep, Florian, can anybody take over?
Assignee: aleth → nobody
Status: ASSIGNED → NEW
On the behalf of Florian:
Closing bugs related to the Instantbird UI as WONTFIX, as the development of the standalone chat client Instantbird has stopped. Instantbird users are encouraged to migrate to Thunderbird. The user interface of instant messaging in Thunderbird will feel familiar, as the Thunderbird IM support started as a fork of Instantbird.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
On the behalf of Florian:
Closing bugs related to the Instantbird UI as WONTFIX, as the development of the standalone chat client Instantbird has stopped. Instantbird users are encouraged to migrate to Thunderbird. The user interface of instant messaging in Thunderbird will feel familiar, as the Thunderbird IM support started as a fork of Instantbird.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: