Bug 50834 - tp_account_get_connection_manager() and _get_protocol() naming is wrong
Summary: tp_account_get_connection_manager() and _get_protocol() naming is wrong
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/xc...
Whiteboard:
Keywords: patch
Depends on:
Blocks: tp-glib-1.0
  Show dependency treegraph
 
Reported: 2012-06-07 07:04 UTC by Xavier Claessens
Modified: 2012-07-02 05:19 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Xavier Claessens 2012-06-07 07:04:22 UTC
Those APIs return the *name* of the CM/Protocol, not the TpConnectionManager/TpProtocol objects.

TpConnection has _get_connection_manager_name() and _get_protocol_name() for the exact same thing.

While doing this, _get_cm_name() is shorter, and is used in tp_protocol_get_cm_name().
Comment 1 Jonny Lamb 2012-06-07 07:08:51 UTC
I say we have two options:

1. Let's use _get_protocol_name() and _get_cm_name() everywhere. I guess means updating TpConnection and obviously TpAccount.

2. Let's actually make these functions return the TpConnectionManager/TpProtocol object.

Option two makes the API nicer in my opinion, but is arguably a bit more work.
Comment 2 Xavier Claessens 2012-06-08 01:48:00 UTC
Here is branch for master: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=cm-name

It deprecate wrong names and add the correct names. Deprecated APIs will then be removed in next.
Comment 3 Simon McVittie 2012-06-08 03:24:17 UTC
(In reply to comment #2)
> Here is branch for master:
[...
> It deprecate wrong names and add the correct names.

That approach is perfect, IMO.

The branch looks good, except for this:

> Note that this introduce a small behaviour change: "connection-manager" and
> "protocol" properties are not CONSTRUCT_ONLY anymore. This is because we can't
> have 2 CONSTRUCT properties setting the same value, otherwise the default-value
> would override the value set on the other property. This shouldn't affect any
> application since that API was added in previous release, and
> tp_account_request_new() should be used anyway.

If you set the property twice (which is now "officially" allowed - the metadata says you can set it at any time, now!), the set_property() implementation asserts. That seems bad.

I would be inclined to omit this patch from master altogether, and just do a straight rename in next. As you say, applications will probably use new() anyway, so this shouldn't result in much/any actual porting between 0.x and 1.0.

> Deprecated APIs will then
> be removed in next.

Ideal.
Comment 4 Simon McVittie 2012-06-08 03:30:49 UTC
(In reply to comment #1)
> 2. Let's actually make these functions return the
> TpConnectionManager/TpProtocol object.

I think that should be a separate bug, tbh.
Comment 5 Xavier Claessens 2012-06-09 01:26:19 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Note that this introduce a small behaviour change: "connection-manager" and
> > "protocol" properties are not CONSTRUCT_ONLY anymore. This is because we can't
> > have 2 CONSTRUCT properties setting the same value, otherwise the default-value
> > would override the value set on the other property. This shouldn't affect any
> > application since that API was added in previous release, and
> > tp_account_request_new() should be used anyway.
> 
> If you set the property twice (which is now "officially" allowed - the metadata
> says you can set it at any time, now!), the set_property() implementation
> asserts. That seems bad.

I don't understand what you mean. "connection-manager" is now read-only and "cm-name" is construct-only, so you can set it only once. The only issue is if an app does g_object_new (TP_TYPE_ACCOUNT_REQUEST, "connection-manager", foo, NULL); it will now fail. But only empathy uses it and any app is using tp_account_request_new() anyway. So really the issue is purely theoric.
Comment 6 Simon McVittie 2012-06-29 03:34:52 UTC
(In reply to comment #5)
> > If you set the property twice (which is now "officially" allowed

Ah, what I'd missed was that this changed from READWRITE to READABLE, so not only are you not allowed to set it twice, you're not even allowed to set it once.

> The only issue is if
> an app does g_object_new (TP_TYPE_ACCOUNT_REQUEST, "connection-manager", foo,
> NULL); it will now fail.

That's an API break.

I would (still) be inclined to omit this patch from master altogether, and just do a straight rename in next.
Comment 7 Xavier Claessens 2012-07-02 05:19:44 UTC
ok, did like that. Branch pushed to both master and next.


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.