Bug 55104

Created attachment 94833 [details] [review]
turn TpCallChannel:state-details to a GVariant

Created attachment 94834 [details] [review]
TpFileTransferChannel: store metadata as a GVariant

Not directly related to this bug but while I'm on it...

Comment on attachment 94833 [details] [review]
turn TpCallChannel:state-details to a GVariant

Review of attachment 94833 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/call-channel.c
@@ +409,5 @@
>  
>    self->priv->state = state;
>    self->priv->flags = flags;
>    self->priv->state_reason = _tp_call_state_reason_new (reason);
> +  self->priv->state_details = tp_asv_to_vardict (details);

Floating ref needs sinking now

@@ +594,4 @@
>        "CallState", NULL);
>    self->priv->flags = tp_asv_get_uint32 (properties,
>        "CallFlags", NULL);
> +  self->priv->state_details = tp_asv_to_vardict (tp_asv_get_boxed (properties,

Also here

@@ +905,5 @@
>    /**
>     * TpCallChannel:state-details:
>     *
> +   * Detailed information about #TpCallChannel:state. It is a #GVariant
> +   * if type #G_VARIANT_TYPE_VARDICT.

s/if/of/ (but at least you fixed "infoermation")

@@ +1092,5 @@
>     * @state: the new #TpCallState
>     * @flags: the new #TpCallFlags
>     * @reason: the #TpCallStateReason for the change
> +   * @details: additional details as a
> +   *   #GVariant of type #G_VARIANT_TYPE_VARDICT readable.

I think the word "readable" is redundant here. It was part of the hint pointing users towards the tp_asv_* family.

@@ +1208,4 @@
>   * @self: a #TpCallChannel
>   * @flags: (out) (allow-none) (transfer none): a place to set the value of
>   *  #TpCallChannel:flags
> + * @details: (out) (allow-none) (transfer full): a place to set the value of

It seems inconsistent that this is (transfer full) but the TpCallStateReason is (transfer none). The _get_ naming hints API users towards (transfer none), I think.

Any thoughts on whether to squash the variant, or maybe even the state and flags too, into the TpCallStateReason (with a rename to ...StateDetails or something if it gains the state and flags)?

I would like to at least add some ABI padding to the TpCallStateReason.

Comment on attachment 94834 [details] [review]
TpFileTransferChannel: store metadata as a GVariant

Review of attachment 94834 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/file-transfer-channel.c
@@ +614,5 @@
>       TP_PROP_CHANNEL_INTERFACE_FILE_TRANSFER_METADATA1_METADATA,
>       TP_HASH_TYPE_METADATA);
>  
> +  if (metadata == NULL)
> +    self->priv->metadata = g_variant_new ("a{ss}", NULL);

a{sas}. Also, please sink the floating ref.

@@ +619,3 @@
>    else
> +    self->priv->metadata = _tp_boxed_to_variant (TP_HASH_TYPE_METADATA,
> +        "a{sas}", metadata);

Sink the floating ref. (You could add one unconditional line that sinks both, maybe.)

@@ +1004,2 @@
>        "Metadata",
>        "The Metadata.Metadata property of this channel",

I'd prefer "... as a variant of type a{sas}, i.e. map with string keys and array-of-strings values" or something, unless gtk-doc already has special knowledge about g_param_spec_variant()'s 4th argument (and perhaps even if it does).

@@ +1649,1 @@
>   *   value of the #TpFileTransferChannel:metadata property

I'd prefer the type to be explicit here too.

(In reply to comment #3)
> Comment on attachment 94833 [details] [review] [review]
> turn TpCallChannel:state-details to a GVariant
> 
> Review of attachment 94833 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: telepathy-glib/call-channel.c
> @@ +409,5 @@
> >  
> >    self->priv->state = state;
> >    self->priv->flags = flags;
> >    self->priv->state_reason = _tp_call_state_reason_new (reason);
> > +  self->priv->state_details = tp_asv_to_vardict (details);
> 
> Floating ref needs sinking now

Oh yeah, I forgot to update this branch.
Done.

> @@ +594,4 @@
> >        "CallState", NULL);
> >    self->priv->flags = tp_asv_get_uint32 (properties,
> >        "CallFlags", NULL);
> > +  self->priv->state_details = tp_asv_to_vardict (tp_asv_get_boxed (properties,
> 
> Also here

Done.

> @@ +905,5 @@
> >    /**
> >     * TpCallChannel:state-details:
> >     *
> > +   * Detailed information about #TpCallChannel:state. It is a #GVariant
> > +   * if type #G_VARIANT_TYPE_VARDICT.
> 
> s/if/of/ (but at least you fixed "infoermation")

fixed.

> @@ +1092,5 @@
> >     * @state: the new #TpCallState
> >     * @flags: the new #TpCallFlags
> >     * @reason: the #TpCallStateReason for the change
> > +   * @details: additional details as a
> > +   *   #GVariant of type #G_VARIANT_TYPE_VARDICT readable.
> 
> I think the word "readable" is redundant here. It was part of the hint
> pointing users towards the tp_asv_* family.

removed.

> @@ +1208,4 @@
> >   * @self: a #TpCallChannel
> >   * @flags: (out) (allow-none) (transfer none): a place to set the value of
> >   *  #TpCallChannel:flags
> > + * @details: (out) (allow-none) (transfer full): a place to set the value of
> 
> It seems inconsistent that this is (transfer full) but the TpCallStateReason
> is (transfer none). The _get_ naming hints API users towards (transfer
> none), I think.
> 
> Any thoughts on whether to squash the variant, or maybe even the state and
> flags too, into the TpCallStateReason (with a rename to ...StateDetails or
> something if it gains the state and flags)?

If we're fine with loosing the coupling with http://telepathy.freedesktop.org/spec/Channel_Type_Call.html#Property:CallStateReason yeah I think that's fine.

> I would like to at least add some ABI padding to the TpCallStateReason.

Done.

(In reply to comment #4)
> Comment on attachment 94834 [details] [review] [review]
> TpFileTransferChannel: store metadata as a GVariant
> 
> Review of attachment 94834 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: telepathy-glib/file-transfer-channel.c
> @@ +614,5 @@
> >       TP_PROP_CHANNEL_INTERFACE_FILE_TRANSFER_METADATA1_METADATA,
> >       TP_HASH_TYPE_METADATA);
> >  
> > +  if (metadata == NULL)
> > +    self->priv->metadata = g_variant_new ("a{ss}", NULL);
> 
> a{sas}. Also, please sink the floating ref.

fixed.

> @@ +619,3 @@
> >    else
> > +    self->priv->metadata = _tp_boxed_to_variant (TP_HASH_TYPE_METADATA,
> > +        "a{sas}", metadata);
> 
> Sink the floating ref. (You could add one unconditional line that sinks
> both, maybe.)

done.

> @@ +1004,2 @@
> >        "Metadata",
> >        "The Metadata.Metadata property of this channel",
> 
> I'd prefer "... as a variant of type a{sas}, i.e. map with string keys and
> array-of-strings values" or something, unless gtk-doc already has special
> knowledge about g_param_spec_variant()'s 4th argument (and perhaps even if
> it does).

done.

> @@ +1649,1 @@
> >   *   value of the #TpFileTransferChannel:metadata property
> 
> I'd prefer the type to be explicit here too.

done.

http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-call-55104 updated

(In reply to comment #5)
> > Any thoughts on whether to squash the variant, or maybe even the state and
> > flags too, into the TpCallStateReason (with a rename to ...StateDetails or
> > something if it gains the state and flags)?
> 
> If we're fine with loosing the coupling with
> http://telepathy.freedesktop.org/spec/Channel_Type_Call.html#Property:
> CallStateReason yeah I think that's fine.

(Not done in the branch I reviewed.)

CallStateReason appears to be used for both CallStateChanged (where it's emitted alongside the state, flags and a{sv} details), and ContentRemoved (where there are no further details).

I started to write "I wonder whether we should have individual getters for all the bits and pieces?" but that would be a really annoying API. I think state and flags as individual getters make sense, but the various reason codes should really be bundled together, so advanced UIs can do the usual Telepathy "string-match D-Bus errors, fall back to switch() on error codes" dance.

I think normally we'd have a getter with multiple "out" values, but then we'd have difficulty putting it in a signal.

I notice with annoyance that we have both Call_State_Reason.Message and CallStateDetails["debug-message"], which appear to be 100% redundant - in 1.0 we should drop one of them, and if the one we keep is Call_State_Reason.Message, put a note in CallStateDetails that says "note that maps like this in Telepathy usually have a "debug-message" key, but in Call1 the same information goes in Call_State_Reason.Message".

I wonder whether it would make most sense to turn Call_State_Reason from a (uuss) to a (uussa{sv}) (or perhaps (uusas{sv}) if we keep "debug-message" but not Message), so we're able to have extensible details in CallMembersChanged and ContentRemoved, not just in CallStateChanged?

I also think that if this thing is going to be C API, we should expose its ref and unref functions.

> > I would like to at least add some ABI padding to the TpCallStateReason.
> 
> Done.

Looks good for merge, while we think about the issue above.

(In reply to comment #7)
> I wonder whether it would make most sense to turn Call_State_Reason from a
> (uuss) to a (uussa{sv}) (or perhaps (uusas{sv}) if we keep "debug-message"
> but not Message)

That last type should be (uusa{sv}) of course.

(In reply to comment #7)
> Looks good for merge, while we think about the issue above.

Merged but let's keep this bug open then.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-glib/issues/103.