My tp-python branch: http://git.collabora.co.uk/?p=user/jonny/telepathy-python.git;a=shortlog;h=refs/heads/misc-for-conference My butterfly branch: http://git.collabora.co.uk/?p=user/jonny/telepathy-butterfly.git;a=shortlog;h=refs/heads/conference
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. ***
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.