Bug 27865 - Port more channel implementations to GabbleBaseChannel
Summary: Port more channel implementations to GabbleBaseChannel
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Will Thompson
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/wj...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2010-04-28 02:10 UTC by Will Thompson
Modified: 2010-05-03 05:59 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

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.