Bug 75204

This branch is getting pretty big so I'm posting it already even if it's not finished yet:
http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-vardict-75204

Don't be too afraid by its size, each commit is well self contained so we can review/merge it partially.

First 4 look good.

-static GHashTable *
-create_request (void)
+static GVariantDict
+dict_request (void)
{
- return tp_asv_new (
- TP_PROP_CHANNEL_CHANNEL_TYPE, G_TYPE_STRING, TP_IFACE_CHANNEL_TYPE_TEXT,
- TP_PROP_CHANNEL_TARGET_ENTITY_TYPE, G_TYPE_UINT,
- TP_ENTITY_TYPE_CONTACT,
- TP_PROP_CHANNEL_TARGET_ID, G_TYPE_STRING, "alice",
- NULL);
+ GVariantDict dict;
+
+ g_variant_dict_init (&dict, NULL);
+
+ g_variant_dict_insert (&dict,
+ TP_PROP_CHANNEL_CHANNEL_TYPE, "s", TP_IFACE_CHANNEL_TYPE_TEXT);
+ g_variant_dict_insert (&dict,
+ TP_PROP_CHANNEL_TARGET_ENTITY_TYPE, "u", TP_ENTITY_TYPE_CONTACT);
+ g_variant_dict_insert (&dict,
+ TP_PROP_CHANNEL_TARGET_ID, "s", "alice");
+
+ return dict;
}

This is undefined behaviour. You're returning a pointer to stack contents which are no longer guaranteed not to be scribbled on after this function returns.

Please do something like this:

    /* @dict is uninitialized on entry. */
    static void
    init_dict_request (GVariantDict *dict)
    {
      g_variant_dict_init (dict, NULL);
      ...
    }

    caller
    {
      GVariantDict dict;

      init_dict_request (&dict);
    }

or this:

    /* @dict must already be initialized on entry. */
    static void
    fill_dict_request (GVariantDict *dict)
    {
    }

    caller
    {
      GVariantDict dict;

      g_variant_dict_init (&dict, NULL);
      fill_dict_request (&dict);
    }

> base-client: remove not vardict filter API
>
> take_*_filter() are still used to implement the vardict API for now so they
> are now static.

Please replace /** with /* when you make public functions static.

> +static const gchar *
>  tp_connection_get_detailed_error (TpConnection *self,

Same here

+ * When constructing a new object, #TpAccountChannelRequest:request
+ * must be set to a non-%NULL value, and the other must remain unspecified.

The part after the second comma can go away now :-)

Looks OK from a skim-read (up to "tp_account_manager_create_account_async: use GVariant"), but I'll need to re-review properly to check for floating ref correctness etc.

I don't think you should do much (any?) more than this in telepathy-glib without catching up in API users, particularly MC, Empathy, and Empathy's account widgets submodule.

I did not review the whole branch, just wondering what would be the best API for TpAccountChannelRequest, and probably other similar helpers like TpAccountRequest.

I was expecting TpAccountChannelRequest to replace its internal GHashTable by a GVariantDict. What Guillaume's branch does is:
1) Create a GVariantDict for the request (which is basically a GHashTable<string, GVariant>).
2) Create a GVariant from the GVariantDict
3) create a TpAccountChannelRequest which converts the GVariant into a GHashTable<string, GVariant>

So that means a double conversion. I don't think performance really matters here, so I wouldn't object, but maybe we can go one step further?

What I would suggest:

 - TpAccountChannelRequest->priv->request should be a GVariantDict.

 - tp_account_channel_request_new()'s request arg should be a GVariantDict that we simply ref to become priv->request. Or even better: remove that arg and caller can just first create an empty TpAccountChannelRequest and then call _set_request_property().

 - _set_request_property() becomes a trivial g_variant_dict_insert_value (self->priv->request, key, variant);

 - We convert the internal GVariantDict to a GVariant only when we are actually doing the channel request. At that point the TpAccountChannelRequest becomes invalide and shouldn't be used anymore. That's already the case I think but our API doesn't enforce it.

 - Similar change for the hints

 - There is one issue with tp_account_channel_request_dup_request() because that means we would have to call g_variant_dict_end() to return the GVariant but that invalidates it... But that function is only used for unit tests, I see no other reason to need it. Asked Ryan and he said we could add a g_variant_dict_get_value(), or g_variant_new_dict(), but he's not really enthousiast with it neither.

(In reply to comment #2)
> First 4 look good.

Merged those already.

> This is undefined behaviour. You're returning a pointer to stack contents
> which are no longer guaranteed not to be scribbled on after this function
> returns.

Good point. I added a !fixup commit.

(In reply to comment #3)
> > base-client: remove not vardict filter API
> >
> > take_*_filter() are still used to implement the vardict API for now so they
> > are now static.
> 
> Please replace /** with /* when you make public functions static.

Done. I changed only those who are still static, not the one I turned to a static function as an intermediary step to make commits smaller but which are finally removed.

(In reply to comment #5)
> + * When constructing a new object, #TpAccountChannelRequest:request
> + * must be set to a non-%NULL value, and the other must remain unspecified.
> 
> The part after the second comma can go away now :-)

Fixed.

> I don't think you should do much (any?) more than this in telepathy-glib
> without catching up in API users, particularly MC, Empathy, and Empathy's
> account widgets submodule.

I pushed a few extra tube channels patches but yeah I'm going to start porting things now.

(In reply to comment #5)
> I don't think you should do much (any?) more than this in telepathy-glib
> without catching up in API users, particularly MC, Empathy, and Empathy's
> account widgets submodule.

http://cgit.collabora.com/git/user/cassidy/telepathy-account-widgets.git/log/?h=next-vardict
http://cgit.collabora.com/git/user/cassidy/empathy/log/?h=next-vardict

http://cgit.collabora.com/git/user/cassidy/telepathy-mission-control/log/?h=next-vardict
http://cgit.collabora.com/git/user/cassidy/telepathy-gabble/log/?h=next-vardict
http://cgit.collabora.com/git/user/cassidy/telepathy-idle/log/?h=next-vardict
http://cgit.collabora.com/git/user/cassidy/telepathy-haze/log/?h=next-vardict

review- for TpAW:

> + v = g_variant_lookup_value (account_params, param,
> + G_VARIANT_TYPE_VARIANT);

The docs say:

"""
In the event that dictionary has the type a{sv}, the expected_type string specifies what type of value is expected to be inside of the variant. If the value inside the variant has a different type then NULL is returned.
"""

so I would expect that call to be this pseudocode:

    v_ = account_params[param]
    if v_.signature == 'v':
        v = v_
    else:
        v = NULL

and it seems that is in fact the case:

>>> from gi.repository import GLib
>>> asv = GLib.Variant.parse(GLib.VariantType.new('a{sv}'), '@a{sv} { "hello": <"world">, "nested": <<42>> }', None, None)
>>> print asv.lookup_value('hello', GLib.VariantType.new('v'))
None
>>> print asv.lookup_value('nested', GLib.VariantType.new('v'))
<42>

So I think you're going to have to use GVariantDict, or iteration.

Empathy, first commit:

+ g_variant_dict_init (&dict, NULL);
+ g_variant_dict_insert (&dict, TP_PROP_CHANNEL_CHANNEL_TYPE, "s",
+ TP_IFACE_CHANNEL_TYPE_TEXT);
+ g_variant_dict_insert (&dict, TP_PROP_CHANNEL_TARGET_ENTITY_TYPE, "u",
+ TP_ENTITY_TYPE_CONTACT);
+ g_variant_dict_insert (&dict, TP_PROP_CHANNEL_TARGET_ID, "s", contact_id);
- req = tp_account_channel_request_new (priv->account, request,
- empathy_get_current_action_time ());
+ req = tp_account_channel_request_new (priv->account,
+ g_variant_dict_end (&dict), empathy_get_current_action_time ());

High-level API, please:

    req = tp_account_channel_request_new_text (priv->account,
        empathy_get_current_action_time ());
    tp_account_channel_request_set_target_id (priv->account,
        TP_ENTITY_TYPE_CONTACT, contact_id);

I'd prefer to use tp_account_channel_request_new_audio_[video_]call() in empathy_call_create_call_request(), too, but that function currently copes with being called with initial_audio=False and we don't have high-level API for that. (Does Empathy ever actually do that?)

> empathy_share_my_desktop_share_with_contact

I thought we had high-level API for this too, but apparently not. Maybe we should. However, you could at least replace these:

+ g_variant_dict_insert (&request, TP_PROP_CHANNEL_TARGET_ENTITY_TYPE, "u",
+ TP_ENTITY_TYPE_CONTACT);
+ g_variant_dict_insert (&request, TP_PROP_CHANNEL_TARGET_HANDLE, "u",
+ tp_contact_get_handle (tp_contact));

with a call to tp_account_channel_request_set_target_contact() after the request has been created?

+ priv->request = g_variant_dict_new (NULL);
+ g_variant_dict_insert (priv->request, TP_PROP_CHANNEL_CHANNEL_TYPE,
+ "s", TP_IFACE_CHANNEL_TYPE_FILE_TRANSFER1);
+ g_variant_dict_insert (priv->request, TP_PROP_CHANNEL_TARGET_ENTITY_TYPE,
+ "u", TP_ENTITY_TYPE_CONTACT);
+ g_variant_dict_insert (priv->request, TP_PROP_CHANNEL_TARGET_HANDLE,
+ "u", contact_handle);
+ g_variant_dict_insert (priv->request,
+ TP_PROP_CHANNEL_TYPE_FILE_TRANSFER1_CONTENT_TYPE,
+ "s", priv->content_type);
+ g_variant_dict_insert (priv->request,
+ TP_PROP_CHANNEL_TYPE_FILE_TRANSFER1_FILENAME, "s", priv->filename);
+ g_variant_dict_insert (priv->request,
+ TP_PROP_CHANNEL_TYPE_FILE_TRANSFER1_SIZE, "t", priv->total_bytes);
+ g_variant_dict_insert (priv->request,
+ TP_PROP_CHANNEL_TYPE_FILE_TRANSFER1_DATE, "t", priv->mtime);
+ g_variant_dict_insert (priv->request, TP_PROP_CHANNEL_TYPE_FILE_TRANSFER1_URI,
+ "s", uri);

tp_account_channel_request_new_file_transfer() + tp_account_channel_request_set_target_contact() + tp_account_channel_request_set_file_transfer_timestamp() + tp_account_channel_request_set_file_transfer_uri() covers all of this.

The hash stuff doesn't have high-level API but could use tp_account_channel_request_set_request_property().

create_text_channel(): similar, please.

empathy_tp_chat_add(): the same.

Empathy, second commit:

+ properties = g_variant_new_parsed ("{ %s: <%b> }",
+ TP_IFACE_ACCOUNT ".Enabled", data->enabled);
+ settings = tp_asv_to_vardict (data->settings);

Not sure why Empathy feels the need to force this... leave it in, but we should spec that newly-created accounts are Enabled by default. Anything else would be silly.

Other than those, Empathy looks OK.

(In reply to comment #9)
> review- for TpAW:
> 
> > + v = g_variant_lookup_value (account_params, param,
> > + G_VARIANT_TYPE_VARIANT);

I think mc-tool has the same bug.

CMs look fine.

(In reply to comment #9)
> So I think you're going to have to use GVariantDict, or iteration.

Good catch! Fixed in http://cgit.collabora.com/git/user/cassidy/telepathy-account-widgets.git/log/?h=next-vardict(In reply to comment #12)

> (In reply to comment #9)
> I think mc-tool has the same bug.

Indeed, fixed: http://cgit.collabora.com/git/user/cassidy/telepathy-mission-control/log/?h=next-vardict

> CMs look fine.

Is the tp-glib branch good to go as well? I should merge this one first.

(In reply to comment #10)
> High-level API, please:

Ok, let's do this in master first then: https://bugzilla.gnome.org/show_bug.cgi?id=725070

I opened bug #75450 about the missing API and made some suggestions there.

(In reply to comment #9)
> >>> from gi.repository import GLib
> >>> asv = GLib.Variant.parse(GLib.VariantType.new('a{sv}'), '@a{sv} { "hello": <"world">, "nested": <<42>> }', None, None)
> >>> print asv.lookup_value('hello', GLib.VariantType.new('v'))
> None
> >>> print asv.lookup_value('nested', GLib.VariantType.new('v'))
> <42>
> 
> So I think you're going to have to use GVariantDict, or iteration.

GVariantDict won't change anything.

>>> asv.lookup_value('hello')
GLib.Variant('s', 'world')
>>> asv.lookup_value('hello', GLib.VariantType.new('s'))
GLib.Variant('s', 'world')

Also tp_account_dup_parameters_vardict()'s doc explicitly says to use g_variant_lookup() or g_variant_lookup_value(). So the doc would have to be fixed if Simon is correct.

http://cgit.collabora.com/git/user/cassidy/telepathy-mission-control/commit/?h=next-vardict&id=c9572997a31ed66c4a1236d09ac0f80369d2c494

It leaks because g_variant_dict_clear() is missing. It also uselessly create a GVariantDict where a simple g_variant_lookup_value(parameters, NULL) does the exact same job.

http://cgit.collabora.com/git/user/cassidy/telepathy-account-widgets.git/commit/?h=next-vardict&id=f95e22637d626811729b3ddc629d0f89a2562bb4

Same here, it it useless to create a whole new GVariantDict. A better fix is a simple s/G_VARIANT_TYPE_VARIANT/NULL/

(In reply to comment #16)
> Same here, it it useless to create a whole new GVariantDict. A better fix is
> a simple s/G_VARIANT_TYPE_VARIANT/NULL/

Ah, I missed that that was possible. If it means "return that value whatever its contained type is", then yes that's a better solution.

Good catch!

I updated both branches:
http://cgit.collabora.com/git/user/cassidy/telepathy-mission-control/log/?h=next-vardict
http://cgit.collabora.com/git/user/cassidy/telepathy-account-widgets.git/log/?h=next-vardict

+1 for both fixup. Didn't read the rest of the branch.

I'd like to have a second look through the telepathy-glib branch, I'll try to get to that tomorrow.

See also Bug #49505 (not a blocker), Bug #55104, Bug #55105, Bug #55106, Bug #55107, Bug #55108, Bug #55109.

examples: use tp_base_client_*_vardict API
+++ b/examples/client/text-handler.c
+ g_variant_dict_insert (&dict, TP_PROP_CHANNEL_TARGET_ENTITY_TYPE, "u",
+ TP_ENTITY_TYPE_CONTACT);

Maybe this should be (guint32) TP_ENTITY_TYPE_CONTACT for obvious correctness.

(However, ISO C says the default promotions promote smaller-than-int integers to int in function calls, so as long as sizeof(enum) <= sizeof(int) and sizeof(int32) <= sizeof(int), everything is fine.)

tp-fs example: use tp_base_client_add_handler_filter_vardict()

Is it deliberate that this example no longer catches video-only calls, and more seriously, no longer tells CMs to make us video-callable?

remove tp_connection_get_detailed_error()

@@ -235,11 +235,13 @@ test_unregistered_error (Test *test,

+ str = tp_connection_dup_detailed_error_vardict (test->conn, NULL);
+ g_assert_cmpstr (str, ==, "net.example.WTF");
+ g_free (str);
+ str = tp_connection_dup_detailed_error_vardict (test->conn, &asv);
+ g_assert_cmpstr (str, ==, "net.example.WTF");
  g_assert (asv != NULL);
+ g_variant_unref (asv);

The second time, str is leaked.

(In reply to comment #20)
> I'd like to have a second look through the telepathy-glib branche

Done, see my last 3 comments. Nothing particularly serious.

(In reply to comment #22)
> Maybe this should be (guint32) TP_ENTITY_TYPE_CONTACT for obvious
> correctness.

done.

(In reply to comment #23)
> tp-fs example: use tp_base_client_add_handler_filter_vardict()
> 
> Is it deliberate that this example no longer catches video-only calls, and
> more seriously, no longer tells CMs to make us video-callable?

It's not; fixed.

(In reply to comment #24)
> remove tp_connection_get_detailed_error()
> 
> @@ -235,11 +235,13 @@ test_unregistered_error (Test *test,
> 
> + str = tp_connection_dup_detailed_error_vardict (test->conn, NULL);
> + g_assert_cmpstr (str, ==, "net.example.WTF");
> + g_free (str);
> + str = tp_connection_dup_detailed_error_vardict (test->conn, &asv);
> + g_assert_cmpstr (str, ==, "net.example.WTF");
>   g_assert (asv != NULL);
> + g_variant_unref (asv);
> 
> The second time, str is leaked.

Fixed.

I merged a rebased version with all the fixup merged.

(In reply to comment #8)
> (In reply to comment #5)
> > I don't think you should do much (any?) more than this in telepathy-glib
> > without catching up in API users, particularly MC, Empathy, and Empathy's
> > account widgets submodule.
> 
> http://cgit.collabora.com/git/user/cassidy/telepathy-account-widgets.git/log/
> ?h=next-vardict
> http://cgit.collabora.com/git/user/cassidy/empathy/log/?h=next-vardict
> 
> http://cgit.collabora.com/git/user/cassidy/telepathy-mission-control/log/
> ?h=next-vardict
> http://cgit.collabora.com/git/user/cassidy/telepathy-gabble/log/?h=next-
> vardict
> http://cgit.collabora.com/git/user/cassidy/telepathy-idle/log/?h=next-vardict
> http://cgit.collabora.com/git/user/cassidy/telepathy-haze/log/?h=next-vardict

I merged all those as well.

Bug #76369 is basically this.

*** This bug has been marked as a duplicate of bug 76369 ***