Closed Bug 955597 Opened 7 years ago Closed 7 years ago

Call getChatRoomDefaultFieldValues lazily for awesometab roomInfos

Categories

(Instantbird :: Other, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: nhnt11)

References

Details

Attachments

(1 file, 5 obsolete files)

*** Original post on bio 2157 at 2013-09-06 12:55:00 UTC ***

getChatRoomDefaultFieldValues is currently called for every channel on RPL_LIST, slowing things down. This can just as well be done lazily when the user actually wants to join the channel, at least for IRC.
Blocks: 955503
*** Original post on bio 2157 at 2013-09-06 14:03:23 UTC ***

This probably involves subclassing the PossibleConversation code and storing the channel name and account, then calling getChatRoomDefaultFieldValues on the channel name when it needs to access those values.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2157 as attmnt 2849 at 2013-09-07 22:30:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354619 - Flags: review?(aleth)
Comment on attachment 8354619 [details] [diff] [review]
Patch

*** Original change on bio 2157 attmnt 2849 at 2013-09-08 23:20:28 UTC ***

I'd like to review this.
Attachment #8354619 - Flags: review?(clokep)
Comment on attachment 8354619 [details] [diff] [review]
Patch

*** Original change on bio 2157 attmnt 2849 at 2013-09-08 23:30:47 UTC ***

This seems to make the assumption that chat room field values always just needs the name instead of just adding a getter and subclassing RoomInfo to have an IRC specific version that only takes the name.
Attachment #8354619 - Flags: review?(clokep) → review-
Attached patch Patch 2 (obsolete) — Splinter Review
*** Original post on bio 2157 as attmnt 2851 at 2013-09-08 23:32:00 UTC ***

This makes chatRoomFieldValues a getter.
Attachment #8354621 - Flags: review?(aleth)
Attachment #8354621 - Flags: review?(clokep)
Comment on attachment 8354619 [details] [diff] [review]
Patch

*** Original change on bio 2157 attmnt 2849 at 2013-09-08 23:32:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354619 - Attachment is obsolete: true
Attachment #8354619 - Flags: review?(aleth)
Attached patch Patch 3 (obsolete) — Splinter Review
*** Original post on bio 2157 as attmnt 2852 at 2013-09-08 23:37:00 UTC ***

Missed a return in the previous patch.
Attachment #8354622 - Flags: review?(aleth)
Attachment #8354622 - Flags: review?(clokep)
Comment on attachment 8354621 [details] [diff] [review]
Patch 2

*** Original change on bio 2157 attmnt 2851 at 2013-09-08 23:37:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354621 - Attachment is obsolete: true
Attachment #8354621 - Flags: review?(clokep)
Attachment #8354621 - Flags: review?(aleth)
Comment on attachment 8354622 [details] [diff] [review]
Patch 3

*** Original change on bio 2157 attmnt 2852 at 2013-09-09 12:09:33 UTC ***

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>--- a/chat/protocols/irc/ircBase.jsm
>+++ b/chat/protocols/irc/ircBase.jsm
>@@ -27,16 +27,30 @@ Cu.import("resource:///modules/ircHandle
> Cu.import("resource:///modules/ircUtils.jsm");
> Cu.import("resource:///modules/jsProtoHelper.jsm");
> 
> XPCOMUtils.defineLazyGetter(this, "DownloadUtils", function() {
>   Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
>   return DownloadUtils;
> });
> 
>+function ircRoomInfo(aName, aTopic, aParticipantCount, aAccountId) {
>+  this.name = aName;
>+  this.topic = aTopic;
>+  this.participantCount = aParticipantCount;
>+  this.accountId = aAccountId;
I think this would be easier to store a reference to the account here and then add a getter for accountId (and simplify the getter for chatRoomFieldValues).

>diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js
>+  get _displayName() this._roomInfo.name,
>+  get _accountId() this._roomInfo.accountId,
>+  get _statusText() {
>+    return "(" + this._roomInfo.participantCount + ") " +
>+      (this._roomInfo.topic || _instantbird("noTopic"));
Does this need to be localized?
Attachment #8354622 - Flags: review?(clokep) → review-
*** Original post on bio 2157 at 2013-09-09 12:22:31 UTC ***

(In reply to comment #7)

> >diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js
> >+  get _displayName() this._roomInfo.name,
> >+  get _accountId() this._roomInfo.accountId,
> >+  get _statusText() {
> >+    return "(" + this._roomInfo.participantCount + ") " +
> >+      (this._roomInfo.topic || _instantbird("noTopic"));
> Does this need to be localized?

My understanding is that putting the participant count at the beginning of the topic sucks and we agreed it should be replaced by something else as soon as we have time to polish the awesometab UI (the current focus is on getting the correct results displayed quickly enough, and I don't think we should let us be distracted by something else until it works). This is bug 955593 (bio 2154).

I don't think we need to localize the current placeholder.
Attached patch Patch 4 (obsolete) — Splinter Review
*** Original post on bio 2157 as attmnt 2854 at 2013-09-09 20:50:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354624 - Flags: review?(aleth)
Attachment #8354624 - Flags: review?(clokep)
Comment on attachment 8354622 [details] [diff] [review]
Patch 3

*** Original change on bio 2157 attmnt 2852 at 2013-09-09 20:50:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354622 - Attachment is obsolete: true
Attachment #8354622 - Flags: review?(aleth)
Comment on attachment 8354624 [details] [diff] [review]
Patch 4

*** Original change on bio 2157 attmnt 2854 at 2013-09-09 21:36:01 UTC ***

Comments given on IRC: http://log.bezut.info/instantbird/130909/#m296
Attachment #8354624 - Flags: review-
Attached patch Patch 5 (obsolete) — Splinter Review
*** Original post on bio 2157 as attmnt 2856 at 2013-09-09 22:02:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354626 - Flags: review?(clokep)
Attachment #8354626 - Flags: review?(florian)
Comment on attachment 8354624 [details] [diff] [review]
Patch 4

*** Original change on bio 2157 attmnt 2854 at 2013-09-09 22:02:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354624 - Attachment is obsolete: true
Attachment #8354624 - Flags: review?(clokep)
Attachment #8354624 - Flags: review?(aleth)
Comment on attachment 8354626 [details] [diff] [review]
Patch 5

*** Original change on bio 2157 attmnt 2856 at 2013-09-09 22:03:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354626 - Attachment is patch: true
Attachment #8354626 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 8354626 [details] [diff] [review]
Patch 5

*** Original change on bio 2157 attmnt 2856 at 2013-09-09 22:04:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354626 - Flags: review?(aleth)
Attached patch Patch 6Splinter Review
*** Original post on bio 2157 as attmnt 2857 at 2013-09-09 22:17:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354627 - Flags: review?(aleth)
Attachment #8354627 - Flags: review?(florian)
Attachment #8354627 - Flags: review?(clokep)
Comment on attachment 8354626 [details] [diff] [review]
Patch 5

*** Original change on bio 2157 attmnt 2856 at 2013-09-09 22:17:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354626 - Attachment is obsolete: true
Attachment #8354626 - Flags: review?(florian)
Attachment #8354626 - Flags: review?(clokep)
Attachment #8354626 - Flags: review?(aleth)
Comment on attachment 8354627 [details] [diff] [review]
Patch 6

*** Original change on bio 2157 attmnt 2857 at 2013-09-09 22:32:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354627 - Flags: review?(florian) → review+
Comment on attachment 8354627 [details] [diff] [review]
Patch 6

*** Original change on bio 2157 attmnt 2857 at 2013-09-09 23:50:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354627 - Flags: review?(clokep) → review+
*** Original post on bio 2157 at 2013-09-09 23:55:46 UTC ***

http://hg.instantbird.org/instantbird/rev/f0cd4b1b5222
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
Comment on attachment 8354627 [details] [diff] [review]
Patch 6

*** Original change on bio 2157 attmnt 2857 at 2013-09-09 23:56:47 UTC ***

Clearing aleth's review, we checked it in without him as he hadn't r-ed any of the previous versions.
Attachment #8354627 - Flags: review?(aleth)
You need to log in before you can comment on or make changes to this bug.