Closed Bug 954121 Opened 10 years ago Closed 10 years ago

Implement default chat name for getChatRoomDefaultFieldValues for js-proto

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 686 at 2011-02-08 01:08:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Depends on: 954083
Attached patch v1.0 (obsolete) — Splinter Review
*** Original post on bio 686 as attmnt 516 at 2011-02-08 01:08:00 UTC ***

getChatRoomDefaultFieldValues can take aDefaultChatName, currently this is not implemented in jsProtoHelper since we (flo and I) didn't think it was used in the UI...but it is used for auto joining chat rooms: http://lxr.instantbird.org/instantbird/source/instantbird/modules/imWindows.jsm#317

This patch handles the default chat name as a special variable in the account called "defaultChatNameField", this can be seen as: http://hg.instantbird.org/experiments/file/1ca57116c03f/components/ircProtocol.js#l255

I'm not sure this is the best way to do this, so let me know what you think flo.
Attachment #8352257 - Flags: review?(florian)
Blocks: 953944
Comment on attachment 8352257 [details] [diff] [review]
v1.0

*** Original change on bio 686 attmnt 516 at 2011-02-08 09:01:48 UTC ***

I think in libpurple aDefaultChatName can contain several fields at once (for example I believe it does so for XMPP chatrooms that are serialized as name@server/nick).
If this is the case, "defaultChatNameField" would need to be a function that returns an array (or just an object?) or field names to fill and their respective values.

If this is not the case and the default value is always just a single field, then you should put a default value (empty string?) in, the prototype for defaultChatNameField, or check that it exists ("defaultChatNameField" in this) before using it.

(Off-topic but only 3 lines above: do you know if there's a reason for defaultFieldValues to be an array rather than just an object?)
Attachment #8352257 - Flags: review?(florian) → review-
*** Original post on bio 686 at 2011-02-08 11:25:21 UTC ***

(In reply to comment #1)
> (From update of attachment 8352257 [details] [diff] [review] (bio-attmnt 516) [details])
> I think in libpurple aDefaultChatName can contain several fields at once (for
> example I believe it does so for XMPP chatrooms that are serialized as
> name@server/nick).
> If this is the case, "defaultChatNameField" would need to be a function that
> returns an array (or just an object?) or field names to fill and their
> respective values.
The idl shows it as a string (unless I'm reading it wrong): http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/public/purpleIAccount.idl#123 This would be easy enough to do though.

> If this is not the case and the default value is always just a single field,
> then you should put a default value (empty string?) in, the prototype for
> defaultChatNameField, or check that it exists ("defaultChatNameField" in this)
> before using it.
Right, I'll add thos checks in.

> (Off-topic but only 3 lines above: do you know if there's a reason for
> defaultFieldValues to be an array rather than just an object?)
It should probably be a generic object, I'll check and change it in my next patch.
*** Original post on bio 686 at 2011-02-09 23:55:20 UTC ***

(In reply to comment #2)
> (In reply to comment #1)
> > (From update of attachment 8352257 [details] [diff] [review] (bio-attmnt 516) [details] [details])
> > I think in libpurple aDefaultChatName can contain several fields at once (for
> > example I believe it does so for XMPP chatrooms that are serialized as
> > name@server/nick).
> > If this is the case, "defaultChatNameField" would need to be a function that
> > returns an array (or just an object?) or field names to fill and their
> > respective values.
> The idl shows it as a string (unless I'm reading it wrong):
> http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/public/purpleIAccount.idl#123
> This would be easy enough to do though.

Yes, it's a string in the idl file, because it's a serialized version of the description of the chatname.
See how libpurple handles the default chat name in IRC (http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/irc/irc.c#289) and in XMPP (http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/jabber/chat.c#70).
Attached patch v2.0Splinter Review
*** Original post on bio 686 as attmnt 518 at 2011-02-10 00:46:00 UTC ***

This now uses a function to parse the default chat name string.

This makes auto-join work with IRC-JS. (see http://hg.instantbird.org/experiments/file/10e80f27e69b/components/ircProtocol.js#l255)
Attachment #8352259 - Flags: review?(florian)
Comment on attachment 8352257 [details] [diff] [review]
v1.0

*** Original change on bio 686 attmnt 516 at 2011-02-10 00:46:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352257 - Attachment is obsolete: true
Comment on attachment 8352259 [details] [diff] [review]
v2.0

*** Original change on bio 686 attmnt 518 at 2011-02-10 09:17:38 UTC ***

>diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm

>+    if (aDefaultChatName && this.parseDefaultChatName) {

I think I would prefer testing ("parseDefaultChatName" in this), rather than this.parseDefaultChatName.

If you agree this change is an improvement, I can make it before pushing the patch.
Looks great otherwise :).
Attachment #8352259 - Flags: review?(florian) → review+
*** Original post on bio 686 at 2011-02-10 11:23:44 UTC ***

(In reply to comment #5)
> (From update of attachment 8352259 [details] [diff] [review] (bio-attmnt 518) [details])
> >diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm
> 
> >+    if (aDefaultChatName && this.parseDefaultChatName) {
> 
> I think I would prefer testing ("parseDefaultChatName" in this), rather than
> this.parseDefaultChatName.
> 
> If you agree this change is an improvement, I can make it before pushing the
> patch.
That's a bit more clear. I apparently couldn't remember the "in" keyword last night. :)
*** Original post on bio 686 at 2011-02-10 21:34:12 UTC ***

https://hg.instantbird.org/instantbird/rev/bb4e9c7298ae
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3a2
You need to log in before you can comment on or make changes to this bug.