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

[PUYDT Julien <julien puydt laposte net>]

On lun, 2004-01-26 at 10:17, Damien Sandras wrote:
> 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).

Does "new" guarantee that the memory allocation was successful? Because
that's something the patch didn't check, but really should have...

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

I'll have a look at it. 

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

Nice.

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

With the unknow part of the structure being a pointer, this is not
really needed anymore.

But notice that doing a:
chat = new thingie ();
then later on:
oh_please_init_that (chat);
is dirty! It really should be:
chat = get_me_a_new_one_please_thanks ();

> Should be gnomemeeting_text_chat_window_new.

Well:
chat = gnomemeeting_text_chat_new ();
would be even better, since the window isn't the only thing initialized.

> It was already misnamed due to the fact that the original patch was a
> contribution and it was not reviewed enough.

That's the reason I prefer going through the ml than having a cvs
account ;-)

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

Nice to know... perhaps I should have a look at how gtk does things,
instead of thinking them up again...

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

Ok.

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

Ok, happy to be able to help,

Snark