954132 - Commands no longer work in protocol overrides

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Florian Quèze [:florian]]

*** Original post on bio 697 at 2011-02-16 12:00:00 UTC ***

Commands no longer work in protocol overrides since the command system has been reimplemented in JS (bug 953569 (bio 118)).

2 possible fixes:

 - add in the purpleIProtocol interface a baseId attribute, so that the conversation object can attempt to execute commands on the base protocol instead of in the override protocol, thus getting the right commands executed. This solution prevents override protocols from registering override-specific commands.

 - when the override protocol is initialized, list the commands of the base protocol, and re-register them with the id of the override protocol. This would require a change to the imICommandsService (maybe rename listCommands to listCommandsForConversation and add a listCommandsForProtocol method that would exclude the application-global commands from the listing).

clokep: any thoughts about this?

Comment 1

[Florian Quèze [:florian]]

*** Original post on bio 697 at 2011-02-16 12:01:23 UTC ***

Blocking 0.3 final as this probably breaks some features in Google talk.

Comment 2

[Patrick Cloke [:clokep]]

*** Original post on bio 697 at 2011-02-16 13:23:42 UTC ***

(In reply to comment #0)
> Commands no longer work in protocol overrides since the command system has been
> reimplemented in JS (bug 953569 (bio 118)).
> 2 possible fixes:
>  - add in the purpleIProtocol interface a baseId attribute, so that the
> conversation object can attempt to execute commands on the base protocol
> instead of in the override protocol, thus getting the right commands executed.
> This solution prevents override protocols from registering override-specific
> commands.
>  - when the override protocol is initialized, list the commands of the base
> protocol, and re-register them with the id of the override protocol. This would
> require a change to the imICommandsService (maybe rename listCommands to
> listCommandsForConversation and add a listCommandsForProtocol method that would
> exclude the application-global commands from the listing).
> clokep: any thoughts about this?

I like the second solution a bit better. I think for XMPP protocols in particular this makes the most sense since they you might override the XMPP implementation...but then include some different functionality. If we can't think of a concrete example of an extra command that would be registered though then we might not want to bother with it.

Comment 3

[Patrick Cloke [:clokep]]

Attachment: WIP

*** Original post on bio 697 as attmnt 603 at 2011-04-30 02:05:00 UTC ***

Here's a first cut at this, it doesn't work properly, however.  It registers the commands properly (as far as I can tell), but the only ones that work are ones that do NOT require any parameters (i.e. /buzz works, but not /join <room name>).

Pretty much I'm re-registering the prpl-jabber commands under prpl-gtalk. You might think it's annoying to have to call registerCommands in the protocol override function, but it's actually good since if you don't want to inherit commands you can do your own commands structure (for GenericProtocolPrototype) and call this.__proto__.registerCommands() instead. :)

Let me know your thoughts flo!

Comment 4

[Florian Quèze [:florian]]

*** Original post on bio 697 at 2011-04-30 10:28:24 UTC ***

Comment on attachment 8352346 [details] [diff] [review] (bio-attmnt 603)
WIP

Was there any issue with my suggestion from comment 0?
> This would
> require a change to the imICommandsService (maybe rename listCommands to
> listCommandsForConversation and add a listCommandsForProtocol method that would
> exclude the application-global commands from the listing).


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

>+  registerCommands: function() {
>+    // Fake the one part of the conversation object needed to
>+    // get the base protocol's commands
>+    let prplId = this.baseId;
>+    let conversation = { account: { protocol: { id: prplId } } };
>+    // Get the base protocol's commands
>+    let prplCommands = Services.cmd.listCommands(conversation);
>+
>+    // Ignore global commands
>+    this.commands = prplCommands.filter(function(aCommand) {
>+      LOG(aCommand.name + " " + (aCommand.priority == Ci.imICommand.PRIORITY_PRPL));
>+      return (aCommand.priority == Ci.imICommand.PRIORITY_PRPL);

You have no guarantee that a command with PRIORITY_PRPL has actually been registered by the protocol plugin.

>+    }).map(function(aCommand) {
>+      let command = {
>+        name: aCommand.name,
>+        helpString: aCommand.helpString,
>+        priority: aCommand.priority,
>+        run: aCommand.run
>+      };
>+      return command;
>+    });
Why do you need to duplicate all the command objects?

>+    // Use the GenericProtocolPrototype to register the commands
>+    this.__proto__.registerCommands();
Does this line produce the expected result? I would expect "this" to be gtalkProtocol and this.__proto__ to be ForwardProtocolPrototype, which would make it loop infinitely.

Comment 5

[Patrick Cloke [:clokep]]

*** Original post on bio 697 at 2011-04-30 13:12:29 UTC ***

(In reply to comment #4)
> (From update of attachment 8352346 [details] [diff] [review] (bio-attmnt 603) [details])
> Was there any issue with my suggestion from comment 0?
> > This would
> > require a change to the imICommandsService (maybe rename listCommands to
> > listCommandsForConversation and add a listCommandsForProtocol method that would
> > exclude the application-global commands from the listing).
No, I was just trying to hack around not having this. Although it would be pretty easy to add, I suppose.  I'll try that next.

> >diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm
> 
> >+  registerCommands: function() {
> >+    // Fake the one part of the conversation object needed to
> >+    // get the base protocol's commands
> >+    let prplId = this.baseId;
> >+    let conversation = { account: { protocol: { id: prplId } } };
> >+    // Get the base protocol's commands
> >+    let prplCommands = Services.cmd.listCommands(conversation);
> >+
> >+    // Ignore global commands
> >+    this.commands = prplCommands.filter(function(aCommand) {
> >+      LOG(aCommand.name + " " + (aCommand.priority == Ci.imICommand.PRIORITY_PRPL));
> >+      return (aCommand.priority == Ci.imICommand.PRIORITY_PRPL);
> 
> You have no guarantee that a command with PRIORITY_PRPL has actually been
> registered by the protocol plugin.
Ah, I was afraid of this, we'll need to add the function then.

> >+    }).map(function(aCommand) {
> >+      let command = {
> >+        name: aCommand.name,
> >+        helpString: aCommand.helpString,
> >+        priority: aCommand.priority,
> >+        run: aCommand.run
> >+      };
> >+      return command;
> >+    });
> Why do you need to duplicate all the command objects?
Oops, this was me trying a couple of things, it's unnecessary.

> >+    // Use the GenericProtocolPrototype to register the commands
> >+    this.__proto__.registerCommands();
> Does this line produce the expected result? I would expect "this" to be
> gtalkProtocol and this.__proto__ to be ForwardProtocolPrototype, which would
> make it loop infinitely.
This seems to work, the GenericProtocolPrototype function gets called (I put some debug statements in it to make sure, and it was working.)

Comment 6

[Patrick Cloke [:clokep]]

Attachment: WIP2

*** Original post on bio 697 as attmnt 698 at 2011-06-04 23:48:00 UTC ***

This creates a new function to get the commands in the commands service as you asked flo.

This registers the commands for the new protocol (I know it's working as I can type /help in a conversation and it will pop up with everything, I've also checked the debug statements in the loop that calls registerCommand).

Unfortunately, no commands that take in parameters seem to have been properly registered, if you attempt to use them they just get printed into the conversation. I'm wondering if it's something with trying to register what is a command written in C via JavaScript, even though it's all XPCOM. Do you have any ideas about this flo? Thanks!

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8352346 [details] [diff] [review]
WIP

*** Original change on bio 697 attmnt 603 at 2011-06-04 23:48:45 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8352441 [details] [diff] [review]
WIP2

*** Original change on bio 697 attmnt 698 at 2011-06-07 22:19:57 UTC ***

Looks OK. The issue with parameters mentionned in comment 6 was a non-issue. The testcase clokep used was broken :).

Comment 9

[Florian Quèze [:florian]]

*** Original post on bio 697 at 2011-06-07 22:52:11 UTC ***

https://hg.instantbird.org/instantbird/rev/a221a47337b3

Comment 10

[Patrick Cloke [:clokep]]

*** Original post on bio 697 at 2011-06-07 23:04:22 UTC ***

Just wanted to mention on here that the Facebook override purposefully doesn't have commands registered for it since they can't do anything since the XMPP Facebook transport is poorly made.