Bug 20942

Summary: tp_asv_new() and tp_asv_set_*
Product: Telepathy Reporter: Danielle Madeley <danielle>
Component: tp-glibAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Danielle Madeley 2009-03-30 02:22:49 UTC
As a compliment to tp_asv_get_*(), I'd like to propose the addition of a:

GHashTable *tp_asv_new (const char *first_key, ...)

Which calls:
    GHashTable *asv = g_hash_table_new_full (
                        g_str_hash, g_str_equal,
                        NULL, (GDestroyNotify) tp_g_value_slice_free);

And then takes a NULL-terminated vararg list, as follows:

    tp_asv_new ("answer", G_TYPE_INT, 42,
            "question", G_TYPE_STR, "we just don't know",
            NULL);

Plus a full set of `void tp_asv_set_TYPE(GHashTable *asv, const char *key, TYPE v)` functions to complement the existing tp_asv_get_TYPE functions e.g.

void
tp_asv_set_string (GHashTable *asv, const char *key, char *value)
{
    g_hash_table_insert (asv, key,
                        tp_g_value_slice_new_string (value));
}

This is effectively a rehash of bug #18220, but I think it would significantly help to improve the readability of a{sv} maps and thus is worth having.

e.g. it drops this map setup for RequestConnection:

        GHashTable *parameters = g_hash_table_new_full (
                        g_str_hash, g_str_equal,
                        NULL, (GDestroyNotify) tp_g_value_slice_free);
        g_hash_table_insert (parameters, "account",
                        tp_g_value_slice_new_string ("davyd"));
        g_hash_table_insert (parameters, "password",
                        tp_g_value_slice_new_string ("sup"));

Down to 3-4 nicely wrapped lines:
        GHashTable *parameters = tp_asv_new (
                        "account", G_TYPE_STRING, "davyd",
                        "password", G_TYPE_STRING, "sup",
                        NULL);
Comment 2 Will Thompson 2009-03-30 06:50:32 UTC
Coding style: Telepathy style for wrapping function calls is:

  foo_get_bar (very, long, argument, list, ...,
      four_space_indent);

I note that http://git.collabora.co.uk/?p=user/davyd/telepathy-glib-davyd.git;a=commitdiff;h=904a2891a96d4ac56128cf9766ac7d9386e53481 "fixes" the indentation still to be wrong. :)

From the perspective of helping people not get this wrong, I'd replace:

  "a #GHashTable for storing a{sv} maps"

with:

  "a #GHashTable created with tp_asv_new()"

If people want to make their own hash table, they can read the tp_asv_new() documentation and discover what they must do.
Comment 3 Danielle Madeley 2009-03-30 19:16:15 UTC
Ok. I should have unnoobed the indentation now.

Added some missing symbols, and a tp_asv_dump() for debugging.

Also added the symbols to the reference docs (which apparently has to be done manually -- crazy).
Comment 4 Simon McVittie 2009-04-01 06:48:43 UTC
Some review comments:

Each tp_asv_set_* that takes a pointer should have tp_asv_take_* and tp_asv_set_static_* variants. The _take_ ones are useful when you're allocating something on the spot, and the _set_static_ ones are useful when you have some immutable static data in your C file.

I think tp_asv_set_bytes() should be slightly inconsistent by taking a length and a gconstpointer, like tp_g_value_slice_new_bytes() does, so it can be used for non-GArray sources of bytes (since the byte array is being copied anyway, there's no requirement that they come from a genuine GArray). If what you're copying *is* in fact a GArray, then tp_asv_set_bytes (asv, "BinaryBlob", array->len, array->data) is just as easy.

(You might have missed some of the boxed GValue slice allocators, because they're in dbus.[ch] rather than util.[ch] - the idea is that anything using GLib can benefit from sliced GValues, but the boxed ones like object path and bytes are specific to dbus-glib).

tp_asv_new() should have the G_GNUC_NULL_TERMINATED annotation.
Comment 5 Danielle Madeley 2009-04-02 19:31:04 UTC
Should all of the tp_g_value_slice_new() functions be documented in the same place? At the moment they're spread across two sections, which as you say, can cause you to miss them.
Comment 6 Danielle Madeley 2009-04-02 20:30:46 UTC
Ok branch updated per feedback.
Comment 7 Danielle Madeley 2009-04-03 20:26:59 UTC
Merged.

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.