[Bug 29751] Presence statuses extending & XMPP privacy list use by plugins
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Wed Aug 25 10:44:06 CEST 2010
https://bugs.freedesktop.org/show_bug.cgi?id=29751
--- Comment #4 from Senko Rasic <senko at senko.net> 2010-08-25 01:44:05 PDT ---
Thanks for the review.
(In reply to comment #1)
> But the point is moot because if this function just returned the array of
> statuses from the interface, gabble_plugin_loader_append_statuses() would be
> way shorter and easier to follow (untested).
Indeed, rewrote it based on your suggestion.
> Coding style dictates:
>
> + if (status != NULL)
> + return status;
>
> Ditto indentation of:
>
> + if (iface->privacy_list_map == NULL)
> + return NULL;
Thanks, fixed.
> I think indenting the second argument of tp_strdiff() in
>
> + if (!tp_strdiff (list_name,
> + iface->privacy_list_map[i].privacy_list_name))
>
> by four spaces would be more readable.
It is indented by four spaces. Did you mean additional four (ie. 8) spaces?
> +/* TODO: update this when telepathy-glib supports setting
> + * statuses at constructor time; until then, gabble_statuses
> + * leaks.
> + */
>
> It's a once-per-process leak; is that so bad that it warrants an entry in
> FIXME.out? :)
Hm, I hadn't known TODOs go into FIXME.out; It's a reminder what to do
once Tp-Glib supports it, and is actually more about the fact that
it'd be nicer to do it at constructor time, than the fact that it
leaks, but I wanted to document it. Removed the TODO keywword.
> +GabblePresenceId _conn_presence_get_type (GabblePresence *presence);
>
> This is the only function in that header with a _ prefix.
Changed so it's not-prefixed. Originally I wanted to denote that it's
"internal", but otoh, gabble is an application not a library (leaving aside the
convenience library and the plugins), so everything's internal anyways.
> + GabblePresenceId presence_type = _conn_presence_get_type (presence);
> +
> + switch (presence_type)
> + {
> + case TP_CONNECTION_PRESENCE_TYPE_AVAILABLE:
>
> The TpConnectionPresenceType enum doesn't match the GabblePresenceId enum. And
The enum type is wrong, I can't believe the compiler didn't catch it.
Originally i tried to use GabblePresenceId directly, but then found out it's
not enough and i have to fall back, so I've copy/pasted the code, but didn't
change the type appropriately. The type should be TpConnectionPresenceType.
> I don't understand this function body at all now. Do plugins have to assign IDs
> to presences that fall off the end of the GabblePresenceId enum or something?
There's never been any explicit ID assignment - it was always implicit, by
making sure gabble_presences table (now base_presences) is in sync with the
PresenceId enum. Which means, that all statuses added by plugins will have id
after the GabblePresenceId (because they'll be after the base_statuses in the
table generated by plugin loader).
But this function is not at all about that. This one is about knowing how to
"render" the status. Since we don't have support for plugins doing it manually
(maybe we'll have to, at some time, but we don't now, and I don't want to guess
the future), this function has a fallback using the presence type id to guess.
So, e.g. if you have custom "at-work" status which is using "at-work-list"
privacy list, but is actually a specialised kind of "busy", when you select it,
gabble would do:
1. set the privacy list to "at-work-list"
2. signal own presence
For (2), it needs to know how to "render" at-work; since it's a specialised
"busy" (the specialisation provided by "at-work-list"), we can put anything
(except "unavailable") there, but to be semantically nice, we put "busy" there
(based on its presence type).
Hope it's clearer now.
(In reply to comment #3)
> It would have been easier to review
> http://git.collabora.co.uk/?p=user/ptlo/telepathy-gabble/.git;a=commitdiff;h=0794ab632674651887d6eb200965aed5caf6cebe
> if it had been in smaller chunks. :(
Yes, definitely. I'm sorry for dumping this big chunk of diff on you. But since
logic is intertwoven, I couldn't break it into smaller self-sufficient pieces,
unless I rewrote it again, piece by piece... Since the code went through many
iterations of code before being final, I couldn't just commit after each
logical change.
> + if (list_name)
> + iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
> + WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
> + '(', "query", ':', NS_PRIVACY,
> + '(', "active",
> + '@', "name", list_name,
> + ')',
> + ')',
> + NULL);
> + else
> + iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
> + WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
> + '(', "query", ':', NS_PRIVACY,
> + '(', "active", ')',
> + ')',
> + NULL);
That's old code, the only thing I touched here is changed invisible
list name / invisibility boolean flag with the list name.
> I think the following is probably clearer:
>
> + iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
> + WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
> + '(', "query", ':', NS_PRIVACY,
> + '(', "active",
> + '*', &active_node,
> + ')',
> + ')',
> + NULL);
> +
> + if (list_name != NULL)
> + wocky_node_set_attribute (active_node, name, list_name);
Wow, I didn't know wocky could do that. It's almost as if it were a whole new
language :) Yes, that does look clearer to me too, changed the code
accordingly.
> Also, since list_name can be NULL, the following will crash on Windows and
> other platforms where printf ("%s", NULL); crashes:
>
> + DEBUG ("Privacy status %s, backed by %s",
> + gabble_statuses[presence->status].name, list_name);
Good catch, fixed.
> + else if (!list_name && base->status != TP_CONNECTION_STATUS_CONNECTED)
>
> list_name == NULL plz?
Fixed styling.
> + g_assert (priv->privacy_statuses);
>
> Why is this an assertion whereas earlier we had a g_return_if_fail() for it?
> Both should really use != NULL anyway.
Actually both should be asserts, since both of them are used only internally.
> I'm having a hard time convincing myself that this is guaranteed to be NULL or
> not NULL at the various points we assert about it. Maybe it could have a
> comment explaining its lifecycle, at least?
The idea is that privacy_statuses being != NULL signals we know the server has
privacy list support. So, it's NULL up until we get response to our "get all
privacy list" request (and the response is not error); then, it's not null
until the object finalization.
I've added comments explaining this hopefully a bit better.
> + for (i = node_iter (iq); i; i = node_iter_next (i))
>
> There's no reason to use the node_iter() macros in new code. In fact, this code
> should use Wocky's node iterators to only iterate across "list" children.
I cargo-culted that from another part of conn-presence. Fixed.
>
> - gchar *new_name = g_strdup_printf ("%s-gabble",
> - priv->invisible_list_name);
> g_free (priv->invisible_list_name);
> - priv->invisible_list_name = new_name;
> + priv->invisible_list_name = g_strdup ("invisible-gabble");
>
> I think this code deliberately extended the previous name over and over,
> because it can be hit more than once. If there's a test case for both
> "invisible" and "invisible-gabble" existing but not satisfying
> is_valid_invisible_list(), this change is okay with me, though.
Well, AFAICS, the code never had a feedback loop for verifying the list after
it's beeen created. There's a possibility of having the code being executed
more than once because of list_push_cb, but in that case the whole sequence
starting with setup_* is ran, not just the verification.
(didn't change anything now; I can revert it, but I'd like to know whether my
assumption of how the old code works are correct, or whether I've missed
something there)
> + /* This relies on the fact the first entries in the statuses table
> + * are from base_statuses. If index to the statuses table is outside
> + * the base_statuses table, the status is provided by a plugin. */
> + if (status >= (sizeof (base_statuses) / sizeof (TpPresenceStatusSpec)))
>
> Yeah, this is pretty scary.
Not really, the index is never outside gabble_statuses (which is used elsewhere
in the code). This is just the easiest way to determine whether the status is
one of the base statuses, or one of the statuses provided by plugins.
> Also, G_N_ELEMENTS.
Changed.
> What's the point of testaway here? Why is it legal for plugins to provide
> statuses that aren't linked to privacy lists?
The point of testaway here is to check that plugins can add any status that
could be added statically, too; including the one that is not backed by privacy
lists, and in fact, is not settable by us. I think it's unneccessary to
artificially restrict type of statuses that could be added, since:
a) it's more work to restrict it than to support adding any status
b) maybe in the future we'll want a second way for plugins to implement
statuses, in which case it wouldn't be nice presence addition was coupled with
privacy list linking
I think the two things are orthogonal and should be decoupled, so I did it that
way.
>
> + elem_list=[]
> + for li in lists:
> + elem_list.append(elem('list', name=li))
>
> elem_list = [elem('list', name=l) for l in lists]. Or even inline it into
> building the iq.
Fixed, thanks.
I've updated my branch with fixes based on your review.
--
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
More information about the telepathy-bugs
mailing list