Closed Bug 953956 Opened 10 years ago Closed 10 years ago

Extend jsProtoHelper to implement purpleIConvChat

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(2 files, 5 obsolete files)

*** Original post on bio 519 at 2010-09-23 22:15:00 UTC ***

Using from sample code from flo I've started to implement a GenericConversationChatPrototype object to implement the basics of purpleIConvChat (and possibly purpleIConvChatBuddy).  I'll attach a patch once I've finished testing.
Blocks: 953944
Attached patch Sample patch to fix name bug (obsolete) — Splinter Review
*** Original post on bio 519 as attmnt 359 at 2010-09-25 13:53:00 UTC ***

Quick sample patch from flo to possibly fix a bug I'm having with changing the name of a conversation.
*** Original post on bio 519 as attmnt 365 at 2010-10-09 10:07:00 UTC ***

Notifying observers now, the other patch was also missing a "," after the name-setter-function.

I haven't tested this patch, though.
*** Original post on bio 519 as attmnt 366 at 2010-10-09 10:12:00 UTC ***

My bad! Sent wrong aSubject on last patch... this patch v3 fixes this.
Comment on attachment 8352107 [details] [diff] [review]
Updated sample patch to fix name bug

*** Original change on bio 519 attmnt 365 at 2010-10-09 10:12:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352107 - Attachment is obsolete: true
*** Original post on bio 519 as attmnt 423 at 2010-12-14 01:32:00 UTC ***

This patch contains the work I've done for IRC (in terms of jsProtoHelper), this separates GenericConversationPrototype into GenericConversationPrototype, which is inherited into GenericConvIMPrototype and GenericConvChatPrototype.  This does not use setters to set name, etc. but it probably should so protocols do not need to fire observers (as often).

Asking for review on this so we can both work on closer versions of jsProtoHelper (note that this will break anything that used GenericConversationPrototype -- i.e. Omegle protocol, since I removed functionality from it).
Attachment #8352166 - Flags: review?
Comment on attachment 8352101 [details] [diff] [review]
Sample patch to fix name bug

*** Original change on bio 519 attmnt 359 at 2010-12-14 01:34:24 UTC ***

Obsolete from the patch that Mic posted (attachment 8352108 [details] [diff] [review] (bio-attmnt 366))
Attachment #8352101 - Attachment is obsolete: true
Comment on attachment 8352166 [details] [diff] [review]
Patch to add ChatConv, ChatConvBuddy

*** Original change on bio 519 attmnt 423 at 2010-12-14 01:35:12 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352166 - Flags: review? → review?(florian)
Comment on attachment 8352166 [details] [diff] [review]
Patch to add ChatConv, ChatConvBuddy

*** Original change on bio 519 attmnt 423 at 2010-12-15 12:17:11 UTC ***

Is GenericConversationPrototype meant to be used for anything but a base for the 2 derived instances? If not (and it seems you are no longer exporting it), you can probably remove from it the QueryInterface and getInterfaces methods, and the isChat property.

Not sure of what you are doing with the _name property in the GenericConv{IM,Chat} prototypes. That seems unfinished.

Is there any use for having a default value for the name of an uninitialized conversation? If not, we should add an aName parameter in the _init method, and require the name is given at this time, so that we are sure the UI doesn't display briefly the default name before having the right one.

Can't the _init method of GenericConvChatPrototype just set the value of _participants and then forward the call to GenericConversationPrototype._init for the generic stuff?

By the way, was your previous problem with inheritance just another instance of "I don't know why it didn't work last time, was probably something silly somewhere else that got fixed since that", or was there any real problem/solution worth sharing? :)
Attachment #8352166 - Flags: review?(florian) → review-
*** Original post on bio 519 at 2010-12-15 13:32:46 UTC ***

(In reply to comment #6)
> (From update of attachment 8352166 [details] [diff] [review] (bio-attmnt 423) [details])
> Is GenericConversationPrototype meant to be used for anything but a base for
> the 2 derived instances? If not (and it seems you are no longer exporting it),
> you can probably remove from it the QueryInterface and getInterfaces methods,
> and the isChat property.
True, I guess they're not doing much.  Is there any reason to keep these and export this? I don't think there is.

> Not sure of what you are doing with the _name property in the
> GenericConv{IM,Chat} prototypes. That seems unfinished.
>
> Is there any use for having a default value for the name of an uninitialized
> conversation? If not, we should add an aName parameter in the _init method, and
> require the name is given at this time, so that we are sure the UI doesn't
> display briefly the default name before having the right one.
I actually changed to using this soon after filing this patch.

> Can't the _init method of GenericConvChatPrototype just set the value of
> _participants and then forward the call to GenericConversationPrototype._init
> for the generic stuff?
Good idea.

> By the way, was your previous problem with inheritance just another instance of
> "I don't know why it didn't work last time, was probably something silly
> somewhere else that got fixed since that", or was there any real
> problem/solution worth sharing? :)
I have no idea what it was, I tried it again and it worked. So I couldn't tell you, sorry!
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 519 as attmnt 425 at 2010-12-15 18:16:00 UTC ***

This fixes the comments from review and a couple of places where I overflowed the line length.
Attachment #8352168 - Flags: review?(florian)
Comment on attachment 8352166 [details] [diff] [review]
Patch to add ChatConv, ChatConvBuddy

*** Original change on bio 519 attmnt 423 at 2010-12-15 18:16:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352166 - Attachment is obsolete: true
Comment on attachment 8352168 [details] [diff] [review]
Patch v2

*** Original change on bio 519 attmnt 425 at 2010-12-16 02:11:54 UTC ***

>+  _init: function(aAccount, aName) {
>+    this._participants = {};
>+    GenericConversationPrototype._init(aAccount, aName);
>   },

This does not work, and using this.__proto__._init(aAccount, aName); didn't seem to either.  They generate a blank conversation where everything is undefined. Not sure how I missed this, but I'm r- it now.
Attachment #8352168 - Flags: review?(florian) → review-
*** Original post on bio 519 at 2010-12-16 08:06:37 UTC ***

(In reply to comment #9)
> (From update of attachment 8352168 [details] [diff] [review] (bio-attmnt 425) [details])
> >+  _init: function(aAccount, aName) {
> >+    this._participants = {};
> >+    GenericConversationPrototype._init(aAccount, aName);
> >   },
> 
> This does not work, and using this.__proto__._init(aAccount, aName); didn't
> seem to either.  They generate a blank conversation where everything is
> undefined. Not sure how I missed this, but I'm r- it now.

Oops, sorry.

GenericConversationPrototype._init(aAccount, aName); calls the code with this = GenericConversationPrototype, it's not what we want at all.

this.__proto__._init(aAccount, aName)
this here refers to the account object defined in your IRC code. So this.__proto__ (which you should rather write this.prototype when you don't need to set it) refers to GenericConvChatPrototype.
this.prototype.prototype._init would probably work but is dangerous because if something is ever implemented using one more level of inheritance, it would break.

Can you do another attempt please?
GenericConversationPrototype._init.apply(this, [aAccount, aName]);
or shorter:
GenericConversationPrototype._init.apply(this, arguments);
*** Original post on bio 519 at 2010-12-16 08:10:16 UTC ***

Comment on attachment 8352168 [details] [diff] [review] (bio-attmnt 425)
Patch v2

I'll review this again (and try it with an updated version of the Omegle plugin) when you attach the fixed version of the patch, but this seems good to me.
Attached patch Patch v3 (obsolete) — Splinter Review
*** Original post on bio 519 as attmnt 427 at 2010-12-16 14:51:00 UTC ***

Updated with idea from comment 10
Attachment #8352170 - Flags: review?(florian)
Comment on attachment 8352168 [details] [diff] [review]
Patch v2

*** Original change on bio 519 attmnt 425 at 2010-12-16 14:51:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352168 - Attachment is obsolete: true
*** Original post on bio 519 as attmnt 429 at 2010-12-16 19:04:00 UTC ***

Looks good!

This is the same as clokep's attachement 427, with some very minor changes (look at the interdiff - I pastebined it on IRC).
Attachment #8352172 - Flags: review+
Comment on attachment 8352170 [details] [diff] [review]
Patch v3

*** Original change on bio 519 attmnt 427 at 2010-12-16 19:04:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352170 - Attachment is obsolete: true
Attachment #8352170 - Flags: review?(florian)
*** Original post on bio 519 by tymerkaev AT gmail.com at 2010-12-16 19:28:39 UTC ***

http://hg.instantbird.org/instantbird/rev/0166084ce2ae
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
*** Original post on bio 519 at 2011-04-13 15:59:49 UTC ***

Cleaning up some old bugs to add milestones.
Target Milestone: --- → 0.3a1
You need to log in before you can comment on or make changes to this bug.