Note that a fix was pulled from an earlier date in my branch which ... hadn't actually been tested.  My branch now has the latest version with various fixes to it.

The branch also as a single commit for Bug 26289

Sorry for taking so long to get back to you about this.

I've cherry-picked the relevant commits from your branch and created a new one for ease of review, although I might just squash them all into one:

http://git.collabora.co.uk/?p=user/jonny/telepathy-python.git;a=shortlog;h=refs/heads/26059

Here's a quick review:

Why did you implement get_handle_by_name? Why not just use RequestHandles and require that it returns Handle objects (or subclasses). The problem I foresee here is that butterfly expects all its handles to be of type ButterflyHandle. This works because it re-implements RequestHandles. What we could do is something like this:

-            ret.append(handle.get_id())
+            ret.append(handle)

in RequestHandles, and add a __int__() method to telepathy.server.Handle which returns the same as get_id(). What do you think? It sounds like a cleaner solution and nicer to CMs using tp-python (we've got to give them something!)

This solution would also mean that bug #27445 could be fixed in tp-python instead of butterfly which is cool. Thoughts?

(In reply to comment #2)
> Why did you implement get_handle_by_name?

I actually use get_handle_by_name in several parts of The One Ring.
1. When i need a single item its nicer to get it than handle the list case
2. I imagine CMs would not normally specialize the boilerplate in RequestHandles but just what is exactly in get_handle_by_name
3. Maybe its a lack of knowledge in how the dbus bindings work but I feel weird internally calling into the external API
4. I'm not too sure what all cases handles should be marked as client handles but its nicer to have a call under the hood for when a handle isn't a client handle

> Why not just use RequestHandles and
> require that it returns Handle objects (or subclasses). The problem I foresee
> here is that butterfly expects all its handles to be of type ButterflyHandle.
> This works because it re-implements RequestHandles. What we could do is
> something like this:
> -            ret.append(handle.get_id())
> +            ret.append(handle)
> 
> in RequestHandles, and add a __int__() method to telepathy.server.Handle which
> returns the same as get_id(). What do you think? It sounds like a cleaner
> solution and nicer to CMs using tp-python (we've got to give them something!)
> 
> This solution would also mean that bug #27445 could be fixed in tp-python
> instead of butterfly which is cool. Thoughts?

I think this change makes sense independent of whether get_handle_by_name is used or not.

(In reply to comment #3)
> I actually use get_handle_by_name in several parts of The One Ring.
> 1. When i need a single item its nicer to get it than handle the list case

Right, butterfly has a handle factory it uses throughout.

> 2. I imagine CMs would not normally specialize the boilerplate in
> RequestHandles but just what is exactly in get_handle_by_name
> 3. Maybe its a lack of knowledge in how the dbus bindings work but I feel weird
> internally calling into the external API

By external API do you mean methods like RequestHandles? Overriding such methods is completely normal.

My point is that the CM-specific work has already been done in redefining RequestHandles -- butterfly does it[0] and sunshine does it[1]. It would be nice to not have to have further CM-specific handle functions and re-use the ones we already have, no?

0. http://git.collabora.co.uk/?p=telepathy-butterfly.git;a=blob;f=butterfly/connection.py;hb=HEAD#l251
1. http://git.collabora.co.uk/?p=telepathy-sunshine.git;a=blob;f=sunshine/connection.py;hb=HEAD#l370

(In reply to comment #4)
> Right, butterfly has a handle factory it uses throughout.

For sunshine, butterfly, the one ring, and my pre-alpha bluewire the handle factory does one thing and exactly one thing, create a handle using handletype specific parameters.

In my view get_handle_by_name translates handle_type/name to a handle object.  I think that translation logic from name to handle init parameters belongs in get_handle_by_name rather than the handle factory.  I used to (no need anymore) and butterfly currently does translation between name and handle init parameters.

Having the logic decoupled is nice for when your protocol initiates the creation of a handle.  You already have the disparate parameters.  Its annoying to construct them into a name just to have the handle factory decompose them again into distinct parameters.
 
> By external API do you mean methods like RequestHandles? Overriding such
> methods is completely normal.

Hmm, my reference in my comment to "external API" was not about overriding but calling them from within my own CM.  My problems are
1. A personal lack of understanding of dbus bindings and any magic it might do to the function
2. proper layering
3. keeping the code more decomposed so I don't call these monoliths that do more than I want.

> My point is that the CM-specific work has already been done in redefining
> RequestHandles -- butterfly does it[0] and sunshine does it[1]. It would be
> nice to not have to have further CM-specific handle functions and re-use the
> ones we already have, no?
> 
> 0.
> http://git.collabora.co.uk/?p=telepathy-butterfly.git;a=blob;f=butterfly/connection.py;hb=HEAD#l251
> 1.
> http://git.collabora.co.uk/?p=telepathy-sunshine.git;a=blob;f=sunshine/connection.py;hb=HEAD#l370

Compared to my branch of tp-python, what parts are butterfly and sunshine overriding?  Not the whole function.  Its the part of the code in the for-loop and that is the code I propose be moved into get_handle_by_name.  I would not want get_handle_by_name to be added just for me but I would expect other CMs to enjoy my very abstract benefits from it ;)

I noticed sunshine has get_handle_id_by_name.  The two main differences are
1. It returns an id rather than an object
2. It only searches rather than doing a search and a construction.

I can understand wanting (2) and could see this being decomposed in a different way.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-python/issues/10.