Bug 29078

Summary: use TpBaseProtocol
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: hazeAssignee: Simon McVittie <smcv>
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/smcv/haze-smcv.git;a=shortlog;h=refs/heads/protocol
Whiteboard: review+
i915 platform: i915 features:
Bug Depends on: 27997, 29076, 29079    
Bug Blocks:    

Description Simon McVittie 2010-07-15 08:26:46 UTC
See branch:

http://git.collabora.co.uk/?p=user/smcv/haze-smcv.git;a=shortlog;h=refs/heads/protocol

This requires a recent version of the corresponding telepathy-glib branch from Bug #27997.
Comment 1 Will Thompson 2010-07-16 04:58:03 UTC
+  /* FIXME: is it safe to pass a NULL account to prpl_info->account for all
+   * prpls? If it is, we could do that to be more likely to normalize right */
+  return g_strdup (purple_normalize (NULL, contact));

You mean ->normalize, not ->account. And, it seems to be, except for Zephyr (which will critical). A bunch of the other normalization functions have comments suggesting that it's not accidental that they work with account == NULL. :)

So I think we should do that; or, make purple_normalize() do that. Your call.

This stuff with haze_foo_manager_append_channel_classes, all with identical implementations, suggests that that should live in tp-glib. Not clear exactly how, though...
Comment 2 Simon McVittie 2010-07-16 05:26:55 UTC
(In reply to comment #1)
> +  /* FIXME: is it safe to pass a NULL account to prpl_info->account for all
> +   * prpls? If it is, we could do that to be more likely to normalize right */
> +  return g_strdup (purple_normalize (NULL, contact));
> 
> You mean ->normalize, not ->account. And, it seems to be, except for Zephyr
> (which will critical). A bunch of the other normalization functions have
> comments suggesting that it's not accidental that they work with account ==
> NULL. :)
> 
> So I think we should do that; or, make purple_normalize() do that. Your call.

purple_normalize() can't do that, because it doesn't know which prpl we're dealing with (it'd normally get that from @account). My vote would be to add:

gchar *
purple_plugin_protocol_info_normalize (PurplePluginProtocolInfo *prpl,
    const char *contact)
{
    if (PURPLE_PROTOCOL_PLUGIN_HAS_FUNC (prpl, normalize))
    {
        return prpl->normalize (NULL, contact);
    }
    else
    {
        return purple_normalize (NULL, contact);
    }
}

and fix the Zephyr plugin in the same Pidgin release.

In the meantime, we could optionally do the same in Haze, and either assume that nobody uses Zephyr or insert some sort of hack. I think purple_normalize is good enough for now, though.

> This stuff with haze_foo_manager_append_channel_classes, all with identical
> implementations, suggests that that should live in tp-glib. Not clear exactly
> how, though...

Perhaps. The problem is that Protocol wants to enumerate all possible-to-implement channel classes statelessly, whereas Requests wants to enumerate all channel classes that will actually work, given a connected Connection, and the TpChannelManager API was designed for the latter.

More precisely, what we want is:

* HazeProtocol has a list of TpChannelManager *classes*: HazeConnection's create_channel_managers() will not create channel managers not chosen from that list

* Each TpChannelManager has a new class-method (i.e. first parameter is TpChannelManagerClass *, not TpChannelManager *) that returns a list of all possible channel classes, which is a superset of the result of foreach_channel_class()

I'll have a look at implementing that in tp-glib.
Comment 3 Simon McVittie 2010-07-16 08:23:49 UTC
(In reply to comment #2)
> More precisely, what we want is:
> 
> * HazeProtocol has a list of TpChannelManager *classes*: HazeConnection's
> create_channel_managers() will not create channel managers not chosen from that
> list
> 
> * Each TpChannelManager has a new class-method (i.e. first parameter is
> TpChannelManagerClass *, not TpChannelManager *) that returns a list of all
> possible channel classes, which is a superset of the result of
> foreach_channel_class()
> 
> I'll have a look at implementing that in tp-glib.

It turns out to be easier to use GTypes rather than classes. I did this (see Bug #27997), updated Haze to cope, and fixed the typo you noticed.
Comment 4 Will Thompson 2010-09-22 07:05:00 UTC
I looked at adding that method to libpurple, but then I noticed that none of the functions defined in prpl.h actually take a PurplePluginProtocolInfo * as the first argument: they either take a PurpleAccount or a PurpleConnection. So adding this more-useful function would be inconsistent. Sigh. So I didn't.

Ship the rest.
Comment 5 Simon McVittie 2010-09-22 09:53:25 UTC
Fixed in git for 0.5.0

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.