In GVariant we trust.
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 ***
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.