[Bug 27201] implement Messages interface

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=27201





--- Comment #3 from Olivier Le Thanh Duong <olivier at lethanh.be>  2010-03-30 16:10:04 PST ---
(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


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.