After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 768909 - roomList: Fix tracking of pending messages
roomList: Fix tracking of pending messages
Status: RESOLVED FIXED
Product: polari
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Polari maintainers
Polari maintainers
Depends on:
Blocks:
 
 
Reported: 2016-07-17 23:04 UTC by Florian Müllner
Modified: 2016-07-18 11:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
roomList: Minor clean up (2.40 KB, patch)
2016-07-17 23:04 UTC, Florian Müllner
committed Details | Review
roomList: Connect channel signals immediately if possible (2.52 KB, patch)
2016-07-17 23:04 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2016-07-17 23:04:06 UTC
I could have sworn there was a bug for this already (counter/active style
not getting removed properly), but can't find it for the life of me ...
Comment 1 Florian Müllner 2016-07-17 23:04:10 UTC
Created attachment 331673 [details] [review]
roomList: Minor clean up

Split out a helper function to get rid of nested ifs.
Comment 2 Florian Müllner 2016-07-17 23:04:15 UTC
Created attachment 331674 [details] [review]
roomList: Connect channel signals immediately if possible

We use the channel's ::message-received and ::pending-message-removed
signals to mark rooms as active/inactive and update the highlight
counter. Currently we are only connecting to those signals when the
room's channel changed, so we fail to update rows for rooms that
already had a channel when added to the list. While this is not the
common case, it can still happen, e.g. when someone initiates a
private chat with us or when re-launched by mission-control after
a crash.
Comment 3 Bastian Ilsø 2016-07-18 11:11:24 UTC
Review of attachment 331673 [details] [review]:

looks good to me, one question:

::: src/roomList.js
@@ +97,3 @@
             context.add_class('inactive');
         else
             context.remove_class('inactive');

i didnt find an answer in the doumentation for GtkStyleContext but could there possibly be a scenario where multiple 'inactive' classes can be added and then when we remove inactive classes we dont get all of them removed? or is gtk clever and attempts only to add classes if they aren't already there?
Comment 4 Bastian Ilsø 2016-07-18 11:15:12 UTC
Review of attachment 331674 [details] [review]:

looks good to me.
Comment 5 Florian Müllner 2016-07-18 11:17:41 UTC
(In reply to Bastian Ilsø from comment #3)
> or is gtk clever and attempts only to add classes if they aren't already there?

It's clever:
https://git.gnome.org/browse/gtk+/tree/gtk/gtkcssnodedeclaration.c#n296
Comment 6 Florian Müllner 2016-07-18 11:19:44 UTC
Attachment 331673 [details] pushed as ee00e30 - roomList: Minor clean up
Attachment 331674 [details] pushed as 5a0d871 - roomList: Connect channel signals immediately if possible