[GSoC] GObjectification Summary

[Mark Doliner]

I have a little more code to look through, but I'm about to head out
of town for the weekend and wanted to send a little more feedback
first. Sorry it's taking me a long time. In my defense, you changed a
lot of code (for the better!) :-P

Has anyone besides me and Ethan reviewed the gobjectification branch?
Anyone else have thoughts? Anyone object to merging the
gobjectification branch (not the .plugins branch yet) to main?

New notes and questions:
- The new conversation code (in gtkconv and probably elsewhere)
assumes that if a conversation isn't an IM then it is a chat. Whereas
the old code always checked is_im and is_chat and did nothing if both
of those checks were false. I think this means stuff will misbehave
and sometimes crash if a plugin tries to extend PidginConversation to
create a third type. Am I correct? I'm totally fine with this, but I
thought I'd mention it in case anyone knows of a good use case where
we want to support this.

- Our GObjects mark as translatable the name and description strings
for each object's properties (e.g. _("Who"), _("Who you're drawing
with.")). That's a large number of new strings to translate. They're
not surfaced to the user, right? I imagine they could be surfaced to
developers in API documentation? Do we think it's worth the burden on
translators to translate these strings?

- Why have an unused PROP_0 enum value?

- In what circumstances do you call g_object_notify() in a
purple_foo_set_bar() function? Are there times when it should not be
called? For example, why isn't it called in
purple_whiteboard_set_prpl_ops().

- Small bit of #if 0'ed code in dbus-server.c
purple_dbus_message_append_values, also dealing with outgoing values,
as with the other code I mentioned earlier. Is this code also not
important?

- Struct member comments should be copied over from old
PurpleCircBuffer to new PurpleCircularBufferPrivate.

- New functions at bottom of circularbuffer.h need doc comments.

- I'm seeing a crash when trying to sign on with an AIM account. I
haven't had a chance to try to debug it yet, but it is reproducible
for me. Does that sound familiar? Stack trace:
http://pastebin.com/20iw9YG0


And a few comments in reply to your previous email inline below:

On Thu, Oct 24, 2013 at 5:52 AM, Ankit Vani <a at nevitus.org> wrote:
> On 24 October 2013 13:57, Mark Doliner <mark at kingant.net> wrote:
>> - Do non-privileged trac users have access to
>> https://developer.pidgin.im/newticket? We previously linked to
>> simpleticket. In doc/finch.1.in and doc/pidgin.1.in
>
> simpleticket redirects to newticket, which asks users to log in.

Ah, you're right. Cool. The current behavior is different than in
years past--I didn't realize that. Thanks for pointing that out.

>> - Need to do anything about the #if 0'ed code in
>> libpurple/plugins/tcl/tcl_signals.c?
>> - Need to do anything about the #if 0'ed code in
>> libpurple/plugins/perl/perl-common.c purple_perl_sv_from_vargs()?
>> - Need to do anything about the #if 0'ed code in
>> libpurple/plugins/perl/perl-handlers.c perl_signal_cb()?
>
> I don't think anything needs to be done about this code, but I'm not 100%
> sure about it, since I haven't really messed around with the loader stuff.
>
> Those #if 0's are mostly due to removal of PurpleValue and the concept of
> 'outgoing' values (instead of which we just use pointers now).
>
> When merging soc.2013.gobjectification.plugins in the future, these files
> will become obsolete (loaders are handled by GPlugin and bindings handled by
> introspection).

Sounds reasonable.

>> - Our example plugins shouldn't #include <internal.h>, right? I guess
>> it doesn't matter if the code is in our tree. Even better would be for
>> these things to live in a separate repo or package outside our tree,
>> and for us to distribute simple autoconf and automake files. (Some
>> files that include internal.h that maybe shouldn't:
>> libpurple/plugins/dbus-example.c libpurple/plugins/debug_example.c
>> libpurple/plugins/helloworld.c libpurple/plugins/notify_example.c)
>
> Yeah I agree that internal.h should not be included in these plugins.
> However, internal.h is used for the translation stuff and stuff like
> DISPLAY_VERSION and PURPLE_WEBSITE.
>
> What do you suggest?

I think this is fine for now. We should add a comment to these files
near the #include "internal.h" along the lines of "We're including
libpurple's internal.h file here because it's convenient for us. This
file is for internal libpurple use only. It should not be included by
3rd party plugins."

>> In an earlier email you said:
>> "These entities are temporarily GBoxed, with a TODO mentioning so in their
>> purple_entity_get_type() function's documentation, and should eventually be
>> converted to GObjects:
>>  GBoxed
>>   +----PurpleCertificatePool
>>   +----PurpleSavedStatus
>>   +----PurpleStoredImage
>>   +----PurpleLog
>>
>> Do we need to do anything about these before merging? Or after merging?
>
> These are the components that should really be GObjects, but haven't been
> GObjectified yet since they're not an immediate concern. The only reason they
> are boxed is so that we have a GType for them for use in loaders, as
> PurpleType no longer exists. I suppose this can be dealt with after merging.

Sounds good to me.