Re: [GnomeMeeting-devel-list] [PRE-PATCH] private struct _GmTextChat

["Damien Sandras" <dsandras seconix com>]

Le 26/1/2004, "PUYDT Julien" <julien puydt laposte net> a écrit:

>On dim, 2004-01-25 at 21:26, Damien Sandras wrote:
>> Hi,
>>
>> I only have a few things to comment on :
>>
>> - that kind of patch is, believe me, very dangerous.
>
>Why so?


Among others because you are using malloc, when I'm using new. And there
is a good reason why new is used (not in the text chat, but for the
rest).

Anyway, before applying it, I would like to see the same kind of patch
for GmWindow.

Don't take it bad, I like the idea of moving things out of common.h
*very* much.

>
>> - I don't like 1 line functions, I don't think it makes much sense to
>> write 4 lines of code for a 1-line function.
>
>Well... since _GmTextChat is internal to the text-chat:
>* gnomemeeting.cpp can't instantiate and delete it (hence the need to
>add two functions: one _new and one _delete);
>* main_window.cpp needs a way to get access to the window, but doesn't
>know where it is in the structure, so there must be a function that
>returns that information!
>

ok, I see your point.

>> - Functions to retrieve things should contain "get" in their names
>
>Well, in fact the first draft of that patch had
>"gnomemeeting_text_chat_widget" ; I didn't like the name and patched my
>patch so it became "gnomemeeting_text_chat_window"... That function may
>disappear, see below.


Should be gnomemeeting_text_chat_window_new.
It was already misnamed due to the fact that the original patch was a
contribution and it was not reviewed enough.
>
>> - Things like :
>> gnomemeeting_text_chat_window (GmTextChat *chat)
>> {
>>   return chat->window;
>> }
>>
>> do not make much sense I believe. Will you do a one line function for
>> each of the fields present in all structures?
>
>Well, I wouldn't have hidden the structure if all of the fields where
>supposed to be public. In fact, after a fresh night, I would rather go
>for:
>typedef struct _GmTextChat GmTextChat;
>typedef struct _GmTextChatPrivate GmTextChatPrivate
>struct _GmTextChat {
>/* public members, like the window */
>GmTextChatPrivate *private;
>}
>
>And then, only define _GmTextChatPrivate in chat_window.cpp. That scheme
>would give us:
>1) both a public part (no need to write an access function for each
>variable that we don't really want to hide, and makes it easy to add a
>new public one),
>2) and a private part (that makes it possible to modify the internal
>implementation of the text-chat at will).
>
>The bonus is that the same scheme could be used for the other structures
>in common.h.


Very clean!
That's what GTK+ is doing.
It will ease the transition to GObject.

>I hope the new precision will make the more bigger and the less smaller
>;-)
>

Indeed. Please also try to do it for GmWindow. I will probably be unable
to review the patch before this WE though, but we have plenty of time
for it as it doesn't modify features and strings.

Doing that kind of cleaning was one of my objective for 1.00. I've done
it at many places (including the prefs window), but I'm really happy
you can continue that job.