Bug 27865

Summary: Port more channel implementations to GabbleBaseChannel
Product: Telepathy Reporter: Will Thompson <will>
Component: gabbleAssignee: Will Thompson <will>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/wjt/telepathy-gabble-wjt.git;a=shortlog;h=refs/heads/basic
Whiteboard: review+
i915 platform: i915 features:

Description Will Thompson 2010-04-28 02:10:02 UTC 
A while back, I wrote GabbleBaseChannel to reduce the boilerplate needed per channel implementation, and based GabbleSearchChannel on it.

It has sat unused since then.

On Monday, I snapped.

 src/base-channel.c     |   53 ++++--
 src/base-channel.h     |    8 +-
 src/im-channel.c       |  396 +++++-------------------------------
 src/im-channel.h       |    8 +-
 src/im-factory.c       |    4 +-
 src/muc-channel.c      |  530 +++++++++++++++---------------------------------
 src/muc-channel.h      |    6 +-
 src/roomlist-channel.c |  320 ++++++------------------------
 src/roomlist-channel.h |    9 +-
 src/roomlist-manager.c |   15 +--
 src/search-channel.c   |    8 +-
 11 files changed, 334 insertions(+), 1023 deletions(-)
Comment 1 Simon McVittie 2010-04-28 03:52:09 UTC 
> +  presence = gabble_presence_cache_get (self->parent.conn->presence_cache,
> +      self->parent.target);

We generally avoid relying on the @parent member of instance structs like this, since it breaks if you change the inheritance hierarchy to include an extra intermediate class, which is meant to be something that's always safe in OO. I'd be inclined to use:

GabbleBaseChannel *base = (GabbleBaseChannel *) self;

presence = gabble_presence_cache_get (base->conn->presence_cache, base->target);

(Or you could call it @chan instead of @base, or something.)

> @@ -462,23 +234,23 @@ gabble_im_channel_dispose (GObject *object)
> +      self->parent.conn->roster, base->target);

Er, this mixes both modes in consecutive arguments :-)

Other than that, review+.
Comment 2 Will Thompson 2010-04-30 13:05:02 UTC 
(In reply to comment #1)
> > +  presence = gabble_presence_cache_get (self->parent.conn->presence_cache,
> > +      self->parent.target);
> 
> We generally avoid relying on the @parent member of instance structs like this,
> since it breaks if you change the inheritance hierarchy to include an extra
> intermediate class, which is meant to be something that's always safe in OO.

Normally I would kick up a fuss because I think that this is boilerplate for a pretty much theoretical gain. However, I did just add an extra intermediate class, and was inconsistent about which style I used in this branch. So... patch pushed.
Comment 3 Simon McVittie 2010-05-03 02:55:23 UTC 
ship it!
Comment 4 Will Thompson 2010-05-03 05:59:08 UTC 
commit 89f15409121bad8adfddde28ce40c7418f33a22c
Merge: 84a6dbd 28a63ce
Author: Will Thompson <will.thompson@collabora.co.uk>
Date:   Mon May 3 13:52:44 2010 +0100

    Merge branch 'basic'
    
    Closes https://bugs.freedesktop.org/show_bug.cgi?id=27865.
    
    Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

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.