Bug 70434

Created attachment 87568 [details] [review]
add IrcCommand extension

I would rename the interface to IrcCommand1 to be next-ready. Other than that, +1.

Renamed and merged, thanks.

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?

Reopening. I don't consider this to have been fixed until it's in telepathy-spec, I'm afraid.

Created attachment 87601 [details] [review]
distribute Irc_Command interface in tarballs

Not doing so breaks distcheck.

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">

(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 on attachment 87601 [details] [review]
distribute Irc_Command interface in tarballs

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

ooops

++

Comment on attachment 87602 [details] [review]
Rename IRC_Command interface to IRCCommand1

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

++

> 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.

Created attachment 87625 [details] [review]
IRC_Command: remove possible errors

Created attachment 87626 [details] [review]
IRC_Command: add Send() example

(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).

(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 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 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 on attachment 87601 [details] [review]
distribute Irc_Command interface in tarballs

merged, thanks

Comment on attachment 87602 [details] [review]
Rename IRC_Command interface to IRCCommand1

merged, thanks

Created attachment 87683 [details] [review]
IRC_Command: add Send() example

Created attachment 87684 [details] [review]
IRC_Command: remove errors that doesn't make sense for Send()

Created attachment 87685 [details] [review]
IRC_Command: explain why the command is in UTF-8

Created attachment 87686 [details] [review]
IRC_Command: prevent user of sending commands for which we have proper API

Looks good. Please merge, correct the copyright date, and import the XML into spec master.

Merged to Idle and master.

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