Bug 27997

Summary: TpBaseProtocol and TpProtocol in tp-glib
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-glibAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium Keywords: patch
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/protocol3
Whiteboard: review+ when spec supports it
i915 platform: i915 features:
Bug Depends on: 20774    
Bug Blocks: 27998, 28819, 29078, 29079    

Description Simon McVittie 2010-05-06 08:59:17 UTC
See Bug #20774. This is the telepathy-glib API for services.
Comment 1 Simon McVittie 2010-05-06 09:03:24 UTC
There are actually three branches here.

basic-protocol-objects adds a TpBaseProtocol base class. It doesn't depend on new codegen, so if we're confident that this code is "the right shape", we could merge it at any time.

protocol-export and protocol-export-2 add the Protocol codegen (which includes a stub TpProtocol class) and turn TpBaseProtocol into a full Protocol object.

See also Bug #27998, which adds the rest of TpProtocol.
Comment 2 Andre Moreira Magalhaes 2010-06-28 09:02:15 UTC
Review+ from ae8a03 to 64eeb7
Comment 3 Simon McVittie 2010-06-28 10:11:48 UTC
(In reply to comment #2)
> Review+ from ae8a03 to 64eeb7

Thanks. (For the record: this covers all of basic-protocol-objects, protocol-export and protocol-export-2.)

Any thoughts on the spec?
Comment 4 Simon McVittie 2010-07-01 03:28:11 UTC
*** Bug 27998 has been marked as a duplicate of this bug. ***
Comment 5 Simon McVittie 2010-07-01 03:37:21 UTC
Over on Bug #27998, Andre wrote:
> Rebased with upstream master and fixed some issues with tests+conflicts in docs
> mainly.
>
> The branch is at
> http://git.collabora.co.uk/?p=user/andrunko/telepathy-glib.git;a=shortlog;h=refs/heads/protocol

I've got rid of my branches in favour of this one, but in future I'd much prefer it if you didn't rebase other people's branches - it makes it difficult to work out what has changed, and introduces a lot of commits which would never actually have worked or even compiled.

A non-trivial merge of master into the branch, or the branch into a copy of master, with appropriate changes to make it compile and work again, would have preserved the history, and preserved the general principle that after each commit, the code should be (at least mostly) usable.

> Just one thing I missed in the review. The methods on Protocol
> (IdentifyAccount, NormalizeContact) are not implemented in client code, should
> they be? I didn't implement them in tp-qt4 either.

Do you mean that generated tp_cli functions are missing, or that high-level wrappers are missing?

As long as we have the generated tp_cli functions, I think we can defer high-level wrappers until a bit later, although we should open a bug for them so they don't get lost.

> I am using this rebased branch as requirement for tp-qt4 tests for Protocol
> objects, as I need the changed echo2 and the addition of protocol in
> ConnectionManager, ...
Comment 6 Simon McVittie 2010-07-01 03:57:42 UTC
I've added a commit to fix the documentation, which wasn't merged quite right.
Comment 7 Andre Moreira Magalhaes 2010-07-01 07:38:40 UTC
(In reply to comment #5)
> Over on Bug #27998, Andre wrote:
> > Rebased with upstream master and fixed some issues with tests+conflicts in docs
> > mainly.
> >
> > The branch is at
> > http://git.collabora.co.uk/?p=user/andrunko/telepathy-glib.git;a=shortlog;h=refs/heads/protocol
> 
> I've got rid of my branches in favour of this one, but in future I'd much
> prefer it if you didn't rebase other people's branches - it makes it difficult
> to work out what has changed, and introduces a lot of commits which would never
> actually have worked or even compiled.
> 
> A non-trivial merge of master into the branch, or the branch into a copy of
> master, with appropriate changes to make it compile and work again, would have
> preserved the history, and preserved the general principle that after each
> commit, the code should be (at least mostly) usable.
Sure, I will keep that in mind next time. I just wanted to have something working so I could work on tp-qt4 tests for Protocol easily.

> > Just one thing I missed in the review. The methods on Protocol
> > (IdentifyAccount, NormalizeContact) are not implemented in client code, should
> > they be? I didn't implement them in tp-qt4 either.
> 
> Do you mean that generated tp_cli functions are missing, or that high-level
> wrappers are missing?
> 
> As long as we have the generated tp_cli functions, I think we can defer
> high-level wrappers until a bit later, although we should open a bug for them
> so they don't get lost.
The high-level wrappers are missing, low-level methods are there. So I keep r+ here and we should open bugs to implement the missing high-level methods later. Same for tp-qt4 impl found at andrunko/cm-protocol that does not wrap these methods.
Comment 8 Simon McVittie 2010-07-15 08:30:09 UTC
The branch smcv/protocol2 makes some changes I found were necessary while porting Haze to use TpBaseProtocol (Bug #29078).
Comment 9 Andre Moreira Magalhaes 2010-07-15 17:13:25 UTC
Looks good.
Comment 10 Simon McVittie 2010-07-16 08:21:52 UTC
... and again, making some changes suggested by wjt on Bug #29078.
Comment 11 Simon McVittie 2010-07-20 06:12:01 UTC
Reviewed by andrunko and ptlo up to 4d74feb906b7f9e37238a9d89245eeaaf1a0ffed.
Comment 12 Simon McVittie 2010-07-20 10:12:36 UTC
Fixed in git for 0.11.11. Thanks, everyone!

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.