[Bug 29751] Presence statuses extending & XMPP privacy list use by plugins

[bugzilla-daemon at freedesktop.org]

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

Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review+ with trivial
                   |                            |changes (marked with ☃)

--- Comment #7 from Will Thompson <will.thompson at collabora.co.uk> 2010-08-25 10:12:51 PDT ---
(In reply to comment #4)
> (In reply to comment #1)
> > 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?

Yes, sorry. I'd make it more obvious it's an argument to tp_strdiff(), I think.
☃

> > +/* 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.

Hmm, yeah, I guess I misunderstood the point of the TODO. Add a link to
<https://bugs.freedesktop.org/show_bug.cgi?id=12896> and mention this on that
bug? ☃

> The enum type is wrong, I can't believe the compiler didn't catch it.

Yeah, I'm pretty surprised too. 

> > 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).

Ah yes. I'd forgotten how exactly the presence mixin works.

> 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.

Yep, okay, this makes sense now. The duplication is a shame, but it seems
unavoidable because the Telepathy enum doesn't have an equivalent of "chat". I
guess we can leave it for 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.

Yep, no worries.

> That's old code, the only thing I touched here is changed invisible
> list name / invisibility boolean flag with the list name.

Ah yeah. Thanks for making it clearer anyway. :)

> > 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.

Yep, much clearer. Thanks.

> > 
> > -          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)

Yeah, you're right. This does work correctly; I was wrong about how the code
worked. It's pretty confusing, but the diagram I rediscovered helped. :)

> > 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.

Okay, fair enough.

Having fixed the things labelled ☃, ship it!

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.