[Bug 14540] Names interface - Aliasing replacement with separate nickname, local alias etc.

[bugzilla-daemon at freedesktop.org]

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

--- Comment #52 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
(In reply to comment #51)
> http://cgit.freedesktop.org/~smcv/telepathy-gabble/commit/?h=wip-less-
> qdata&id=e5cd53ee8ebca0d0839ad1345183adc93510335b --> you can replace
> g_hash_table_lookup_extended() with g_hash_table_contains(), no?

True.

(In reply to comment #50)
> http://cgit.freedesktop.org/~smcv/telepathy-glib/tree/tests/dbus/names.
> c?h=names&id=e55d7bc56c4af7e80d9be785596844255de7875d --> I was doing the
> same for avatars, I changed ADD macro to do the 3 g_test_add() instead of
> repeating them. Dunno if you find that better :-)

I think it's perhaps clearer as-is, for now? We can easily change this if we
add more modes.

> http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/
> ?h=names&id=29766a5cfc785bc314b40ec87196e6b78bc0b68a --> you can give NULL
> callback to g_simple_async_report_error_in_idle(). Early return saves a bit
> of work, but we don't do it usually... Don't really matters of you prefer
> like that :-)

I'd dimly remembered that GSimpleAsyncResult doesn't always like NULL
callbacks, checked in my documentation (for 2.32) and noticed there was no
(allow-none). Is that a new thing?

> http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/
> ?h=names&id=57eaa0241be6ad60ca2a818cb120e4a8243a99ea --> 
> + g_value_set_string (value,
> + iface->dup_nickname (conn,
> + tp_base_connection_get_self_handle (conn)));
> --> should be g_value_take_string()

Indeed, this is a leak.

> + str = iface->dup_nickname (base, contact);
> +
> + if (!tp_str_empty (str))
> --> you should free str in the case it is g_strdup(""); or is that a
> forbidden return value for dup_nickname? Same for dup_local_alias() later in
> the patch.

This is in aliasing_dup_alias(), right? I do free it whenever it isn't
returned. (There's an unnecessary-but-harmless g_free (NULL) if it returns
NULL.)

> tp_names_mixin_nicknames_changed: Maybe I've misunderstood something, but if
> nickname != legacy_alias then you don't have to include that contact in the
> array.

Suppose we have:

    local_alias = empty
    nickname = "Dave"
    identifier = "dave at example.com"
    =>
    legacy alias = "Dave"

and we change nickname to empty. Now the legacy alias changes from "Dave" to
"dave at example.com" and we have to emit AliasesChanged.

For simplicity, the code does not currently detect the case where

    local_alias = empty
    nickname = "dave at example.com"
    identifier = "dave at example.com"
    =>
    legacy alias = "dave at example.com"

and we change nickname to empty, which has no practical effect; it will still
emit a redundant AliasesChanged signal in this case. If you want to handle this
case fully correctly, then yes we need to bring back the one_changed logic.

(Here, "empty" means either "" or NULL.)

> Also why did you remove the check if the array is empty, we could
> still have empty array here and avoid useless signals, no?

The NicknamesChanged hash table can only be empty in the case where the input
is empty, which is handled by an early-return now.

The AliasesChanged array can only be empty in the case where we clear a
nickname that happens to be identical to the identifier (which I think is
enough of a corner-case that I don't mind a redundant signal), or if the input
is empty (in which case the same early-return catches it).

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