Bug 56181

Created attachment 68801 [details] [review]
connection: Don't reset the capabilities in UpdateCapabilities

Created attachment 68802 [details] [review]
connection: Don't reset the capabilities in UpdateCapabilities

Get rid of gabble_caps_channel_manager_reset_capabilities and its implementation in GabbleMediaFactory too, since they are unused now.

From #telepathy on Freenode:

10:05 <sjoerd> rishi: btw remind me of your gabble bug number ?
10:06 <rishi> sjoerd: https://bugs.freedesktop.org/56181
10:07 <sjoerd> rishi: looks good, can you commit or do you not have access?
10:08 <rishi> sjoerd: I have access.
10:09 <sjoerd> cool, add a reviewed by tag and go go go :)
10:09 <rishi> sjoerd: Ok to push to master as well?
10:09 <sjoerd> yeah

By removing gabble_caps_channel_manager_reset_capabilities aren't you breaking API in the stable branch?

Also if you're should remove the reset_caps member from GabbleCapsChannelManagerIface struct.

(In reply to comment #4)
> By removing gabble_caps_channel_manager_reset_capabilities aren't you
> breaking API in the stable branch?

_reset_capabilities isn't public API. Also it doesn't remove StreamedMedia, it just changes the details of the Call1 detection (in a way that doesn't break Call..)

(In reply to comment #5)
> (In reply to comment #4)
> > By removing gabble_caps_channel_manager_reset_capabilities aren't you
> > breaking API in the stable branch?
> 
> _reset_capabilities isn't public API.

I don't understand why it's not? Its signature was removed from gabble/caps-channel-manager.h which certainly is public API. For example, the Ytstenut plugin defines a new caps channel manager implementation based on this public API.

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > By removing gabble_caps_channel_manager_reset_capabilities aren't you
> > > breaking API in the stable branch?
> > 
> > _reset_capabilities isn't public API.
> 
> I don't understand why it's not? Its signature was removed from
> gabble/caps-channel-manager.h which certainly is public API. For example,
> the Ytstenut plugin defines a new caps channel manager implementation based
> on this public API.

It's an experimental API that we don't even bother installing the headers for in distribution. So i guess it's public for some value of public for which i really can't care much (and i'm not sure we gave any abi stability guarantees about it ever given it's marked as experimental).

The API removed here is fundamentally broken though and based on me misunderstanding the spec, so better get rid of it.

(In reply to comment #7)
> The API removed here is fundamentally broken though and based on me
> misunderstanding the spec, so better get rid of it.

Another possible resolution would be to keep the API, but warn if the vfunc is actually implemented.

However, the plugin API is sufficiently flaky that the SONAME of the plugin changes every version (even in the stable branch!), which I think indicates that Sjoerd is right - we don't trust the plugin API this much.

The update needed for ytstenut-plugins will be to delete this line:

> plugin-base/channel-manager.c:  iface->reset_caps = NULL;

which I don't think is particularly onerous.

Created attachment 69827 [details] [review]
caps-channel-manager: Drop reset_caps, which is now unused

(In reply to comment #9)
> Created attachment 69827 [details] [review]
> caps-channel-manager: Drop reset_caps, which is now unused

16:01 < smcv> rishi: I'm inclined to do that on master but not on the stable 
              branch, I think

(because while we don't guarantee API even on the stable branch anyway, I happen to know that this change will break Ytstenut.)