Implement default chat name for getChatRoomDefaultFieldValues for js-proto

RESOLVED FIXED in 0.3a2

Status

Chat Core
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: clokep, Assigned: clokep)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

956 bytes, patch
florian
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

*** Due to BzAPI limitations, the initial description is in comment 1 ***
(Assignee)

Updated

4 years ago
Depends on: 954083
(Assignee)

Comment 1

4 years ago
Created attachment 8352257 [details] [diff] [review]
v1.0

*** 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)
(Assignee)

Updated

4 years ago
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-
(Assignee)

Comment 3

4 years ago
*** 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).
(Assignee)

Comment 5

4 years ago
Created attachment 8352259 [details] [diff] [review]
v2.0

*** 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)
(Assignee)

Comment 6

4 years ago
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+
(Assignee)

Comment 8

4 years ago
*** 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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3a2
You need to log in before you can comment on or make changes to this bug.