[Bug 32611] Implement gabble Room interface updates

[bugzilla-daemon at freedesktop.org]

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

--- Comment #20 from Will Thompson <will.thompson at collabora.co.uk> 2011-09-15 10:50:20 PDT ---
Pushed a couple of patches to each branch fixing your comments. (I actually
realised an untested aspect of the subject code too, so there's a patch fixing
that as well.)

(In reply to comment #19)
> + if (priv->must_provide_password == !!must_provide_password)
> 
> lolz

I'm going to defend this on the grounds that it makes == on gbooleans safe.

> I highly endorse commits like cdf60cb7bbec25d1.

Thanks!

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

It does. I was a bit suspicious but it seems to be correct.

> I just opened bug #40798.

Yes.

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

It saves fishing the porter out of the Gabble connection, and doing the error
parsing in the callback: porter_send_iq_finish() returns TRUE if we got a reply
which happened to be an error, whereas conn_send_iq_finish() returns FALSE in
that case.

I do think it's worth not typing that stuff out over and over. :)

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

Straw poll of telepathy-glib:

• base-connection.h has them in the middle of the prototypes \o/;
• contact.h has them above the prototypes but below the typedefs;
• contact-search-result.h has them above the typedefs and prototypes;
• media-interfaces.h has them in between the typedefs and the prototypes.

So yeah, below typedefs and structs, but above prototypes, seems to be most
common. I can change this if you like. I put them at the bottom because I think
they're ugly and wanted them out of the way. Would you rather they were above
or below the enum definition?

I picked above.

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

The two hash tables have different keys. The keys passed over D-Bus are
strings; the keys on the table built by validate_properties are elements of the
TpBaseRoomConfigProperty enum.

I'll document validate_properties to make the difference clear.

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

That's true. IRC has this issue, for instance. FWIW the old API had the same
issue. :) What else should we do? Only allow changing one field at a time?
Represent partial success with a successful return with a dictionary of what
failed? (I really hate APIs like the latter.)

With the status quo, if some, but not all, changes were applied, the properties
which did get changed should have their values updated in any case.
Well-behaved CMs could prioritise more crucial settings over less crucial ones:
so like, update the password before disabling invite-only, or whatever.

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

Good point.

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

Because TpBaseRoomConfig expects the source object in the callback to be a
TpBaseRoomConfig subclass.

The longer answer is that I was planning to move all the room config code into
room-config.c in Gabble, but didn't bother.

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

I appreciate that. I'm pretty confident they're 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.