Bug 27201

http://git.collabora.co.uk/?p=user/olethanh/telepathy-butterfly.git;a=shortlog;h=refs/heads/messages_iface
Branch that implement it

I had a quick look over the code. Here are some comments to get you started:

-    def Send(self, message_type, text):
+    def _send_text_message(self, message_type, text):

I think it might have been me, but I was far too keen to use an underscore to prefix method names, even though they are being called from other classes. butterfly is not a library, so having do_this and do_that methods (sans underscore) really doesn't matter. Louis-Francis' webcam work did this nicer, for example.

I want to have a good look over butterfly (and perhaps tp-python too, but later) and fix this all over the place, but let's not introduce it more.

Does that make sense? (I'm basically asking for s/_send_text_message/send_text_message/g)

+        self._pending_messages2 = {}

Can this have a more useful name for this please? :-)

+        "Send a simple text message, return true if send correctly"""

Mismatched quotes and it's past tense, so s/send correctly/sent correctly/.

+    def _signal_text_sent(self, timestamp, message_type, text):
+            headers = {'message-sent' : timestamp,

Indented once too.

+    def GetPendingMessageContent(self, Message_ID, Parts):

I realise you copied these from the spec, but argument names Message_ID and Parts are not very pythonic. Can we rename to message_id and parts instead?

+    @dbus.service.signal('org.freedesktop.Telepathy.Channel.Interface.Messages', signature='aa{sv}')
+    def MessageReceived(self, Message):

Why did you add the decorator?

I need to read the Messages interface again and take another look at your branch.

(In reply to comment #2)
> I had a quick look over the code. Here are some comments to get you started:
> 
> -    def Send(self, message_type, text):
> +    def _send_text_message(self, message_type, text):
> 
> I think it might have been me, but I was far too keen to use an underscore to
> prefix method names, even though they are being called from other classes.
> butterfly is not a library, so having do_this and do_that methods (sans
> underscore) really doesn't matter. Louis-Francis' webcam work did this nicer,
> for example.
> 
> I want to have a good look over butterfly (and perhaps tp-python too, but
> later) and fix this all over the place, but let's not introduce it more.
> 
> Does that make sense? (I'm basically asking for
> s/_send_text_message/send_text_message/g)
> 
I fell add the _ only when the method is called by the class and subclasses but if you fell strongly about it why not.

> +        self._pending_messages2 = {}
> 
> Can this have a more useful name for this please? :-)
Well the spec call it PendingMessage but self._pending_messages is already defined by ChannelTypeText in tp-python and I couldn't find any better names, any suggestion?

> 
> +        "Send a simple text message, return true if send correctly"""
> 
> Mismatched quotes and it's past tense, so s/send correctly/sent correctly/.
Corrected

> 
> +    def _signal_text_sent(self, timestamp, message_type, text):
> +            headers = {'message-sent' : timestamp,
> 
> Indented once too.
Corrected

> 
> +    def GetPendingMessageContent(self, Message_ID, Parts):
> 
> I realise you copied these from the spec, but argument names Message_ID and
> Parts are not very pythonic. Can we rename to message_id and parts instead?
> 
Corrected, and all the other methods which had non-lower case only arguments
> +   
> @dbus.service.signal('org.freedesktop.Telepathy.Channel.Interface.Messages',
> signature='aa{sv}')
> +    def MessageReceived(self, Message):
> 
> Why did you add the decorator?
> 
All the signals redefined in tp-python re-add the decorator, didn't actually test if that made a difference or not.

> I need to read the Messages interface again and take another look at your
> branch.
> 

Ok, I made another commit on top

(In reply to comment #3)
> (In reply to comment #2)

> > +   
> > @dbus.service.signal('org.freedesktop.Telepathy.Channel.Interface.Messages',
> > signature='aa{sv}')
> > +    def MessageReceived(self, Message):
> > 
> > Why did you add the decorator?
> > 
> All the signals redefined in tp-python re-add the decorator, didn't actually
> test if that made a difference or not.
just tested, it doesn't emit it if we don't add the decorator

(In reply to comment #4)
> just tested, it doesn't emit it if we don't add the decorator

Odd. Well use the interface constant instead of "org.freedes..."

+    # run in an glib's idle so we emit the signal after either Send or SendMessage return
+    @async
+    def _signal_text_sent(self, timestamp, message_type, text):

dbus-python has a better way of doing this instead of throwing things in an idle. See CreateChannel in tp-python's ConnectionInterfaceRequests.

Other than that yeah let's go. Ping me when you've updated your branch.

(In reply to comment #5)
> (In reply to comment #4)
> > just tested, it doesn't emit it if we don't add the decorator
> 
> Odd. Well use the interface constant instead of "org.freedes..."
> 
Done
> +    # run in an glib's idle so we emit the signal after either Send or
> SendMessage return
> +    @async
> +    def _signal_text_sent(self, timestamp, message_type, text):
> 
> dbus-python has a better way of doing this instead of throwing things in an
> idle. See CreateChannel in tp-python's ConnectionInterfaceRequests.
> 
> Other than that yeah let's go. Ping me when you've updated your branch.

I know about it but I actually found it far less readable than the simple async decorator. However if you feel strongly about it the top commit of the branche does that

Okay, fine, get merging.

Merged thanks