|
Description
Simon McVittie
2013-09-16 16:33:18 UTC
Repurposing this bug for "do all the remaining changes that are clearly going to break the world". Done: spec patches. To do: telepathy-glib (now including the Logger), all CMs, MC, Empathy. Created attachment 88629 [details] [review] [01/12] Change all namespaces from im.telepathy1 to im.telepathy.v1 We don't own telepathy1.im, but we do own telepathy.im. Created attachment 88630 [details] [02/12] Flatten Requests interface into Connection --- Fixes Bug #50093. Commit message needs amending to say so. Created attachment 88631 [details] [review] [03/12] NewChannel: be singular We no longer recommend emitting plural NewChannels signals, so we might as well simplify the signal. Created attachment 88632 [details] [review] [04/12] ObserveChannel: be singular Created attachment 88633 [details] [review] [05/12] HandleChannel: be singular Created attachment 88634 [details] [review] [06/12] ChannelDispatchOperation, Approver: be singular This means we can use immutable properties for the Channel. Created attachment 88635 [details] [review] [07/12] HandleWithTime: fold into HandleWith Created attachment 88636 [details] [review] [08/12] Handler.HandleWith::User_Action_Time: use a signed type User_Action_Timestamp is signed, so this should be signed too. Created attachment 88637 [details] [review] [09/12] Abolish Channel.I.DTMF Distribute its remaining non-obsolete functionality between Content.I.DTMF and Channel.T.Call. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=46443 Created attachment 88638 [details] [review] [10/12] Flatten Connection.I.Contacts into Connection --- Bug #50093 again Created attachment 88639 [details] [review] [11/12] A.I.Hidden, AM.I.Hidden: remove They were added as drafts in 2011 and never undrafted. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=33101 Created attachment 88640 [details] [review] [12/12] Give each draft interface a reference to its bug ... or at least *a* relevant bug. --- Still to be done in the spec before 1.0: do a sweep through these interfaces and either undraft or delete, erring on the side of delete. We can bring them back later if they turn out to be necessary. (In reply to comment #3) > Created attachment 88630 [details] > [02/12] Flatten Requests interface into Connection > > --- > > Fixes Bug #50093. Commit message needs amending to say so. This might actually not be such a great idea. EnsureChannel and CreateChannel should clearly be core, and RequestableChannelClasses (aka TP_CONNECTION_FEATURE_CAPABILITIES) can reasonably be core while connected, but Channels (and its change-notification, NewChannel(s) and ChannelClosed) are sufficiently special-purpose that I think only the AM and regression tests should be using it. Having the Channels and their immutable properties in the GetAll result seems non-optimal. I'm tempted to revise this plan to: * move EnsureChannel, CreateChannel, RequestableChannelClasses to Connection * rename the rest of Requests to ChannelList * keep ChannelList mandatory (In reply to comment #2) > Created attachment 88629 [details] [review] [review] > [01/12] Change all namespaces from im.telepathy1 to im.telepathy.v1 > > We don't own telepathy1.im, but we do own telepathy.im. Moving this to Bug #71262. Comment on attachment 88630 [details] [02/12] Flatten Requests interface into Connection Obsoleted by Bug #71262 Comment on attachment 88638 [details] [review] [10/12] Flatten Connection.I.Contacts into Connection Obsoleted by Bug #50093 (In reply to comment #16) > Comment on attachment 88630 [details] > [02/12] Flatten Requests interface into Connection > > Obsoleted by Bug #71262 ... er, Bug #50093 (In reply to comment #2) > [01/12] Change all namespaces from im.telepathy1 to im.telepathy.v1 > > We don't own telepathy1.im, but we do own telepathy.im. Obsoleted by Bug #71262. The rest of the patches on this bug are going to need rebasing. Comment on attachment 88631 [details] [review] [03/12] NewChannel: be singular Review of attachment 88631 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88632 [details] [review] [04/12] ObserveChannel: be singular Review of attachment 88632 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88633 [details] [review] [05/12] HandleChannel: be singular Review of attachment 88633 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88634 [details] [review] [06/12] ChannelDispatchOperation, Approver: be singular Review of attachment 88634 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88635 [details] [review] [07/12] HandleWithTime: fold into HandleWith Review of attachment 88635 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88636 [details] [review] [08/12] Handler.HandleWith::User_Action_Time: use a signed type Review of attachment 88636 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88637 [details] [review] [09/12] Abolish Channel.I.DTMF Review of attachment 88637 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88639 [details] [review] [11/12] A.I.Hidden, AM.I.Hidden: remove Review of attachment 88639 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88640 [details] [review] [12/12] Give each draft interface a reference to its bug Review of attachment 88640 [details] [review]: ----------------------------------------------------------------- ++ good idea TODO: delete Chan.I.Addressing draft as per Bug #42918. This is all going to need rebasing, unfortunately.
Someone should do the whole spec -> tp-glib -> {CMs, MC} dance and a round of releases/snapshots (0.99.7?) to land all this in one go, I think - otherwise it'll take forever to get everything back into sync.
(In reply to comment #30) > This is all going to need rebasing, unfortunately. Merged those already (not conflicting): de3eb33 Give each draft interface a reference to its bug 023eb15 A.I.Hidden, AM.I.Hidden: remove 01dd498 Abolish Channel.I.DTMF c5295fd Handler.HandleWith::User_Action_Time: use a signed type 0589cc4 HandleWithTime: fold into HandleWith 431a64e ChannelDispatchOperation, Approver: be singular 3510bdc HandleChannel: be singular (In reply to comment #30) > This is all going to need rebasing, unfortunately. All the spec patches have been merged to next. (In reply to comment #18) > (In reply to comment #16) > > Comment on attachment 88630 [details] > > [02/12] Flatten Requests interface into Connection > > > > Obsoleted by Bug #71262 > > ... er, Bug #50093 Sorry, please revert this one (I've put a suggested patch in smcv/next). I decided against it: (In reply to comment #14) > This might actually not be such a great idea. EnsureChannel and > CreateChannel should clearly be core, and RequestableChannelClasses (aka > TP_CONNECTION_FEATURE_CAPABILITIES) can reasonably be core while connected, > but Channels (and its change-notification, NewChannel(s) and ChannelClosed) > are sufficiently special-purpose that I think only the AM and regression > tests should be using it. Having the Channels and their immutable properties > in the GetAll result seems non-optimal. > > I'm tempted to revise this plan to: > > * move EnsureChannel, CreateChannel, RequestableChannelClasses to Connection > > * rename the rest of Requests to ChannelList > > * keep ChannelList mandatory Your call whether to move EnsureChannel, CreateChannel to Connection. On balance it's probably not a good idea. (Rationale: we only expect MC to use them.) Moving RequestableChannelClasses is probably still a good idea; I haven't done it. (Rationale: we expect non-MC things to use it.) Renaming Requests to ChannelList is entirely cosmetic, and would even be misleading if we leave EnsureChannel, CreateChannel on that interface, so let's not. (In reply to comment #33) > (In reply to comment #18) > > (In reply to comment #16) > > > Comment on attachment 88630 [details] > > > [02/12] Flatten Requests interface into Connection > > > > > > Obsoleted by Bug #71262 > > > > ... er, Bug #50093 > > Sorry, please revert this one (I've put a suggested patch in smcv/next). I > decided against it: Did you forget to push the branch? But yeah go ahead and merge it. > (In reply to comment #14) > > This might actually not be such a great idea. EnsureChannel and > > CreateChannel should clearly be core, and RequestableChannelClasses (aka > > TP_CONNECTION_FEATURE_CAPABILITIES) can reasonably be core while connected, > > but Channels (and its change-notification, NewChannel(s) and ChannelClosed) > > are sufficiently special-purpose that I think only the AM and regression > > tests should be using it. Having the Channels and their immutable properties > > in the GetAll result seems non-optimal. > > > > I'm tempted to revise this plan to: > > > > * move EnsureChannel, CreateChannel, RequestableChannelClasses to Connection > > > > * rename the rest of Requests to ChannelList > > > > * keep ChannelList mandatory > > Your call whether to move EnsureChannel, CreateChannel to Connection. On > balance it's probably not a good idea. (Rationale: we only expect MC to use > them.) Agreed; if anything "should clients use it or just MC" is a good way to split ifaces I think. > Moving RequestableChannelClasses is probably still a good idea; I haven't > done it. (Rationale: we expect non-MC things to use it.) Fair enough. > Renaming Requests to ChannelList is entirely cosmetic, and would even be > misleading if we leave EnsureChannel, CreateChannel on that interface, so > let's not. I don't really care but I think we have enough in our plate atm to bikeshed on cosemetic only changes. :) Created attachment 92141 [details] [review] move RequestableChannelClasses to Connection Comment on attachment 92141 [details] [review] move RequestableChannelClasses to Connection Review of attachment 92141 [details] [review]: ----------------------------------------------------------------- Looks good, except: ::: doc/templates/interface.html @@ +380,4 @@ > and <a href="Channel_Dispatcher.html">ChannelDispatcher</a>. > If supported on this protocol, the property should appear in either the > Fixed_Properties or Allowed_Properties of > + a <a href="Connection_Interface_Requests.html#im.telepathy.v1.Connection.RequestableChannelClasses">RequestableChannelClass</a> should also be Connection.html#... now (and maybe grep for other uses of that filename) @@ +394,4 @@ > and <a href="Channel_Dispatcher.html">ChannelDispatcher</a>. > The property should also appear in either the Fixed_Properties > or Allowed_Properties of > + a <a href="Connection_Interface_Requests.html#im.telepathy.v1.Connection.RequestableChannelClasses">RequestableChannelClass</a> likewise (In reply to comment #36) > Comment on attachment 92141 [details] [review] [review] > move RequestableChannelClasses to Connection > > Review of attachment 92141 [details] [review] [review]: > ----------------------------------------------------------------- > > Looks good, except: > > ::: doc/templates/interface.html > @@ +380,4 @@ > > and <a href="Channel_Dispatcher.html">ChannelDispatcher</a>. > > If supported on this protocol, the property should appear in either the > > Fixed_Properties or Allowed_Properties of > > + a <a href="Connection_Interface_Requests.html#im.telepathy.v1.Connection.RequestableChannelClasses">RequestableChannelClass</a> > > should also be Connection.html#... now > > (and maybe grep for other uses of that filename) That was the only ones. Fixed and merged; thanks. Here is the tp-glib branch: http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-spec-sync I updated the spec patch by patch to make things easier for me and the reviewer. I didn't change the TpChannelManager::new-channels: API yet. How would you re-design it? I updated most CM. Only tests need to be updated: http://cgit.collabora.com/git/user/cassidy/telepathy-gabble/log/?h=next-spec-sync http://cgit.collabora.com/git/user/cassidy/telepathy-salut/log/?h=next-new-spec http://cgit.collabora.com/git/user/cassidy/telepathy-idle/log/?h=next-spec-sync http://cgit.collabora.com/git/user/cassidy/telepathy-haze/log/?h=next-spec-sync I'll do MC now. Here we go: http://cgit.collabora.com/git/user/cassidy/telepathy-mission-control/log/?h=next-spec-sync Updating all the tests was good fun! MC looks good, also please delete the functions that deal with channel details ((o, a{sv}) pair) if they're no longer called. (git grep details)
Gabble looks good Salut, Idle look good Haze looks good > - tp_tests_add_channel_to_ptr_array Can that function be removed entirely? + g_object_notify ((GObject *) self, "channel"); + + if (g_hash_table_lookup (self->priv->immutable_properties, + TP_PROP_CHANNEL_DISPATCH_OPERATION_CHANNEL) == NULL) + { + g_hash_table_insert (self->priv->immutable_properties, + g_strdup (TP_PROP_CHANNEL_DISPATCH_OPERATION_CHANNEL), + tp_g_value_slice_new_boxed (DBUS_TYPE_G_OBJECT_PATH, path)); + } + + if (g_hash_table_lookup (self->priv->immutable_properties, + TP_PROP_CHANNEL_DISPATCH_OPERATION_CHANNEL_PROPERTIES) == NULL) + { + g_hash_table_insert (self->priv->immutable_properties, + g_strdup (TP_PROP_CHANNEL_DISPATCH_OPERATION_CHANNEL_PROPERTIES), + tp_g_value_slice_new_boxed ( + TP_HASH_TYPE_STRING_VARIANT_MAP, properties)); + } +} Shouldn't we be updating internal state *before* emitting signals? > Handler.HandleWith::User_Action_Time: use a signed type I was surprised how small this patch is, but the high-level API is signed already, so it seems correct. > RequestableChannelClasses has been moved to Connection I think you still need to update the client API to match? (In reply to comment #38) > I didn't change the TpChannelManager::new-channels: API yet. How would you > re-design it? As I'm sure you already know: The high-level API should be tp_channel_manager_emit_new_channel(), possibly replacing the GSList with a different data structure. tp_channel_manager_emit_new_channels() should just go away, if it hasn't already. ::new-channels should be renamed to ::new-channel and have signature: new-channel (TpChannelManager *self, TpExportableChannel *channel, something<something> requests) The thing I'm not sure about, and probably the thing you're not sure about, is the type of the requests. We should do something that is introspectable, if that isn't hideously painful. #gnome-hackers suggests that GList<TpChannelManagerRequest *>, where TpChannelManagerRequest is opaque, would be the best thing to have in a signal - apparently GList<boxed> and GList<GObject> have regression test coverage, which is a good way to make sure they still work. How about this? * Turn TpBaseConnection's ChannelRequest into TpChannelManagerRequest * Make it refcounted, or maybe even make it into a trivial GObject (there's no reason why a channel manager should need to ref it, but introspected code will do it anyway) * TpChannelManagerRequestFunc's 2nd argument becomes a TpChannelManagerRequest* * tp_channel_manager_create_channel, ..._ensure_channel, _request_already_satisfied, _emit_request_failed[_printf], and the corresponding vfuncs should take that type too * tp_channel_manager_request_channel (and its vfunc) should be removed altogether * Write some silly Python that subclasses TpChannelManager and TpExportableChannel (you might need to temporarily remove "TpExportableChannel requires TpSvcChannel") * Verify that the chosen type for ::new-channel can work (In reply to comment #41) > MC looks good, also please delete the functions that deal with channel > details ((o, a{sv}) pair) if they're no longer called. (git grep details) Done. (2 top new patches). I guess I should wait to have a spec and tp-glib release before starting merging all this? (In reply to comment #45) > > - tp_tests_add_channel_to_ptr_array > > Can that function be removed entirely? Yes, removed. > + g_object_notify ((GObject *) self, "channel"); > + > + if (g_hash_table_lookup (self->priv->immutable_properties, > + TP_PROP_CHANNEL_DISPATCH_OPERATION_CHANNEL) == NULL) > + { > + g_hash_table_insert (self->priv->immutable_properties, > + g_strdup (TP_PROP_CHANNEL_DISPATCH_OPERATION_CHANNEL), > + tp_g_value_slice_new_boxed (DBUS_TYPE_G_OBJECT_PATH, path)); > + } > + > + if (g_hash_table_lookup (self->priv->immutable_properties, > + TP_PROP_CHANNEL_DISPATCH_OPERATION_CHANNEL_PROPERTIES) == NULL) > + { > + g_hash_table_insert (self->priv->immutable_properties, > + g_strdup (TP_PROP_CHANNEL_DISPATCH_OPERATION_CHANNEL_PROPERTIES), > + tp_g_value_slice_new_boxed ( > + TP_HASH_TYPE_STRING_VARIANT_MAP, properties)); > + } > +} > > Shouldn't we be updating internal state *before* emitting signals? Technically the "channel" property is just self->priv->channel which has just been updated. I just implemented the same logic as in maybe_set_connection() and maybe_set_account() but yeah, I can move all their g_object_notify() if you prefer. > > RequestableChannelClasses has been moved to Connection > > I think you still need to update the client API to match? Good catch; this wasn't properly tested so I missed it. I added an extra test. (In reply to comment #47) > Technically the "channel" property is just self->priv->channel which has > just been updated. I just implemented the same logic as in > maybe_set_connection() and maybe_set_account() but yeah, I can move all > their g_object_notify() if you prefer. I'd be OK with just moving that one g_object_notify to be after updating the object-path and immutable properties - yes in theory "channel" literally only means self->priv->channel, but I think there's value in having those closely-related things (appear to API users to) update simultaneously. (In reply to comment #46) > I guess I should wait to have a spec and tp-glib release before starting > merging all this? Yes, or make a set yourself. New patches look good so far (up to 4d2ba2ae3), assuming you apply the "fixup!" before merging. (In reply to comment #50) > New patches look good so far (up to 4d2ba2ae3), assuming you apply the > "fixup!" before merging. I squashed it now you reviewed it. (In reply to comment #48) > (In reply to comment #47) > > Technically the "channel" property is just self->priv->channel which has > > just been updated. I just implemented the same logic as in > > maybe_set_connection() and maybe_set_account() but yeah, I can move all > > their g_object_notify() if you prefer. > > I'd be OK with just moving that one g_object_notify to be after updating the > object-path and immutable properties - yes in theory "channel" literally > only means self->priv->channel, but I think there's value in having those > closely-related things (appear to API users to) update simultaneously. done. (In reply to comment #45) > * Turn TpBaseConnection's ChannelRequest into TpChannelManagerRequest > > * Make it refcounted, or maybe even make it into a trivial GObject > (there's no reason why a channel manager should need to ref it, > but introspected code will do it anyway) > > * TpChannelManagerRequestFunc's 2nd argument becomes a > TpChannelManagerRequest* > > * tp_channel_manager_create_channel, ..._ensure_channel, > _request_already_satisfied, _emit_request_failed[_printf], and the > corresponding vfuncs should take that type too > > * tp_channel_manager_request_channel (and its vfunc) should be removed > altogether > > * Write some silly Python that subclasses TpChannelManager and > TpExportableChannel (you might need to temporarily remove > "TpExportableChannel requires TpSvcChannel") > > * Verify that the chosen type for ::new-channel can work I implemented exactly this in http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-spec-sync The ugly python code to test is in http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=test-gir but it's not mergeable atm. This looks great! > - g_type_interface_add_prerequisite (type, TP_TYPE_SVC_CHANNEL); Yeah, I was worried you might have to do that in order to test it with GIR. I don't think not merging this test is a problem. > +#include "channel-manager-request-internal.h" Missing from 7dd315. It doesn't really matter a great deal, since the next patch adds it anyway. > + /*<private>*/ > + GObject parent; > + > + DBusGMethodInvocation *context; > + TpChannelManagerRequestMethod method; If the struct ever has to become public, I'd prefer this stuff to be in priv... talking of which, there's no priv pointer. No need to fix that right now though. > + unsigned yours : 1; I think we use gboolean (and avoid bitfield syntax) fairly consistently, since bitfields are a bit of a trap (they fail if signed, and gboolean is signed). > + g_return_val_if_fail (method < NUM_TP_CHANNEL_MANAGER_REQUEST_METHOD, NULL); TP_NUM_..._METHODS > + //GParamSpec *spec; Please delete any remaining commented-out code at the end of the branch, and use /* */ for anything intentionally left in. Please remove get_property, set_property, constructed, dispose unless you're going to use them later in the branch. > +void > +_tp_channel_manager_request_cancel (TpChannelManagerRequest *self) Now that we have a little less control over its lifetime, please g_return_if_fail (self->context != NULL) > +void > +_tp_channel_manager_request_satisfy (TpChannelManagerRequest *self, > + TpExportableChannel *channel) Here, too > +void > +_tp_channel_manager_request_fail (TpChannelManagerRequest *self, > + GError *error) And here (In reply to comment #53) > > +#include "channel-manager-request-internal.h" > > Missing from 7dd315. It doesn't really matter a great deal, since the next > patch adds it anyway. Fixed. > > + /*<private>*/ > > + GObject parent; > > + > > + DBusGMethodInvocation *context; > > + TpChannelManagerRequestMethod method; > > If the struct ever has to become public, I'd prefer this stuff to be in > priv... talking of which, there's no priv pointer. No need to fix that right > now though. I don't think it should, TpBaseConnection should be the only one having to use it directly as it implements Requests. I had to made it public in my WIP commit only to be able to instantiate a ChannelManager directly. > > + unsigned yours : 1; > > I think we use gboolean (and avoid bitfield syntax) fairly consistently, > since bitfields are a bit of a trap (they fail if signed, and gboolean is > signed). Fixed. > > + g_return_val_if_fail (method < NUM_TP_CHANNEL_MANAGER_REQUEST_METHOD, NULL); > > TP_NUM_..._METHODS Fixed. > > + //GParamSpec *spec; > > Please delete any remaining commented-out code at the end of the branch, and > use /* */ for anything intentionally left in. > > Please remove get_property, set_property, constructed, dispose unless you're > going to use them later in the branch. Done. > > +void > > +_tp_channel_manager_request_cancel (TpChannelManagerRequest *self) > > Now that we have a little less control over its lifetime, please > g_return_if_fail (self->context != NULL) > > > +void > > +_tp_channel_manager_request_satisfy (TpChannelManagerRequest *self, > > + TpExportableChannel *channel) > > Here, too > +void > > +_tp_channel_manager_request_fail (TpChannelManagerRequest *self, > > + GError *error) > > And here All done. (In reply to comment #54) > > If the struct ever has to become public, I'd prefer this stuff to be in > > priv... talking of which, there's no priv pointer. No need to fix that right > > now though. > > I don't think it should, TpBaseConnection should be the only one having to > use it directly as it implements Requests. That reasoning is fine, leave it as-is. > All done. Looks good. Please adjust the CMs to compile against this branch, then merge everything. Suggested release name, if you do this round of releases: "jigsaw falling into place". (In reply to comment #39) > I updated most CM. Only tests need to be updated: > http://cgit.collabora.com/git/user/cassidy/telepathy-gabble/log/?h=next-spec- > sync > http://cgit.collabora.com/git/user/cassidy/telepathy-salut/log/?h=next-new- > spec > http://cgit.collabora.com/git/user/cassidy/telepathy-idle/log/?h=next-spec- > sync > http://cgit.collabora.com/git/user/cassidy/telepathy-haze/log/?h=next-spec- > sync All those branches have been updated. Spec 0.99.7 released. tp-glib branch merged and released. (In reply to comment #56) > (In reply to comment #39) > > I updated most CM. Only tests need to be updated: > > http://cgit.collabora.com/git/user/cassidy/telepathy-gabble/log/?h=next-spec- > > sync > > http://cgit.collabora.com/git/user/cassidy/telepathy-salut/log/?h=next-new- > > spec > > http://cgit.collabora.com/git/user/cassidy/telepathy-idle/log/?h=next-spec- > > sync > > http://cgit.collabora.com/git/user/cassidy/telepathy-haze/log/?h=next-spec- > > sync > > All those branches have been updated. All merged. Rakia it's the last one TODO and then I think we can close this bug. All the CMs have been updated and tagged with 0.99.7; closing. |
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.