Bug 70434 - Idle: add IrcCommand extension
Summary: Idle: add IrcCommand extension
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: idle (show other bugs)
Version: unspecified
Hardware: Other All
: medium enhancement
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-10-13 20:11 UTC by Guillaume Desmottes
Modified: 2013-10-16 15:49 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
add IrcCommand extension (6.71 KB, patch)
2013-10-13 20:12 UTC, Guillaume Desmottes
Details | Splinter Review
distribute Irc_Command interface in tarballs (777 bytes, patch)
2013-10-14 14:10 UTC, Simon McVittie
Details | Splinter Review
Rename IRC_Command interface to IRCCommand1 (8.94 KB, patch)
2013-10-14 14:11 UTC, Simon McVittie
Details | Splinter Review
IRC_Command: remove possible errors (1.25 KB, patch)
2013-10-14 20:03 UTC, Guillaume Desmottes
Details | Splinter Review
IRC_Command: add Send() example (1.26 KB, patch)
2013-10-14 20:04 UTC, Guillaume Desmottes
Details | Splinter Review
IRC_Command: add Send() example (1.30 KB, patch)
2013-10-15 17:52 UTC, Guillaume Desmottes
Details | Splinter Review
IRC_Command: remove errors that doesn't make sense for Send() (1.15 KB, patch)
2013-10-15 17:53 UTC, Guillaume Desmottes
Details | Splinter Review
IRC_Command: explain why the command is in UTF-8 (1.26 KB, patch)
2013-10-15 17:53 UTC, Guillaume Desmottes
Details | Splinter Review
IRC_Command: prevent user of sending commands for which we have proper API (3.82 KB, patch)
2013-10-15 17:53 UTC, Guillaume Desmottes
Details | Splinter Review

Description Guillaume Desmottes 2013-10-13 20:11:32 UTC
Proper IRC clients are able to send arbitrary IRC commands to the server. For example, I use /bip to communicate with my bouncer.

I implemented a very simple Idle specific extension doing just that.
Comment 1 Guillaume Desmottes 2013-10-13 20:12:13 UTC
Created attachment 87568 [details] [review]
add IrcCommand extension
Comment 2 Xavier Claessens 2013-10-13 20:26:52 UTC
I would rename the interface to IrcCommand1 to be next-ready. Other than that, +1.
Comment 3 Guillaume Desmottes 2013-10-13 20:38:10 UTC
Renamed and merged, thanks.
Comment 4 Simon McVittie 2013-10-14 11:49:25 UTC
Comment on attachment 87568 [details] [review]
add IrcCommand extension

Review of attachment 87568 [details] [review]:
-----------------------------------------------------------------

::: extensions/Connection_Interface_Irc_Command.xml
@@ +1,2 @@
> +<?xml version="1.0" ?>
> +<node name="/Connection_Interface_Irc_Command" xmlns:tp="http://telepathy.freedesktop.org/wiki/DbusSpec#extensions-v0">

Add to telepathy-spec (when ready), please. I'm trying to reduce the number of extensions in CMs, and this is no worse than Conn.I.Cellular.

We'd normally use the case combination "IRC_Command" (leading to TpSvcConnectionInterfaceIRCCommand, connection_interface_irc_command etc.)

I'd be tempted to call it /Connection_Interface_IRC_Command1 from the start, so the generated code doesn't need a sed application in next.

@@ +23,5 @@
> +    <tp:requires interface="org.freedesktop.Telepathy.Connection"/>
> +    <method name="Send" tp:name-for-bindings="Send">
> +      <arg direction="in" name="Command" type="s">
> +        <tp:docstring>
> +          The command followed by its arguments.

Are IRC commands guaranteed to be representable in UTF-8, or should we be sending binary? Or should we transcode UTF-8 into the configured server charset like we do for PRIVMSGs?

I think we need examples, because the conventional client syntax "/badger snake" does not actually exist in the IRC protocol (although clients that do not understand /badger would frequently interpret it as "/quote badger snake" which means "send the literal string 'badger snake' to the server).

For instance, "MODE #telepathy +s" or "PRIVMSG xclaesse :are you there?" might make good examples.

If the user sends a command that does something Telepathy can already do, should Idle do things like spawning a channel and emitting MessageSent as a side-effect? I think the answer should be "yes" but I could be persuaded otherwise. Other possibilities are "remain silent" and "raise an error".

I think this method call should return an opaque string token, and there should be a signal CommandResponse(s Token, s Response) or CommandResponse(s Token, ay Response), depending whether we decide IRC is a text stream or a bytestream. Alternatively, we could wait for the server response, translate it into success-or-error and return it, and advise clients to set a longer-than-default timeout if the command might take a while.

@@ +34,5 @@
> +        <tp:error name="org.freedesktop.Telepathy.Error.Disconnected"/>
> +        <tp:error name="org.freedesktop.Telepathy.Error.NetworkError"/>
> +        <tp:error name="org.freedesktop.Telepathy.Error.NotAvailable"/>
> +        <tp:error name="org.freedesktop.Telepathy.Error.InvalidArgument"/>
> +        <tp:error name="org.freedesktop.Telepathy.Error.PermissionDenied"/>

When would you get these errors? Disconnected and NetworkError I can see, but how do the rest work? Are we expecting the CM to interpret the response?
Comment 5 Simon McVittie 2013-10-14 11:49:48 UTC
Reopening. I don't consider this to have been fixed until it's in telepathy-spec, I'm afraid.
Comment 6 Simon McVittie 2013-10-14 14:10:18 UTC
Created attachment 87601 [details] [review]
distribute Irc_Command interface in tarballs

Not doing so breaks distcheck.
Comment 7 Simon McVittie 2013-10-14 14:11:36 UTC
Created attachment 87602 [details] [review]
Rename IRC_Command interface to IRCCommand1

---

Here's the diff for just the XML file, with 'git diff -M', since it won't be very reviewable in Splinter:

similarity index 94%
rename from extensions/Connection_Interface_Irc_Command.xml
rename to extensions/Connection_Interface_IRC_Command1.xml
index 625bd9f..c46791b 100644
--- a/extensions/Connection_Interface_Irc_Command.xml
+++ b/extensions/Connection_Interface_IRC_Command1.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" ?>
-<node name="/Connection_Interface_Irc_Command" xmlns:tp="http://telepathy.freedesktop.org/wiki/DbusSpec#extensions-v0">
+<node name="/Connection_Interface_IRC_Command1" xmlns:tp="http://telepathy.freedesktop.org/wiki/DbusSpec#extensions-v0">
   <tp:copyright> Copyright (C) 2005, 2006 Collabora Limited </tp:copyright>
   <tp:copyright> Copyright (C) 2005, 2006 Nokia Corporation </tp:copyright>
   <tp:copyright> Copyright (C) 2006 INdT </tp:copyright>
@@ -18,7 +18,7 @@ Lesser General Public License for more details.</p>
 License along with this library; if not, write to the Free Software
 Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.</p>
   </tp:license>
-  <interface name="org.freedesktop.Telepathy.Connection.Interface.IrcCommand1"
+  <interface name="org.freedesktop.Telepathy.Connection.Interface.IRCCommand1"
     tp:causes-havoc='not well-tested'>
     <tp:requires interface="org.freedesktop.Telepathy.Connection"/>
     <method name="Send" tp:name-for-bindings="Send">
Comment 8 Simon McVittie 2013-10-14 14:13:31 UTC
(In reply to comment #4)
> Are IRC commands guaranteed to be representable in UTF-8, or should we be
> sending binary? Or should we transcode UTF-8 into the configured server
> charset like we do for PRIVMSGs?

Not addressed by my patches

> I think we need examples

Not addressed by my patches, but from the regression test it appears my interpretation was correct: Send("PRIVMSG xclaesse :hi") would be valid, Send("/msg xclaesse hi") would not.

> When would you get these errors?

Also not addressed.
Comment 9 Guillaume Desmottes 2013-10-14 18:57:04 UTC
Comment on attachment 87601 [details] [review]
distribute Irc_Command interface in tarballs

Review of attachment 87601 [details] [review]:
-----------------------------------------------------------------

ooops

++
Comment 10 Guillaume Desmottes 2013-10-14 18:57:29 UTC
Comment on attachment 87602 [details] [review]
Rename IRC_Command interface to IRCCommand1

Review of attachment 87602 [details] [review]:
-----------------------------------------------------------------

++
Comment 11 Guillaume Desmottes 2013-10-14 19:46:55 UTC
> Are IRC commands guaranteed to be representable in UTF-8, or should we be
> sending binary? Or should we transcode UTF-8 into the configured server
> charset like we do for PRIVMSGs?

The conversion is done from UTF-8 by idle_connection_hton() which is called by _send_with_priority(), so using UTF-8 as input is fine here, isn't it?

> I think we need examples, because the conventional client syntax "/badger
> snake" does not actually exist in the IRC protocol (although clients that do
> not understand /badger would frequently interpret it as "/quote badger
> snake" which means "send the literal string 'badger snake' to the server).
> 
> For instance, "MODE #telepathy +s" or "PRIVMSG xclaesse :are you there?"
> might make good examples.

Ok, I'll add examples.

> If the user sends a command that does something Telepathy can already do,
> should Idle do things like spawning a channel and emitting MessageSent as a
> side-effect? I think the answer should be "yes" but I could be persuaded
> otherwise. Other possibilities are "remain silent" and "raise an error".

humm good point. Let's raise an error for now? If there is TP API to do that use the TP API.

> I think this method call should return an opaque string token, and there
> should be a signal CommandResponse(s Token, s Response) or CommandResponse(s
> Token, ay Response), depending whether we decide IRC is a text stream or a
> bytestream. Alternatively, we could wait for the server response, translate
> it into success-or-error and return it, and advise clients to set a
> longer-than-default timeout if the command might take a while.

We are not garanteed to receive a reply so in any case we should have a timeout to not keep the token in memory forever.

> @@ +34,5 @@
> > +        <tp:error name="org.freedesktop.Telepathy.Error.Disconnected"/>
> > +        <tp:error name="org.freedesktop.Telepathy.Error.NetworkError"/>
> > +        <tp:error name="org.freedesktop.Telepathy.Error.NotAvailable"/>
> > +        <tp:error name="org.freedesktop.Telepathy.Error.InvalidArgument"/>
> > +        <tp:error name="org.freedesktop.Telepathy.Error.PermissionDenied"/>
> 
> When would you get these errors? Disconnected and NetworkError I can see,
> but how do the rest work? Are we expecting the CM to interpret the response?

Leftover from copy pasting, I'll remove it.
Comment 12 Guillaume Desmottes 2013-10-14 20:03:50 UTC
Created attachment 87625 [details] [review]
IRC_Command: remove possible errors
Comment 13 Guillaume Desmottes 2013-10-14 20:04:06 UTC
Created attachment 87626 [details] [review]
IRC_Command: add Send() example
Comment 14 Simon McVittie 2013-10-15 10:20:21 UTC
(In reply to comment #11)
> > Are IRC commands guaranteed to be representable in UTF-8, or should we be
> > sending binary? Or should we transcode UTF-8 into the configured server
> > charset like we do for PRIVMSGs?
> 
> The conversion is done from UTF-8 by idle_connection_hton() which is called
> by _send_with_priority(), so using UTF-8 as input is fine here, isn't it?

OK. When I wrote that I was reading the spec, not the implementation: specifications that don't specify this sort of thing considered harmful :-)

I think this is worth calling out in the spec, maybe something like this:

The command is supplied in UTF-8 (because strings on D-Bus are always UTF-8). It is transcoded into the connection's configured character set, if different, before sending to the server.

> > If the user sends a command that does something Telepathy can already do,
> > should Idle do things like spawning a channel and emitting MessageSent as a
> > side-effect? I think the answer should be "yes" but I could be persuaded
> > otherwise. Other possibilities are "remain silent" and "raise an error".
> 
> humm good point. Let's raise an error for now? If there is TP API to do that
> use the TP API.

Raising an error for now seems OK: there can't be any existing code that relies on particular commands working. Let's spec that 

We should perhaps take the "signal it as if they'd used the right API" approach for future feature additions.

> > I think this method call should return an opaque string token, and there
> > should be a signal CommandResponse(s Token, s Response) or CommandResponse(s
> > Token, ay Response), depending whether we decide IRC is a text stream or a
> > bytestream. Alternatively, we could wait for the server response, translate
> > it into success-or-error and return it, and advise clients to set a
> > longer-than-default timeout if the command might take a while.
> 
> We are not garanteed to receive a reply so in any case we should have a
> timeout to not keep the token in memory forever.

Reading the IRC RFC again, there's no way to tell how numeric (server-to-client) responses relate to textual (client-to-server) commands: commands aren't guaranteed to provoke a response, and responses are only distinguished by order). If we have this message sequence:

    --> FOO cassidy :here is a thing
    --> BAR #telepathy :whatever
    <-- 481 :Permission Denied- You're not an IRC operator

we can't distinguish between two possibilities: either there was no reply to the FOO command, and the BAR command got error 481; or the FOO command got error 481, and there was no reply (yet) to the BAR command.

So, never mind, we can't do this in a machine-readable way at all - the best we can do is to send arbitrary textual messages to the user (see also Bug #39157, Bug #26696, and I just opened Bug #70489).
Comment 15 Simon McVittie 2013-10-15 10:24:52 UTC
(In reply to comment #14)
> Raising an error for now seems OK: there can't be any existing code that
> relies on particular commands working. Let's spec that 

Er, that should have said: Let's spec that implementations MAY raise <insert error here> for commands that have a more appropriate D-Bus API.

InvalidArgument seems the closest we have, or we could add a new error, NotLikeThat or UseBetterAPI or something?
Comment 16 Simon McVittie 2013-10-15 10:26:09 UTC
Comment on attachment 87625 [details] [review]
IRC_Command: remove possible errors

Review of attachment 87625 [details] [review]:
-----------------------------------------------------------------

::: extensions/Connection_Interface_IRC_Command1.xml
@@ -31,4 @@
>          <p>Send an arbitrary IRC command to the server.</p>
>        </tp:docstring>
>        <tp:possible-errors>
> -        <tp:error name="org.freedesktop.Telepathy.Error.Disconnected"/>

Disconnected is perfectly reasonable. IRCCommand1.Send shouldn't work until we're connected.

@@ -31,5 @@
>          <p>Send an arbitrary IRC command to the server.</p>
>        </tp:docstring>
>        <tp:possible-errors>
> -        <tp:error name="org.freedesktop.Telepathy.Error.Disconnected"/>
> -        <tp:error name="org.freedesktop.Telepathy.Error.NetworkError"/>

Also perfectly reasonable.

@@ -34,5 @@
> -        <tp:error name="org.freedesktop.Telepathy.Error.Disconnected"/>
> -        <tp:error name="org.freedesktop.Telepathy.Error.NetworkError"/>
> -        <tp:error name="org.freedesktop.Telepathy.Error.NotAvailable"/>
> -        <tp:error name="org.freedesktop.Telepathy.Error.InvalidArgument"/>
> -        <tp:error name="org.freedesktop.Telepathy.Error.PermissionDenied"/>

Removing these three is fine.
Comment 17 Simon McVittie 2013-10-15 10:27:25 UTC
Comment on attachment 87626 [details] [review]
IRC_Command: add Send() example

Review of attachment 87626 [details] [review]:
-----------------------------------------------------------------

::: extensions/Connection_Interface_IRC_Command1.xml
@@ +29,5 @@
>        </arg>
>        <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
>          <p>Send an arbitrary IRC command to the server.</p>
> +        <p>For example, an IRC client receiving <code>/bip blreset</code> from
> +           the user would call this method with <code>BIP blreset</code> as

I'd say "... might call this ...", since the /command thing is only a client convention.
Comment 18 Simon McVittie 2013-10-15 12:09:25 UTC
Comment on attachment 87601 [details] [review]
distribute Irc_Command interface in tarballs

merged, thanks
Comment 19 Simon McVittie 2013-10-15 12:09:33 UTC
Comment on attachment 87602 [details] [review]
Rename IRC_Command interface to IRCCommand1

merged, thanks
Comment 20 Guillaume Desmottes 2013-10-15 17:52:44 UTC
Created attachment 87683 [details] [review]
IRC_Command: add Send() example
Comment 21 Guillaume Desmottes 2013-10-15 17:53:03 UTC
Created attachment 87684 [details] [review]
IRC_Command: remove errors that doesn't make sense for Send()
Comment 22 Guillaume Desmottes 2013-10-15 17:53:14 UTC
Created attachment 87685 [details] [review]
IRC_Command: explain why the command is in UTF-8
Comment 23 Guillaume Desmottes 2013-10-15 17:53:27 UTC
Created attachment 87686 [details] [review]
IRC_Command: prevent user of sending commands for which we have proper API
Comment 24 Simon McVittie 2013-10-16 11:35:30 UTC
Looks good. Please merge, correct the copyright date, and import the XML into spec master.
Comment 25 Guillaume Desmottes 2013-10-16 15:49:52 UTC
Merged to Idle and master.

TODO: make a tp-glib release and use its codegen rather than extensions.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.