[Telepathy] Review of Will's telepathy-glib requestotron branch

[Simon McVittie]

Reviewed asynchronously due to Linux Plumbers Conference travel - greetings
from the skies over, er, Minnesota, I think? Obviously, the actual
transmission of this email will be (or was, from your perspective) somewhat
later...

I reviewed up to commit afa0599419bfd12b2663339673d469da37052623 (I'd have
looked for a newer version, but Three connectivity in Heathrow airport
seems to fail).

Things marked [trivia] or [later] are not necessarily merge or release
blockers, given the importance of getting the requestotron stuff moving, but
should be done sometime (for [trivia] just do them in a trivia branch,
or convince me I'm wrong; for [later] file a bug). That covers most of it,
in fact.

signals-marshal.list
====================

To simplify merges, in files like this one where order is totally
irrelevant, please use asciibetical order (vim: :1,$!LC_ALL=C sort) -
any consistent order is fine, I just picked this one arbitrarily

gtypes docs
===========

Please merge the Requests Private subsection containing the underlying
functions (tp_type_dbus_array_a_7bsv_7das and friends) into the existing
subsection containing the other similar functions. The public #defines
like TP_HASH_TYPE_CHANNEL_CLASS are conceptually part of the Requests
interface, so giving them their own subsection is good, but the underlying
functions are not specific to any interface, and so should all go together
in LC_COLLATE=C order (also, this simplifies merges).

base-connection.c
=================

[trivia] I'd be inclined to fold the copyright years into a range
(Copyright (C) 2005-2008...), it's starting to get quite stupid
otherwise :-)

[trivia] While you're touching the <telepathy-glib/*> headers at the
top anyway, please sort them ASCIIbetically.

[trivia] The addition of requests_iface_init() means that
service_iface_init should probably be renamed to conn_iface_init (like
it should have been all along, but I was young and naive).

[trivia] To make the intention completely clear, get_channel_details()
should probably use TP_HASH_TYPE_QUALIFIED_PROPERTY_VALUE_MAP rather
than TP_HASH_TYPE_STRING_VARIANT_MAP, now we have it.

[trivia] get_channel_details() should allocate structure to be of length
2, since it always gets 2 members :-)

satisfy_requests() is a bit confusingly named - it's only called for
TpChannelFactory channel creations. Rename it to factory_satisfy_requests
or something? (satisfy_request (singular), on the other hand, is fine!)

Similarly channel_closed_cb, connection_new_channel_cb,
connection_channel_error_cb should probably be factory_channel_closed_cb,
factory_new_channel_cb, factory_channel_error_cb

[later?] tp_base_connection_close_all_channels: please discuss with
Rob whether we want to give channel managers a close_all callback, or
add a signal disconnect-imminent(guint reason), to give them a chance
to emit Closed (or perhaps a Group change, if that's more meaningful)
just before status-changed goes out. If we do want such a callback,
we can add it as part of the EnsureChannel round of changes, though,
so this isn't a release blocker IMO. Also, TpChannel will do the
right thing even if the Channel Closed signal never comes (it'll become
invalidated with an error in domain TP_ERRORS_DISCONNECTED whose code is
a TpConnectionStatusChangedReason, I believe - and that's at least as
informative as anything the Channel could come up with on its own!) so
perhaps we don't need that.

RETURN_INVALID_ARGUMENT should be of the form do {...} while (0), or
better G_STMT_START {...} G_STMT_END (or whatever they're really called
- I can never remember), so that compilers in ultra-pedantic mode won't
complain about "RETURN_INVALID_ARGUMENT(...);", i.e. "{...};", which is
a block "{...}" followed by the useless null statement ";"

conn_requests_check_basic_properties should require that TargetHandle
is nonzero if present, so the complaint about TargetHandle should say
"in range 1 to 2**32-1"

conn_requests_requestotron_validate_handle: I think it's worth having
separate error returns (with different messages) for "when requesting
a TargetHandleType, you must also specify either a TargetHandle or a
TargetID" and "TargetHandle and TargetID must not both be given"

c_r_r_validate_handle: tp_handle_ensure (usually? always?) raises
NotAvailable on error, but CreateChannel is specified to raise
InvalidHandle (slightly bending the usual semantics) if the TargetID is
inappropriate - either propose a small spec patch, or transform the error
(you can just alter error->code in-place, I think)

conn_requests_offer_request: note for the future that the behaviour of
having suppress_handler = FALSE for Ensure requests is debatable - either
think about this some more, or put a FIXME comment in your Ensure branch

[trivia] in conn_requests_offer_request I'd exchange "request->context =
NULL" with the preceding blank line, to make it more obvious that the
async return is exactly the reason that you may (and must) do this

[trivia] end of tp_base_connection_channel_manager_iter_next: the style that
daf and I have been encouraging would put a blank line between the if-block
and the line "iter->index++"

[trivia] channel-manager.h, exportable-channel.h: no gtkdoc in headers
in new code please (to reduce recompilations and -dev package size) -
move them to the .c

[trivia/later] base-connection.h: moving gtkdoc to the .c (to reduce
recompilations and -dev package size) is encouraged, feel free to do
this at any convenient time

TpBaseConnectionClass struct docs: Maybe it's too early to document
create_channel_factories as deprecated - "In new code, setting this to
NULL is recommended" is a weaker form? We certainly don't want anyone
trying to make it G_GNUC_DEPRECATED!

Similarly, it may be too early to deprecate TpChannelFactory.

Any thoughts on a better (than POINTER) GType for a
GHashTable<TpExportableChannel, GSList<request token> > as per the FIXME
I put in exportable-channel.c? We can't dup request tokens, though (the
TpChannelManager semantics are that the manager MUST forget about all requests
that it has mentioned in a signal) so trying to invent a boxed type may just
be an exercise in futility

[later] please enable Requests in the echo CM (the only example CM with
proper channels, currently) so there's a minimal example

TpChannelManagerRequestFunc: if TargetHandleType is None, TargetHandle
is omitted

TpChannelManagerIface doc: "thirteen GCallback fields" is in fact a lie,
there are "only" 11 - but you might as well just say "several" tbh

TpChannelManagerIface _near_future: perhaps replace
with GCallback _reserved_for_ensure_channel, GCallback
_reserved_for_foreach_contact_channel_class, and whatever the other caps
function is?

tp_dbus_properties_mixin_make_properties_hash: When termination
is reached (i.e. interface is NULL), property points sizeof(char*)
bytes past interface, to some misc bit of stack (in practice, most
likely to be platform calling convention internals like the saved IP,
which makes my security brain cell itch, even though it's clearly not a
problem when this function is used correctly). I suggest you move the
update of property to the loop body. Even if you overrule me on that,
in the loop body you need to assert (or at least return_val_if_fail)
that property != NULL, to avoid the potential for an an off-by-one
(accidentally concatenating two of the strings by omitting the comma,
perhaps) which would cause us to miss the NULL terminator entirely.

General
=======

After merging this lot, please write an interim NEWS entry noting that
tp-glib claims to implement spec 0.7.11, with initial Requests support
(CreateChannel but not EnsureChannel) - it's probably worth mentioning
those functions by name in NEWS. At that point it's good for a release
whenever, I think, but please let me review the release preparation - I
did all the previous releases, and the process is probably rather subtle
(and/or insane).

See you,
    Simon