Bug 27014

TP-Python :
 * requests: 0 is a valid TargetHandleType
Ok but we should probably check against <= LAST_HANDLE_TYPE for the uper limit
 * channel: TargetHandle=0 if the channel handle is None
It would probably better to have a NoneHandle object whose get_id() return 0 instead of using None when there is no Handle, this would avoid having special cases like this everywhere.
  * channel: don't find the channel handle if TargetHandleT... 
Only one IF would work as well
  * channel: add a comment on get_props to clarify more...
Nitpicking but proper python docstring would be nicer
  * channelmanager: rename requestable_channel_classes... 
What's the point ?
  * channelmanager: add implement_channel_classes function
Please document the new function, especially the arguments
Maybe display a deprecation warning

(In reply to comment #1)
> TP-Python :

I won't amend any previous commits to make it easier to review.

>  * requests: 0 is a valid TargetHandleType
> Ok but we should probably check against <= LAST_HANDLE_TYPE for the uper limit

Done.

>  * channel: TargetHandle=0 if the channel handle is None
> It would probably better to have a NoneHandle object whose get_id() return 0
> instead of using None when there is no Handle, this would avoid having special
> cases like this everywhere.

Done.

>   * channel: don't find the channel handle if TargetHandleT... 
> Only one IF would work as well

Done.

>   * channel: add a comment on get_props to clarify more...
> Nitpicking but proper python docstring would be nicer

Done.

>   * channelmanager: rename requestable_channel_classes... 
> What's the point ?

Because it is now called what it does, as opposed to something else. It doesn't contain any channel classes.

>   * channelmanager: add implement_channel_classes function
> Please document the new function, especially the arguments

Done.

> Maybe display a deprecation warning

Hm, how can I do this but be sure that dbus-python won't pick it up and use it as a dbus method error?

My branch has been updated with the above changes.

If you use warnings.warn("bla", DeprecationWarning) it won't pose any problem with python-dbus.

Could you also please put a note in the NEWS file about the use of your new function? Thanks

This only a preliminary review, I didn't test it, because I would rather want to wait you divide the two use case in two separate classes. The TextChannel class as it is in this patch is barely understandable and maintainable.

TP-Butterfly:
  * Need to be adapted to use NoneHandle
  * Please check version against (0,15,16,1) that's the actual dev version numbers
  * ButterflyChannelManager: shouldn't that actually be different classes with each of the Initial* as a mandatory parameter?
  * AddMembers is implemented but the group flag is 0, which don't allow the client to call it

(In reply to comment #3)
> If you use warnings.warn("bla", DeprecationWarning) it won't pose any problem
> with python-dbus.

Done.

> Could you also please put a note in the NEWS file about the use of your new
> function? Thanks

Done.

(In reply to comment #4)
> This only a preliminary review, I didn't test it, because I would rather want
> to wait you divide the two use case in two separate classes. The TextChannel
> class as it is in this patch is barely understandable and maintainable.

jaksdhkjhadfjkadshakjsdha

I'll do this later. For now:

>   * Need to be adapted to use NoneHandle

Done. 

>   * Please check version against (0,15,16,1) that's the actual dev version
> numbers

Telepathy projects have a policy of not depending on unreleased versions of libraries. I don't expect to merge my conference branch to butterfly until a new tp-python release is cut. So, no.

>   * ButterflyChannelManager: shouldn't that actually be different classes with
> each of the Initial* as a mandatory parameter?

Please say "channel class" when you mean that. I got so confused because I thought you were talking about python classes. Okay, yeah I've fixed that now.

>   * AddMembers is implemented but the group flag is 0, which don't allow the
> client to call it

Good catch. Fixed.

(In reply to comment #4)
> This only a preliminary review, I didn't test it, because I would rather want
> to wait you divide the two use case in two separate classes. The TextChannel
> class as it is in this patch is barely understandable and maintainable.

I've done this. From the commit message:

    text channel: split into three subclasses: Im, Muc and Conference

    ButterflyTextChannel was getting a little heavy. The class hierarchy
    now goes like this:

     telepathy.server.Channel
      +----telepathy.server.ChannelTypeText
            +----ButterflyTextChannel
                  +----ButterflyImChannel
                  +----ButterflyMucChannel
                        +----ButterflyConferenceChannel

    Simple.

Enjoy reviewing!

Merged! Thanks for your reviews.

*** Bug 25757 has been marked as a duplicate of this bug. ***

*** Bug 22457 has been marked as a duplicate of this bug. ***