[Bug 32611] Implement gabble Room interface updates

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Thu Sep 15 17:14:27 CEST 2011


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

--- Comment #19 from Jonny Lamb <jonny.lamb at collabora.co.uk> 2011-09-15 08:14:26 PDT ---
+ if (priv->must_provide_password == !!must_provide_password)

lolz

I highly endorse commits like cdf60cb7bbec25d1.

Maybe you should return from SetSubject if the channel gets disposed? Does that
DBusGMethodInvocation object get cleared up? Oh I now see you return in
close_channel(). Does that always get called? I guess it does.

I just opened bug #40798.

Why are we using conn_util_send_iq_async which appears to only be a small
wrapper around wocky_porter_send_iq_async? The error parsing seems pretty
small. Maybe it's worth it. You clearly think so.

Nit-picking: why are the type macros underneath the method protoypes in
room-config.h? I know it doesn't really matter but literally no other GObject
library does it, no? It looks weird.

What's the point in copying the hash table in validate_properties? If it's
valid then it returns a complete copy of the hash table given to
UpdateConfiguration. If it's not valid, validate_properties returns NULL. Why
don't you just make it return a boolean and then use the one given to the hash
table? Ohh, is it so you can use the hash table after the function returns (but
before the dbus method call returns) and you can't trust dbus-glib not to free
the hash table even if you ref it? I guess that's fine then, but a comment
explaining this would be nice.

I'm not sure this UpdateConfiguration API is great for protocols where setting
property can succeed and another fail in the same operation. This seems to
think all will succeed or all will fail. I don't have this protocol or even a
solution in mind for this.

+ priv->properties_being_updated = validated_properties;

You're banking on the hash table which is owned by GabbleRoomConfig being
around during the update_configuration call which might not hold if it's moved
to tp-glib. I'd be happier if you reffed it tbh.

Question: why do you create a new GSimpleAsyncResult in
gabble_room_config_update_configuration_async? Why don't you just pass
everything through to gabble_muc_channel_update_configuration_async?

Well, I hope I've done a good enough job of reviewing this. Commits like
4308d550320 are pretty hard to review so that you are confident "it's fine".

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



More information about the telepathy-bugs mailing list