I have put together a TpHandler class that allows you to write a Client.Handler easily using tp-glib. You provide a channel filter and a handle-channels callback. It returns an object you can register on the bus that handles all of the properties for you (including HandledChannels). When HandleChannels is called, it converts all of the objects to their TpProxy equivalents and calls your callback for you. http://git.collabora.co.uk/?p=user/danni/telepathy-glib.git;a=shortlog;h=refs/heads/tphandler Things that warrant looking at: it's possible that the automatic tracking of HandledChannels needs improving for transient Client entries (e.g., where you've registered another client for a short period to have a second filter in place). Also, at the moment when you dispose the object it closes all the channels.
I've been messing with this today. I've already mentioned a few of these to you, but I thought I'd write them down here: * there needs to be a way to add user_data (or a weak object) to the HandleChannels callback * the coding style is not consistent with the rest of tp-glib * it would be great to also implement approver and observer. Maybe at that point this object should be TpClient instead * some documentation needs to be updated (such as whether the arguments in the HandleChannels callback are guaranteed to be ready/prepared * there are some leaks of GErrors in tp_handler_handle_channels I'm not trying to pretend that I've done a code review of it. I'm merely pointing out a few things that I think should be fixed. I'm working on adding approver support right now. It could be made a little prettier though (at the moment, the variable naming assumes the object is *just* a handler, for example). I can post my patch here after testing though.
tp_handler_signals[REMOVE_REQUEST] = g_signal_new ("remove-request", G_OBJECT_CLASS_TYPE (klass), G_SIGNAL_RUN_LAST, G_STRUCT_OFFSET (TpHandlerClass, remove_request), NULL, NULL, _tp_marshal_VOID__OBJECT_BOXED, G_TYPE_NONE, 3, TP_TYPE_CHANNEL_REQUEST, G_TYPE_STRING, G_TYPE_STRING); The marshaller seems wrong here.
Design ====== If the intention is for this to be used in simple cases where subclassing-in-C is too hard, it should perhaps be called TpSimpleHandler, and have a superclass TpBaseClient (which would currently do hardly anything and just be a shim around GObject, but could be extended over time). Can language bindings arrange for arbitrary code to run in their class_init? If so, the TpBaseClient base class could have a "hidden vtable" like the TpSvcFooIface interfaces do, with class methods tp_base_client_class_implement_handle_channels() etc., allowing us to add more methods without having to reserve lots of ABI padding in the class struct. To be honest I'd be more inclined to implement Observer first - it's the easiest of the three client classes (Handler is next, and Approver is the hardest, IMO). I don't like the way the handler object does almost all the work, but then requires you to request a bus name and register its object path? I'd be more in favour of some sort of all-in-one "registration" function, like perhaps: client = tp_base_client_new (dbus_daemon, ...); tp_base_client_set_bypass_approval (client, TRUE); ... more setup functions that you can only call *before* register() ... /* registers you on @dbus_daemon */ tp_base_client_register (client); Would it be better for language bindings, etc., if the HandleChannels pseudo-virtual-method was a signal, with it documented that you should have exactly one handler for the signal? It seems strange that TpHandlerHandleChannelsCb returns a boolean, but on failure can only return one error (NotAvailable). Shouldn't it let you return any GError? I think the HandleChannels pseudo-virtual-method should have as its first argument some sort of opaque context object. In telepathy-qt4, the equivalent methods have a first parameter that abstracts returning from the D-Bus method (it's a wrapper around the Qt equivalent of a DBusGMethodInvocation). The C equivalent would be something like: void my_handle_channels_impl (TpHandler *self, TpHandlerContext *context, TpAccount *account, TpConnection *connection, TpChannel **channels, TpChannelRequest **requests, guint64 user_action_time); /* it is an error to not call one of these methods in HandleChannels */ void tp_handler_context_delay (TpHandlerContext *); void tp_handler_context_accept (TpHandlerContext *); void tp_handler_context_fail (TpHandlerContext *, const GError *); I think the Handler_Info dict should also be kept in the context object, so that when we start defining keys for it, we can do things like this: /* generic extension API */ const GValue *tp_handler_context_get_info (TpHandlerContext *, const gchar *); /* imagine we'd defined keys "is-badgered" (b) and "mushroom-names" (as) */ gboolean tp_handler_context_is_badgered (TpHandlerContext *); GStrv tp_handler_context_dup_mushroom_names (TpHandlerContext *); We could even move (expected-to-be-)rarely-used arguments into the context object: the channel requests, perhaps. Details ======= Why do the add-request, remove-request signals need vtable slots as well? I'd be inclined to make add-request only contain the TpChannelRequest, and give it an accessor for its own immutable properties. A method called lookup_channel_request shouldn't return a ref; calling it ensure_channel_request would be better, I think. remove-request shouldn't be emitted at all for requests that we didn't already know about, so we still need a lookup_channel_request, but one with lookup (i.e. borrow-if-exists) semantics :-)
Here's how I see a Client base class working: http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=blob;f=telepathy-glib/base-client.h;hb=sketch-base-client From that basis, it'd be possible to implement a TpSimpleHandler that doesn't subclass, or something that subclasses and sets things up from information in the class struct, or whatever. In tp_base_client_new(), uniquify_name = TRUE would append a unique token to the name, to make it easier to make temporary client heads. Perhaps the final name would be (in pseudo-Python) "%s.%s.n%u" % (name, tp_escape_as_identifier(unique_name), incrementing_counter_per_process) or some such, which would lead to (Empathy @ :1.42)'s third temporary client being called ...Client.Empathy._3a1_2e42.n3 or something. (Well-known bus name components can't start with a digit even though unique bus name components can, which is why escaping and so on are necessary, even though all we're doing is embedding a bus name in another bus name... thanks, Havoc.)
(In reply to comment #3) > Design > ====== FWIW, I like all of Simon's suggestions.
Is it ok to call G_IMPLEMENT_INTERFACE even if we don't actually implement the iface? tp_base_client_register() should probably check that at least of the observer/approver/handler has been set as it's not valid to be a Client without implemeneting at least one of those interface. Any reason to not have a observer and approver variant of tp_base_client_be_a_handler() ? I'm not sure CLIENT_HANDLER_BYPASSES_APPROVAL should be a flag. It's pretty different from others flags as it doesn't affect the interfaces implemented. Any reason to call this TpBaseClient rather than TpClient? What should do function like _tp_base_client_observe_channels()? Call a virtual method or... ? implementation detai: finalize shouldn't use g_hash_table_destroy to destroy filters.
(In reply to comment #6) >Is it ok to call G_IMPLEMENT_INTERFACE even if we don't actually implement the > iface? IMO, yes, as long as we don't put it in the Interfaces property. > tp_base_client_register() should probably check that at least of the > observer/approver/handler has been set as it's not valid to be a Client without > implemeneting at least one of those interface. Yes, that sounds sensible. > Any reason to not have a observer and approver variant of > tp_base_client_be_a_handler() ? It doesn't (currently) make sense to be an Observer or Approver with an empty list of filters. It does make sense to be a Handler with an empty list of filters (it means you'll only handle channels for which you are the PreferredHandler). > I'm not sure CLIENT_HANDLER_BYPASSES_APPROVAL should be a flag. It's pretty > different from others flags as it doesn't affect the interfaces implemented. As of spec 0.19.5 (which is newer than this sketch) we want another such flag, CLIENT_OBSERVER_SHOULD_RECOVER (it's in the sketch, in fact, but is commented out). > Any reason to call this TpBaseClient rather than TpClient? TpClient already exists, and is a TpProxy subclass (used by e.g. MC) to talk to a client :-) (This is consistent with our naming for Connections and ConnectionManagers.) > What should do function like _tp_base_client_observe_channels()? Call a virtual > method or... ? I'm not sure. I think either a virtual method or a signal emission would make sense; see above. (If we also want something resembling Danielle's TpHandler, we should call it TpSimpleHandler, make it a TpBaseClient subclass, and have it take filters and a callback in its constructor for C developers' convenience; similarly for TpSimpleObserver and potentially TpSimpleApprover.) > implementation detai: finalize shouldn't use g_hash_table_destroy to destroy > filters. Fair enough. (But, note that this object copies its filters rather than reffing them. This is the only safe thing to do, because you can't know how callers will behave.)
(In reply to comment #7) > It doesn't (currently) make sense to be an Observer or Approver with an empty > list of filters. It does make sense to be a Handler with an empty list of > filters (it means you'll only handle channels for which you are the > PreferredHandler). Really? I did write test approvers and observers having an empty list of filters. They were called for all channels. > > I'm not sure CLIENT_HANDLER_BYPASSES_APPROVAL should be a flag. It's pretty > > different from others flags as it doesn't affect the interfaces implemented. > > As of spec 0.19.5 (which is newer than this sketch) we want another such flag, > CLIENT_OBSERVER_SHOULD_RECOVER (it's in the sketch, in fact, but is commented > out). I'm afraid we'll end up with a lot of different not related flags when clients will grow more feature. But yeah, implementation detail. > > What should do function like _tp_base_client_observe_channels()? Call a virtual > > method or... ? > > I'm not sure. I think either a virtual method or a signal emission would make > sense; see above. > > (If we also want something resembling Danielle's TpHandler, we should call it > TpSimpleHandler, make it a TpBaseClient subclass, and have it take filters and > a callback in its constructor for C developers' convenience; similarly for > TpSimpleObserver and potentially TpSimpleApprover.) I think I prefer the virtual method + callback passed to constructor for Simple versions approach. Signals doesn't fit very well here.
(In reply to comment #8) > Really? I did write test approvers and observers having an empty list of > filters. They were called for all channels. Are you sure you mean an empty list of filters, as opposed to a list containing only an empty filter? Here's what's meant to happen: ApproverChannelFilters = [] # matches nothing ApproverChannelFilters = [{}] # matches everything ApproverChannelFilters = [{x: y}] # matches some things (common case) If you really do mean that an empty list matches everything, please file a bug on MC - it's not meant to behave like that. > I think I prefer the virtual method + callback passed to constructor for Simple > versions approach. > Signals doesn't fit very well here. Fair enough, although one reason I was in favour of using signals was that they're probably easier to do in non-C languages. We could always make a subclass that uses a signal (TpSignallingObserver?) though, if we need that.
(In reply to comment #9) > (In reply to comment #8) > > Really? I did write test approvers and observers having an empty list of > > filters. They were called for all channels. > > Are you sure you mean an empty list of filters, as opposed to a list containing > only an empty filter? Here's what's meant to happen: > > ApproverChannelFilters = [] # matches nothing > ApproverChannelFilters = [{}] # matches everything That was what I meant. Thanks for the clarification.
(In reply to comment #7) > (In reply to comment #6) > >Is it ok to call G_IMPLEMENT_INTERFACE even if we don't actually implement the > > iface? > > IMO, yes, as long as we don't put it in the Interfaces property. One issue here: all the interfaces appear in the D-Bus introspection.
(In reply to comment #11) > One issue here: all the interfaces appear in the D-Bus introspection. As long as you return an appropriate error or stub result, I don't mind. MC believes the Interfaces property, not the Introspect() output, and so should everything else.
(In reply to comment #10) > > Are you sure you mean an empty list of filters, as opposed to a list containing > > only an empty filter? Here's what's meant to happen: > > > > ApproverChannelFilters = [] # matches nothing > > ApproverChannelFilters = [{}] # matches everything > > That was what I meant. Thanks for the clarification. We should copy this information to the spec.
I implemented the proposed API for Observer: http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/base-client-25236 Please take a quick look and tell me if I'm going to the right direction. The first commit has been amended so it's not your original commit any more.
I haven't done a detailed review, just skim-reading. You're going in the right sort of direction, except that ObserveChannels should be more OO and more convenient. > tp_base_client_register (TpBaseClient *self) ... > + dbus_g_connection_register_g_object ( This could use the new API on TpDBusDaemon to not need the DBusGConnection. > +struct _TpBaseClientClass { > + /*<private>*/ > + GObjectClass parent_class; > + TpDBusPropertiesMixinClass dbus_properties_class; > +}; This should probably have a bit of ABI padding; add GCallback[4] or something? > +TpObserveChannelsContext * tp_observe_channels_context_new ( Internal functions like this one should start with "_tp" so they're not ABI. > + g_type_add_class_private (g_define_type_id, sizeof ( Needs a dependency on GLib 2.24. I think it's legitimate for 0.11.x to have that dependency, but wjt might disagree. > +typedef void (*TpBaseClientClassObserveChannelsImpl) ( > + TpBaseClient *self, > + const gchar *account, > + const gchar *connection, > + const GPtrArray *channels, > + const gchar *dispatch_operation, > + const GPtrArray *requests, > + TpObserveChannelsContext *context); @account should be a TpAccount that's guaranteed to have CORE ready. @connection should be a TpConnection that's guaranteed to have CORE ready. (Note that this does not guarantee it's CONNECTED!) @channels should be a GList of TpChannel, each of which is guaranteed to have CORE ready. @dispatch_operation should be a TpChannelDispatchOperation or NULL. It shouldn't be (guaranteed|trying) to be ready, even when TpChannelDispatchOperation has a decent amount of API (because most observers won't want it to be ready). @requests should be a GList of TpChannelRequest, which have at least their object paths (so they can be compared for equality) but are otherwise not (guaranteed|trying) to be ready. TpBaseClient should probably have a way to ask for extra features on every Account, every Connection and every Channel, too, but that's not critical; we can add it later.
(In reply to comment #15) > > tp_base_client_register (TpBaseClient *self) > ... > > + dbus_g_connection_register_g_object ( > > This could use the new API on TpDBusDaemon to not need the DBusGConnection. done. > > +struct _TpBaseClientClass { > > + /*<private>*/ > > + GObjectClass parent_class; > > + TpDBusPropertiesMixinClass dbus_properties_class; > > +}; > > This should probably have a bit of ABI padding; add GCallback[4] or something? I used GCallback _padding[7]; like in the existing objects. > > +TpObserveChannelsContext * tp_observe_channels_context_new ( > > Internal functions like this one should start with "_tp" so they're not ABI. Aleady fixed. :) > > + g_type_add_class_private (g_define_type_id, sizeof ( > > Needs a dependency on GLib 2.24. I think it's legitimate for 0.11.x to have > that dependency, but wjt might disagree. bumped. > > +typedef void (*TpBaseClientClassObserveChannelsImpl) ( > > + TpBaseClient *self, > > + const gchar *account, > > + const gchar *connection, > > + const GPtrArray *channels, > > + const gchar *dispatch_operation, > > + const GPtrArray *requests, > > + TpObserveChannelsContext *context); > > @account should be a TpAccount that's guaranteed to have CORE ready. > > @connection should be a TpConnection that's guaranteed to have CORE ready. > (Note that this does not guarantee it's CONNECTED!) > > @channels should be a GList of TpChannel, each of which is guaranteed to have > CORE ready. > > @dispatch_operation should be a TpChannelDispatchOperation or NULL. It > shouldn't be (guaranteed|trying) to be ready, even when > TpChannelDispatchOperation has a decent amount of API (because most observers > won't want it to be ready). > > @requests should be a GList of TpChannelRequest, which have at least their > object paths (so they can be compared for equality) but are otherwise not > (guaranteed|trying) to be ready. > > TpBaseClient should probably have a way to ask for extra features on every > Account, every Connection and every Channel, too, but that's not critical; we > can add it later. Ok, I'll make those changes.
I think the Observer API is basically done: http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/base-client-25236 Please take a look and let me know how it looks to you. Then I'll mimic this API (and implementation) for Handler and Approver.
The Observer needs documentation; Guillaume is working on it already. > +test_basis (Test *test, Do you mean test_basics? :-) > + "/org/freedesktop/Telepathy/Account/fake", > + "/org/freedesktop/Telepathy/Connection/fake", I'd prefer a syntactically valid object path (".../fake/fake/fake" would be sufficient). > + param_spec = g_param_spec_pointer ("dbus-context", "D-Bus context", > + "The DBusGMethodInvocation associated with the ObserveChannels call", > + G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS); I think this should be write-only - once the TpOCC has been constructed, it's responsible for replying to the DBusGMI, and nobody else can do so safely. If the client wants to find out the caller's unique name (which is the only other thing you do with a DBusGMI, in practice), we can add tp_o_c_c_get_caller(). > +tp_observe_channels_context_accept (TpObserveChannelsContext *self) I'd prefer it if this, and fail(), had a check for "already done that" - probably a g_return_if_fail. They should NULL out priv->dbus_context when called (because replying frees the context). Now that you've added the concept of status later in the branch, you can use that for the check. > + g_type_add_class_private (g_define_type_id, sizeof ( > + TpBaseClientClassPrivate));) Nitpicking: this introduces an empty statement, which causes some compilers to warn. Get rid of the trailing semicolon to fix that. > + DEBUG ("ObserveChannels has not be implemented"); "not been implemented", and I'd prefer "class %s does not implement ObserveChannels" % G_OBJECT_TYPE_NAME (self) > + g_warning ("Implementation of ObserveChannels didn't call " > + "tp_observe_channels_context_{accept,fail,delay}"); I'd like a G_OBJECT_TYPE_NAME here, and it should probably be a g_critical(); also, it should probably reply on D-Bus, perhaps with NotImplemented or NotAvailable (we don't have a NotImplementedCorrectly error yet). > + [glib-2.0 >= 2.24, gobject-2.0 >= 2.22, gio-2.0 >= 2.22]) These three come from the same source tarball, so there's no point in not bumping all three versions; I think it's more explicit / clearer if you keep all three the same. Please also bump the versions in the pkg-config? > TpBaseClientClassPrivate *klass_pv = G_TYPE_CLASS_GET_PRIVATE ( I'd prefer class_priv, or possibly cls_priv if you renamed klass to cls. The only reason to mis-spell "class" is to be nice to C compilers that are actually a C++ compiler in disguise. > + tp_value_array_unpack (g_ptr_array_index (channels_arr, i), 2, > + &chan_path, &chan_props); > + > + channel = tp_channel_new_from_properties (connection, > + chan_path, chan_props, &error); > + if (channel == NULL) This leaks the path and properties. I'd free and unref them (respectively) after creating the Channel, but before checking whether it's NULL. > +struct _TpObserveChannelsContext { ... > + /* List of reffed TpChannelDispatchOperation */ > + TpChannelDispatchOperation *dispatch_operation; I think this comment is a lie - it should be "reffed, or NULL". > + GPtrArray *requests; Deserves a comment. > + channels = g_ptr_array_new_with_free_func (g_object_unref); Perhaps use g_ptr_array_sized_new (channels_arr->len) and call g_ptr_array_set_free_func immediately after, to save reallocs? The same for the ChannelRequests. > tp_observe_channels_context_prepare_async (TpObserveChannelsContext *self, ... > g_return_if_fail (self->priv->result == NULL); I think prepare_async, prepare_finish could be internal, in which case this assertion would be acceptable (library users could never hit it). Otherwise, it's inappropriate. > + if (!tp_proxy_is_prepared (channel, TP_CHANNEL_FEATURE_CORE)) > + return FALSE; I wonder whether we should also guarantee GROUP. > -#include <telepathy-glib/base-client-context-internal.h> > +#include <telepathy-glib/observe-channels-context-internal.h> I'd prefer it if you kept this block of headers (in base-client.c) in alphabetical order (rationale: makes it more likely that conflicts have a trivial resolution). > Disable Approver and Handler support for now It might be cleaner to do this as a deletion of code, and git revert it when working on A and H code. I don't know whether A or H is better to do first. Perhaps before doing either, it would be worth doing a TpSimpleObserver that loosely resembles Danielle's TpHandler (i.e. you instantiate it and give it a list of filters, a function pointer for the callback, and some sort of user_data for the callback, rather than having to subclass it: Danielle's rationale is that subclassing in GObject/C is unnecessarily hard).
(In reply to comment #18) > The Observer needs documentation; Guillaume is working on it already. > > > +test_basis (Test *test, > > Do you mean test_basics? :-) fixed. > > + "/org/freedesktop/Telepathy/Account/fake", > > + "/org/freedesktop/Telepathy/Connection/fake", > > I'd prefer a syntactically valid object path (".../fake/fake/fake" would be > sufficient). Done. > > + param_spec = g_param_spec_pointer ("dbus-context", "D-Bus context", > > + "The DBusGMethodInvocation associated with the ObserveChannels call", > > + G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS); > > I think this should be write-only - once the TpOCC has been constructed, it's > responsible for replying to the DBusGMI, and nobody else can do so safely. If > the client wants to find out the caller's unique name (which is the only other > thing you do with a DBusGMI, in practice), we can add tp_o_c_c_get_caller(). done. > > +tp_observe_channels_context_accept (TpObserveChannelsContext *self) > > I'd prefer it if this, and fail(), had a check for "already done that" - > probably a g_return_if_fail. They should NULL out priv->dbus_context when > called (because replying frees the context). done. > Now that you've added the concept of status later in the branch, you can use > that for the check. _tp_observe_channels_context_get_state is an internal API. Do you think I should move the state attribute as a public one? > > + g_type_add_class_private (g_define_type_id, sizeof ( > > + TpBaseClientClassPrivate));) > > Nitpicking: this introduces an empty statement, which causes some compilers to > warn. Get rid of the trailing semicolon to fix that. removed. > > + DEBUG ("ObserveChannels has not be implemented"); > > "not been implemented", and I'd prefer "class %s does not implement > ObserveChannels" % G_OBJECT_TYPE_NAME (self) fixed. > > + g_warning ("Implementation of ObserveChannels didn't call " > > + "tp_observe_channels_context_{accept,fail,delay}"); > > I'd like a G_OBJECT_TYPE_NAME here, and it should probably be a g_critical(); > also, it should probably reply on D-Bus, perhaps with NotImplemented or > NotAvailable (we don't have a NotImplementedCorrectly error yet). done. > > + [glib-2.0 >= 2.24, gobject-2.0 >= 2.22, gio-2.0 >= 2.22]) > > These three come from the same source tarball, so there's no point in not > bumping all three versions; I think it's more explicit / clearer if you keep > all three the same. Please also bump the versions in the pkg-config? done (I amend the commit). > > TpBaseClientClassPrivate *klass_pv = G_TYPE_CLASS_GET_PRIVATE ( > > I'd prefer class_priv, or possibly cls_priv if you renamed klass to cls. The > only reason to mis-spell "class" is to be nice to C compilers that are actually > a C++ compiler in disguise. renamed. > > + tp_value_array_unpack (g_ptr_array_index (channels_arr, i), 2, > > + &chan_path, &chan_props); > > + > > + channel = tp_channel_new_from_properties (connection, > > + chan_path, chan_props, &error); > > + if (channel == NULL) > > This leaks the path and properties. I'd free and unref them (respectively) > after creating the Channel, but before checking whether it's NULL. fixed. > > +struct _TpObserveChannelsContext { > ... > > + /* List of reffed TpChannelDispatchOperation */ > > + TpChannelDispatchOperation *dispatch_operation; > > I think this comment is a lie - it should be "reffed, or NULL". fixed. > > + GPtrArray *requests; > > Deserves a comment. done. > > + channels = g_ptr_array_new_with_free_func (g_object_unref); > > Perhaps use g_ptr_array_sized_new (channels_arr->len) and call > g_ptr_array_set_free_func immediately after, to save reallocs? The same for the > ChannelRequests. done. > > tp_observe_channels_context_prepare_async (TpObserveChannelsContext *self, > ... > > g_return_if_fail (self->priv->result == NULL); > > I think prepare_async, prepare_finish could be internal, in which case this > assertion would be acceptable (library users could never hit it). Otherwise, > it's inappropriate. Already made it internal. :) > > + if (!tp_proxy_is_prepared (channel, TP_CHANNEL_FEATURE_CORE)) > > + return FALSE; > > I wonder whether we should also guarantee GROUP. We could stick to CORE for now and add _set_channel_features () API later. Or just add GROUP to be coherent with tp_channel_call_when_ready(). You call. > > -#include <telepathy-glib/base-client-context-internal.h> > > +#include <telepathy-glib/observe-channels-context-internal.h> > > I'd prefer it if you kept this block of headers (in base-client.c) in > alphabetical order (rationale: makes it more likely that conflicts have a > trivial resolution). sorted. > > Disable Approver and Handler support for now > > It might be cleaner to do this as a deletion of code, and git revert it when > working on A and H code. Sure; but I prefer to do the deletion commit just before merging so I can easily revert it right after and avoid lolo conflicts. > I don't know whether A or H is better to do first. Perhaps before doing either, > it would be worth doing a TpSimpleObserver that loosely resembles Danielle's > TpHandler (i.e. you instantiate it and give it a list of filters, a function > pointer for the callback, and some sort of user_data for the callback, rather > than having to subclass it: Danielle's rationale is that subclassing in > GObject/C is unnecessarily hard). I think TpSimpleObserver is a good second step. What about Recovering? How this API should deal with it?
(In reply to comment #19) > > > + tp_value_array_unpack (g_ptr_array_index (channels_arr, i), 2, > > > + &chan_path, &chan_props); > > > + > > > + channel = tp_channel_new_from_properties (connection, > > > + chan_path, chan_props, &error); > > > + if (channel == NULL) > > > > This leaks the path and properties. I'd free and unref them (respectively) > > after creating the Channel, but before checking whether it's NULL. Humm actually not. From tp_value_array_unpack's documentation: "The contents of the values aren't copied into the variables, and so become invalid when array is freed."
(In reply to comment #20) > Humm actually not. From tp_value_array_unpack's documentation: > "The contents of the values aren't copied into the variables, and so become > invalid when array is freed." Oh, I obviously wasn't reading thoroughly enough... I should change the example in the docs to use a const gchar *, then.
I pushed a first version of documentation to the branch (not including annotations yet).
http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/base-client-25236 now includes documentation and annotations (not sure if that's enough though). http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/simple-observer is a first implementation of TpSimpleObserver making use of TpBaseClient.
This looks as though it's on track. I think it's worth merging as soon as it can support observers cleanly. (In reply to comment #21) > Oh, I obviously wasn't reading thoroughly enough... I should change the example > in the docs to use a const gchar *, then. (Fixed in master, does not affect 0.10.) Review ====== > tp_observe_channels_context_get_recovering is_recovering(), perhaps? > + * TpBaseClientClassObserveChannelsImpl: ... > + * @dispatch_operation: a #TpChannelDispatchOperation or %NULL; the > + * dispatch_operation is not garanteed to be prepared "not guaranteed to" (same misspelling in @requests). Annotations needed (where "..." means the docs you already have): @channels: (element-type Tp.Channel): ... @requests: (element-type Tp.ChannelRequest): ... @dispatch_operation: (allow-none): ... I'd be inclined to call the first parameter "client" or "observer" rather than "self" here, but that's a matter of taste. > + * tp_base_client_add_observer_filter: ... > + * @filter: (transfer none) (element-type utf8 GObject.Value): This seems to contain non-breaking spaces in the annotations, for some reason? Is that what you intended? (To see non-breaking spaces in vim, ":set list listchars=tab:»→,trail:…,nbsp:‽" or some such.) It would be nice to assert (with g_return_if_fail) that there is an observe_channels implementation. You should explicitly say that the client mustn't have been registered, and I don't think this is really enough documentation (it should be possible to guess more or less what's going on without referring to telepathy-spec). How about this? /** * tp_base_client_add_observer_filter: * @self: a #TpBaseClient * @filter: (transfer none) (element-type utf8 GObject.Value): * a %TP_HASH_TYPE_CHANNEL_CLASS * * Register a new channel class as Observer.ObserverChannelFilter. * The @observe_channels virtual method set up using * tp_base_client_implement_observe_channels() will be called whenever * a new channel's properties match the ones in @filter. * * This method may only be called before tp_base_client_register() is * called, and may only be called on objects that implement * @observe_channels. * * Since: 0.11.UNRELEASED */ > + * tp_base_client_take_observer_filter: ... > + * @filter: (transfer container) (element-type utf8 GObject.Value): This contains non-breaking spaces, and parameters that are stolen (like this one) should be (transfer full). Also, the entire function should really be (skip) (there's no point in it for language bindings), but that breaks gtk-doc <= 1.14, so we can't do that until 1.15 comes out (or at least until we get a gtk-doc with patches from upstream git into major distributions). > + * Define the value of the Observer.Recover property. That's not really enough. How about this? * Set whether the channel dispatcher should attempt to recover * this Observer if it crashes. (This is implemented by setting * the value of its Recover D-Bus property.) * * Normally, Observers are only notified when new channels * appear. If an Observer is set to recover, when it registers with * tp_base_client_register(), it will also be told about any channels * that already existed before it started. * * For Observers that are activatable as a D-Bus service, if the * Observer exits or crashes while there are any channels that match * its filter, it will automatically be restarted by service-activation. * * This method may only be called before tp_base_client_register() is * called, and may only be called on objects that implement * @observe_channels. > + * tp_base_client_register: ... > + * Returns: %TRUE if the client has been registered. > + * Methods that set the filters and other immutable state cannot be called > + * after this one. I'd prefer an example: "... state, such as tp_base_client_add_observer_filter(), cannot ..." > + * tp_base_client_register: ... > + * Returns: %TRUE if the client has been registered. I'd prefer this worded as "if the client was registered successfully" - your wording makes it sound as if, if you call this method twice, the second call will also return TRUE. > + g_warning ("Failed to register bus name %s\n", string->str); No thanks. If registering the bus name failed, just raise the GError - it's up to the caller what they do about it. > + path = g_strdup_printf ("/%s", g_strdelimit (string->str, ".", '/')); g_strdelimit edits the string in-place. It's OK here because you don't re-use it, but I'd prefer it to be obviously correct at a glance: path = g_strdup_printf ("/%s", string->str); g_strdelimit (path, ".", '/'); > + case PROP_DBUS_DAEMON: > + g_value_set_object (value, self->priv->dbus); > + break; > + case PROP_NAME: I vaguely prefer to have a blank line between cases in a switch, although it doesn't really matter when the implementations are so small. > + case PROP_DBUS_DAEMON: > + self->priv->dbus = g_value_dup_object (value); I'd prefer this case to start with: g_assert (self->priv->dbus == NULL); /* construct-only */ to make it obvious why it isn't a potential leak. Similar for PROP_NAME. > +#if 0 > + case DP_APPROVER_CHANNEL_FILTER: If you're going to #if these, you should do the same to their TpDBusPropertiesMixinPropImpl and TpDBusPropertiesMixinIfaceImpl structs. Otherwise, you'll be remotely-assertable (by retrieving the offending properties). I wonder whether there should be a property and/or accessor for the client's bus name and object path? If so, they should be set during the constructor based on name and uniquify, and then used as-is at register time (i.e. move some code from register to constructor). > + TpBaseClientClassPrivate *cls_pv = G_TYPE_CLASS_GET_PRIVATE ( > + TP_BASE_CLIENT_GET_CLASS (self), TP_TYPE_BASE_CLIENT, > + TpBaseClientClassPrivate); Please avoid nonstandard abbreviations like "pv" where feasible (private things are called private or priv). Can we cache this in FooClass.priv in class_init, like the way we cache object private structures in Foo.priv in the instance init? Then you could use cls->priv->observe_channels_impl(). > + "Implementation of ObserveChannels of %s didn't call " Nitpicking: "Implementation of ObserveChannels in %s" would avoid the repeated "of" while remaining grammatically correct. > + * tp_observe_channels_context_delay: > + * @self: a #TpObserveChannelsContext > + * > + * Called by #TpBaseClientClassObserveChannelsImpl to indicate that it > + * implements the method in an async way. It has to ref the > + * #TpObserveChannelsContext before calling this function and is responsible > + * to call tp_observe_channels_context_accept once the call is done. I'd prefer: ... an async way. The caller must take a reference to the #TpObserveChannelsContext before calling this function, and is responsible for calling either tp_observe_channels_context_accept() or tp_observe_channels_context_fail() later. If disposed in state NONE or DELAYED, the context should probably g_critical(), and raise NotImplemented on D-Bus. > + * If the "recovering" key in present in the Observer_Info hash table > + * associated with this context, its value; %FALSE if the key is not present. > + * > + * Returns: value That describes the mechanism, but not the purpose. I'd prefer: * If this call to ObserveChannels is for channels that already * existed before this observer started (because the observer used * tp_base_client_set_observer_recover()), return %TRUE. * * In most cases, the result is %FALSE. * * Returns: %TRUE for pre-existing channels, %FALSE for new channels Perhaps ..._is_recovering() would be a better name? I don't know. > +_tp_observe_channels_context_prepare_async (TpObserveChannelsContext *self, ... > + g_return_if_fail (self->priv->result == NULL); This deserves a comment, something like: /* This is only used once, by TpBaseClient, so for simplicity, we only * allow one asynchronous preparation */ Still to review =============== I haven't reviewed the test or the SimpleClient yet. Future work =========== I think we should get Observers working nicely (including an example and perhaps TpSimpleObserver) before we move on to the other classes. After that, I'd recommend Handler next - Approver is harder, because it needs more API on ChannelDispatchOperation. > +/** > + * SECTION:base-client > + * @title: TpBaseClient > + * @short_description: base class for Telepathy clients on D-Bus > + * > + * This base class makes it easier to write #TpSvcClient > + * implementations. Subclasses should usually pass the filters they > + * want and override the D-Bus methods they implement. > + */ Eventually, I'd like a code example or a reference to TpSimpleObserver (etc.) here. It'd be nice to have an observer in examples/client/ that just does a g_message() whenever a StreamedMedia call starts or finishes, or something similarly easy.
(In reply to comment #24) > This looks as though it's on track. I think it's worth merging as soon as it > can support observers cleanly. Agreed. > > tp_observe_channels_context_get_recovering > > is_recovering(), perhaps? renamed. > > + * TpBaseClientClassObserveChannelsImpl: > ... > > + * @dispatch_operation: a #TpChannelDispatchOperation or %NULL; the > > + * dispatch_operation is not garanteed to be prepared > > "not guaranteed to" (same misspelling in @requests). > > Annotations needed (where "..." means the docs you already have): > > @channels: (element-type Tp.Channel): ... > @requests: (element-type Tp.ChannelRequest): ... > @dispatch_operation: (allow-none): ... > > I'd be inclined to call the first parameter "client" or "observer" rather than > "self" here, but that's a matter of taste. Fixed. > > + * tp_base_client_add_observer_filter: > ... > > + * @filter: (transfer none) (element-type utf8 GObject.Value): > > This seems to contain non-breaking spaces in the annotations, for some reason? > Is that what you intended? Humm weird. > (To see non-breaking spaces in vim, ":set list listchars=tab:»→,trail:…,nbsp:‽" > or some such.) > > It would be nice to assert (with g_return_if_fail) that there is an > observe_channels implementation. Done in tp_base_client_take_observer_filter as _add_ uses this one. > You should explicitly say that the client mustn't have been registered, and I > don't think this is really enough documentation (it should be possible to guess > more or less what's going on without referring to telepathy-spec). How about > this? > > /** > * tp_base_client_add_observer_filter: > * @self: a #TpBaseClient > * @filter: (transfer none) (element-type utf8 GObject.Value): > * a %TP_HASH_TYPE_CHANNEL_CLASS > * > * Register a new channel class as Observer.ObserverChannelFilter. > * The @observe_channels virtual method set up using > * tp_base_client_implement_observe_channels() will be called whenever > * a new channel's properties match the ones in @filter. > * > * This method may only be called before tp_base_client_register() is > * called, and may only be called on objects that implement > * @observe_channels. > * > * Since: 0.11.UNRELEASED > */ I replaced doc by this. > > + * tp_base_client_take_observer_filter: > ... > > + * @filter: (transfer container) (element-type utf8 GObject.Value): > > This contains non-breaking spaces, and parameters that are stolen (like this > one) should be (transfer full). fixed. > Also, the entire function should really be (skip) (there's no point in it for > language bindings), but that breaks gtk-doc <= 1.14, so we can't do that until > 1.15 comes out (or at least until we get a gtk-doc with patches from upstream > git into major distributions). I added a FIXME. > > + * Define the value of the Observer.Recover property. > > That's not really enough. How about this? > > * Set whether the channel dispatcher should attempt to recover > * this Observer if it crashes. (This is implemented by setting > * the value of its Recover D-Bus property.) > * > * Normally, Observers are only notified when new channels > * appear. If an Observer is set to recover, when it registers with > * tp_base_client_register(), it will also be told about any channels > * that already existed before it started. > * > * For Observers that are activatable as a D-Bus service, if the > * Observer exits or crashes while there are any channels that match > * its filter, it will automatically be restarted by service-activation. > * > * This method may only be called before tp_base_client_register() is > * called, and may only be called on objects that implement > * @observe_channels. Much better; I changed. I also added a g_return_if_fail checking that observe_channels_impl has been defined. > > + * tp_base_client_register: > ... > > + * Returns: %TRUE if the client has been registered. > > + * Methods that set the filters and other immutable state cannot be called > > + * after this one. > > I'd prefer an example: "... state, such as > tp_base_client_add_observer_filter(), cannot ..." done. > > + * tp_base_client_register: > ... > > + * Returns: %TRUE if the client has been registered. > > I'd prefer this worded as "if the client was registered successfully" - your > wording makes it sound as if, if you call this method twice, the second call > will also return TRUE. changed. > > + g_warning ("Failed to register bus name %s\n", string->str); > > No thanks. If registering the bus name failed, just raise the GError - it's up > to the caller what they do about it. replaced by a DEBUG(). > > + path = g_strdup_printf ("/%s", g_strdelimit (string->str, ".", '/')); > > g_strdelimit edits the string in-place. It's OK here because you don't re-use > it, but I'd prefer it to be obviously correct at a glance: > > path = g_strdup_printf ("/%s", string->str); > g_strdelimit (path, ".", '/'); changed. > > + case PROP_DBUS_DAEMON: > > + g_value_set_object (value, self->priv->dbus); > > + break; > > + case PROP_NAME: > > I vaguely prefer to have a blank line between cases in a switch, although it > doesn't really matter when the implementations are so small. done. > > + case PROP_DBUS_DAEMON: > > + self->priv->dbus = g_value_dup_object (value); > > I'd prefer this case to start with: > > g_assert (self->priv->dbus == NULL); /* construct-only */ > > to make it obvious why it isn't a potential leak. Similar for PROP_NAME. done. > > +#if 0 > > + case DP_APPROVER_CHANNEL_FILTER: > > If you're going to #if these, you should do the same to their > TpDBusPropertiesMixinPropImpl and TpDBusPropertiesMixinIfaceImpl structs. > Otherwise, you'll be remotely-assertable (by retrieving the offending > properties). As said in an earlier comment, ,I'll remove the code once the branch is ready to be merged (to be able to revert the commit without conflict). I #if 0 them for now. > I wonder whether there should be a property and/or accessor for the client's > bus name and object path? If so, they should be set during the constructor > based on name and uniquify, and then used as-is at register time (i.e. move > some code from register to constructor). Sounds like a good idea. I'll add that. > > + TpBaseClientClassPrivate *cls_pv = G_TYPE_CLASS_GET_PRIVATE ( > > + TP_BASE_CLIENT_GET_CLASS (self), TP_TYPE_BASE_CLIENT, > > + TpBaseClientClassPrivate); > > Please avoid nonstandard abbreviations like "pv" where feasible (private things > are called private or priv). > > Can we cache this in FooClass.priv in class_init, like the way we cache object > private structures in Foo.priv in the instance init? Then you could use > cls->priv->observe_channels_impl(). Very good idea! Done. > > + "Implementation of ObserveChannels of %s didn't call " > > Nitpicking: "Implementation of ObserveChannels in %s" would avoid the repeated > "of" while remaining grammatically correct. changed. > > + * tp_observe_channels_context_delay: > > + * @self: a #TpObserveChannelsContext > > + * > > + * Called by #TpBaseClientClassObserveChannelsImpl to indicate that it > > + * implements the method in an async way. It has to ref the > > + * #TpObserveChannelsContext before calling this function and is responsible > > + * to call tp_observe_channels_context_accept once the call is done. > > I'd prefer: > > ... an async way. The caller must take a reference to the > #TpObserveChannelsContext before calling this function, and is responsible for > calling either tp_observe_channels_context_accept() or > tp_observe_channels_context_fail() later. changed. > If disposed in state NONE or DELAYED, the context should probably g_critical(), > and raise NotImplemented on D-Bus. done. > > + * If the "recovering" key in present in the Observer_Info hash table > > + * associated with this context, its value; %FALSE if the key is not present. > > + * > > + * Returns: value > > That describes the mechanism, but not the purpose. I'd prefer: > > * If this call to ObserveChannels is for channels that already > * existed before this observer started (because the observer used > * tp_base_client_set_observer_recover()), return %TRUE. > * > * In most cases, the result is %FALSE. > * > * Returns: %TRUE for pre-existing channels, %FALSE for new channels changed. > Perhaps ..._is_recovering() would be a better name? I don't know. > > > +_tp_observe_channels_context_prepare_async (TpObserveChannelsContext *self, > ... > > + g_return_if_fail (self->priv->result == NULL); > > This deserves a comment, something like: > > /* This is only used once, by TpBaseClient, so for simplicity, we only > * allow one asynchronous preparation */ added. > > Still to review > =============== > > I haven't reviewed the test or the SimpleClient yet. I guess you mean SimpleObserver :) I think that's ok, we can merge the Observer bits of BaseClient first. At least we know that it allows to easily write SimpleObserver. > Future work > =========== > > I think we should get Observers working nicely (including an example and > perhaps TpSimpleObserver) before we move on to the other classes. After that, > I'd recommend Handler next - Approver is harder, because it needs more API on > ChannelDispatchOperation. > > > +/** > > + * SECTION:base-client > > + * @title: TpBaseClient > > + * @short_description: base class for Telepathy clients on D-Bus > > + * > > + * This base class makes it easier to write #TpSvcClient > > + * implementations. Subclasses should usually pass the filters they > > + * want and override the D-Bus methods they implement. > > + */ > > Eventually, I'd like a code example or a reference to TpSimpleObserver (etc.) > here. > > It'd be nice to have an observer in examples/client/ that just does a > g_message() whenever a StreamedMedia call starts or finishes, or something > similarly easy. Would you implement it using TpBaseClient or TpSimpleObserver?
(In reply to comment #25) > > I wonder whether there should be a property and/or accessor for the client's > > bus name and object path? If so, they should be set during the constructor > > based on name and uniquify, and then used as-is at register time (i.e. move > > some code from register to constructor). > > Sounds like a good idea. I'll add that. I added tp_base_client_get_bus_name() and tp_base_client_get_object_path()
(In reply to comment #25) > > Eventually, I'd like a code example or a reference to TpSimpleObserver (etc.) > > here. > > > > It'd be nice to have an observer in examples/client/ that just does a > > g_message() whenever a StreamedMedia call starts or finishes, or something > > similarly easy. > > Would you implement it using TpBaseClient or TpSimpleObserver? What about referring as TpSimpleObserver as a good example of TpBaseClient subclass and implement the example using TpSimpleObserver ?
(In reply to comment #25) > > Still to review > > =============== > > > > I haven't reviewed the test or the SimpleClient yet. > > I guess you mean SimpleObserver :) No, I meant the SimpleClient used by the regression test :-P > I think that's ok, we can merge the Observer bits of BaseClient first. At least > we know that it allows to easily write SimpleObserver. Agreed. I'll re-review shortly. (In reply to comment #27) > (In reply to comment #25) > > > Eventually, I'd like a code example or a reference to TpSimpleObserver (etc.) > > > here. > > > > > > It'd be nice to have an observer in examples/client/ that just does a > > > g_message() whenever a StreamedMedia call starts or finishes, or something > > > similarly easy. > > > > Would you implement it using TpBaseClient or TpSimpleObserver? > > What about referring as TpSimpleObserver as a good example of TpBaseClient > subclass and implement the example using TpSimpleObserver ? That'd work. If it wasn't for all the boilerplate needed to implement a subclass, I'd say do the example for TpBaseClient and port to TpSimpleObserver afterwards... but then, if it wasn't for all the boilerplate needed to implement a subclass, we wouldn't need TpSimpleObserver at all.
I rebased the branch on top of master (to enjoy the new valgrind goodness) and pushed 2 more patches making test-base-client leak free.
I removed the #if 0 code and made sure that "make check" pass.
I pushed few more patches fixing a stupid bug and adding base-client to tp-glib.h
I still need to review the regression test (including SimpleClient) - I'll do that next. (In reply to comment #29) > I rebased the branch on top of master (to enjoy the new valgrind goodness) and > pushed 2 more patches making test-base-client leak free. I assume I've already seen everything up to "allow tp_base_client_register to fail". The changes up to and including "add base-client.h to telepathy-glib.h" look good, except for: > 61 * Signature of the implementation of the ObserveChannels method. > 62 * This function MUST call tp_observe_channels_context_accept, > 63 * tp_observe_channels_context_delay or tp_observe_channels_context_fail. This isn't an RFC (or telepathy-spec, which pretends to be one) so I'd be inclined to not capitalize MUST. How about this? * Signature of the implementation of the ObserveChannels method. * * This function must call either tp_observe_channels_context_accept, * tp_observe_channels_context_delay or tp_observe_channels_context_fail * on @context before it returns. > +/* FIXME: This function should be (skip) but that breaks gtk-doc <= 1.14 */ As of current master, we've decided to ignore that limitation and rely on having the necessary patch in gtk-doc (1.14-2 in Debian), so please (skip) this. > + DEBUG ("Failed to register bus name %s\n", string->str); Remove \n http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=8310b0e8bf0752604daae52c1cd8f50351a3752e > a I assume this was meant to be squashed into "base-client: remove Approver and Handler code for now" :-)
The regression test contains some dead code, but nothing serious. (in the test) > + test->dbus = test_dbus_daemon_dup_or_die (); > + g_assert (test->dbus != NULL); The assert here is pointless - the "or_die" suffix means "assert on failure". > + "dbus-connection", ((TpProxy *) test->dbus)->dbus_connection, Unnecessary - TpProxy gets this from the dbus-daemon if necessary. > + if (test->error != NULL) > + { > + g_error_free (test->error); > + test->error = NULL; > + } g_clear_error? > + if (test->interfaces != NULL) > + { > + g_strfreev (test->interfaces); > + test->interfaces = NULL; > + } g_strfreev is NULL-safe, just do this unconditionally. (in simple-client.[ch]) > + * Copyright (C) 2007-2008 Collabora Ltd. <http://www.collabora.co.uk/> 2007-2010 (and wjt will be happy if you use © rather than (C) in new files :-)
(In reply to comment #32) n everything up to "allow tp_base_client_register to > fail". The changes up to and including "add base-client.h to telepathy-glib.h" > look good, except for: > > > 61 * Signature of the implementation of the ObserveChannels method. > > 62 * This function MUST call tp_observe_channels_context_accept, > > 63 * tp_observe_channels_context_delay or tp_observe_channels_context_fail. > > This isn't an RFC (or telepathy-spec, which pretends to be one) so I'd be > inclined to not capitalize MUST. How about this? > > * Signature of the implementation of the ObserveChannels method. > * > * This function must call either tp_observe_channels_context_accept, > * tp_observe_channels_context_delay or tp_observe_channels_context_fail > * on @context before it returns. changed. > > +/* FIXME: This function should be (skip) but that breaks gtk-doc <= 1.14 */ > > As of current master, we've decided to ignore that limitation and rely on > having the necessary patch in gtk-doc (1.14-2 in Debian), so please (skip) > this. done. > > + DEBUG ("Failed to register bus name %s\n", string->str); > > Remove \n done. > http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=8310b0e8bf0752604daae52c1cd8f50351a3752e > > a > > I assume this was meant to be squashed into "base-client: remove Approver and > Handler code for now" :-) Indeed :) It is now.
(In reply to comment #33) > The regression test contains some dead code, but nothing serious. > > (in the test) > > + test->dbus = test_dbus_daemon_dup_or_die (); > > + g_assert (test->dbus != NULL); > > The assert here is pointless - the "or_die" suffix means "assert on failure". removed. > > + "dbus-connection", ((TpProxy *) test->dbus)->dbus_connection, > > Unnecessary - TpProxy gets this from the dbus-daemon if necessary. removed. > > + if (test->error != NULL) > > + { > > + g_error_free (test->error); > > + test->error = NULL; > > + } > > g_clear_error? I always forget about this function. Fixed. > > + if (test->interfaces != NULL) > > + { > > + g_strfreev (test->interfaces); > > + test->interfaces = NULL; > > + } > > g_strfreev is NULL-safe, just do this unconditionally. done. > (in simple-client.[ch]) > > + * Copyright (C) 2007-2008 Collabora Ltd. <http://www.collabora.co.uk/> > > 2007-2010 (and wjt will be happy if you use © rather than (C) in new files :-) done.
(In reply to comment #34) > > > 61 * Signature of the implementation of the ObserveChannels method. > > > 62 * This function MUST call tp_observe_channels_context_accept, > > > 63 * tp_observe_channels_context_delay or tp_observe_channels_context_fail. > > > > This isn't an RFC (or telepathy-spec, which pretends to be one) so I'd be > > inclined to not capitalize MUST. How about this? > > > > * Signature of the implementation of the ObserveChannels method. > > * > > * This function must call either tp_observe_channels_context_accept, > > * tp_observe_channels_context_delay or tp_observe_channels_context_fail > > * on @context before it returns. > > changed. I don't see this in the branch, did you forget to push it? > > > + DEBUG ("Failed to register bus name %s\n", string->str); > > > > Remove \n > > done. I don't see this either? I'll clone this bug for TpSimpleObserver, and for Handler and Approver support.
(In reply to comment #36) > (In reply to comment #34) > > > > 61 * Signature of the implementation of the ObserveChannels method. > > > > 62 * This function MUST call tp_observe_channels_context_accept, > > > > 63 * tp_observe_channels_context_delay or tp_observe_channels_context_fail. > > > > > > This isn't an RFC (or telepathy-spec, which pretends to be one) so I'd be > > > inclined to not capitalize MUST. How about this? > > > > > > * Signature of the implementation of the ObserveChannels method. > > > * > > > * This function must call either tp_observe_channels_context_accept, > > > * tp_observe_channels_context_delay or tp_observe_channels_context_fail > > > * on @context before it returns. > > > > changed. > > I don't see this in the branch, did you forget to push it? > > > > > + DEBUG ("Failed to register bus name %s\n", string->str); > > > > > > Remove \n > > > > done. > > I don't see this either? Humm weird; I'm sure I fixed this; I guess I forgot to commit or lost the patches while jungling with branches. Anyway, it's pushed now.
I've pushed a (non-trivial) merge to 'master' in my personal repository, to make it easier to do a final review pass; feel free to base further work on that merge. I did spot a couple of problems on that review pass, though: > + if (self->priv->uniquify_name) > + { > + const gchar *unique; > + > + unique = tp_dbus_daemon_get_unique_name (self->priv->dbus); > + g_string_append_printf (string, ".%s", tp_escape_as_identifier (unique)); > + g_string_append_printf (string, ".n%u", unique_counter++); > + } This leaks tp_escape_as_identifier (unique). You'll need to use a temporary variable and free it afterwards. (While you're fixing that, you might as well combine the two calls to g_string_append_printf.) > + /** > + * TpObserveChannelsContext:dbus-context: > + * I think this should be annotated as (skip), and write-only. It's not useful for user code to look at, once the object has been constructed. Introspection ============= We should also get this introspectable (do this as a separate branch) by putting it in the list of introspectable headers in introspect.am, and sanity-checking the .gir output.
(In reply to comment #37) > jungling with branches I think you might have accidentally the next release name :-)
Done. I also changed TpBaseClient to be an abstract type.
Merged; will be in 0.11.5. Thanks a lot for your review.
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.