[Bug 55104] Provide GVariant-based access to TpCallChannel:state-details

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=55104

--- Comment #5 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> ---
(In reply to comment #3)
> 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

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.