Re: Patches

["M . Thielker" <balsa t-data com>]

Hi,

On 2001.07.15 21:29 Pawel Salek wrote:
> Is there any reason why do you use in src/save-restore.c:free_toolbar()
> sometimes g_free() and sometime malloc/free(). I changed them all to
> consistently use g_malloc/g_free, I hope it is ok. I split also toolbar
> loading into a separate routine: config_global_load() has been already
> long.
> 
This is perfect. I didn't stumble onto the existence of the g_malloc and
g_free functions until I needed g_strdup because I had to pass a value that
would be g_free'd in another function. When I lloked for g_strdup I found
the entire glib alloc stuff and used it after that. All the code I wrote
before that had malloc/free, I dudn't bother to change it because a comment
in another source file indicated, that is is OK to use mallloc/free as well
as g_malloc/g_free, as logn as the 2 aren't mixed (e.g. mallog/g_free).
Thanks for changing it.

> I have noticed in src/sendmsg-window.c that:
> 		  get_toolbar(GTK_WIDGET(window), 1));
> 
> I.e. 1 is a hardcoded magic number identifying compose toolbar. This is
> not
> very self-explanatory. There should be an enum for this.
> 
Yes, you're right. I had first developed the main window toolbar, the
multi-toolbar magic was added later. I wasn't in the mood to go back and
change it everywhere I used it, so I left it hardcoded.

> I reimplemented toolbar-factory.c:populate_stock_toolbar()
> 
I'll have a look at it as soon as it makes it onto my CVS server - it's not
on there yet.

> This is not mentioned in HACKING, but the loops
>     for(i=0;i<MAXTOOLBARITEMS;i++)
> 	{
> 
> should be rather formatted as
> 
>     for(i=0; i<MAXTOOLBARITEMS; i++) {
> 
> (libmutt is an exception).
> 
Hmm, there's been much discussion about this in various forums on the 'net.
I personally prefer my indentation style, because it keeps the braces
aligned:

for(....)
{
|	loop code...
|	loop code...
|	loop code...
|	loop code...
}

Of course it takes up one line of screen real estate, but I don't mind. To
me it's more readable that way because it implies that any for/do/while/if
that does not have a line with a single open brace following it is NOT a
compound statement. The for(...) | style, combined with sloppy indentation
of code as a general rule makes it hard to see the beginning and end of
blocks.
But, well, I'll do it the other way in future patches.

> All global function should have description covering the purpose of the
> function, the input and output arguments. If a pointer is returned, it
> should be clearly stated if it should be released (disctinction between
> char* and const char* is useful).
> 
I wish that was the case for all functions, unfortunately, most are
undocumented......

> If the code is not dependent on balsa_app, it should go into libbalsa,
> whenever possible (I am not sure if it applies here, I mention it just in
> case).
> 
I really don't see the point of dividing it that way. I much more like to
draw the line at the point where a function becomes generally usable. If any
program other than Balsa itself could put a mail handling or mail display
function to use, it should go into the lib, is my opinion.

> file toolbar-prefs.c:
> toolbar_buttons - should not the strings be marked for translation?
> 
Yes! But I don't really understand how that translation stuff works....
What is the "po" subdir and how is it used?

> file toolbar-prefs.c:
> int->gpointer->int may not be identity transformation. Usage of
> GPOINTER_TO_INT() macro is recommended for portability.
> 
Sorry, didn't know that.

> file toolbar-prefs.h did not have LICENSE section and protection against
> multiple inclusion.
> 
An oversight..... I hope you added the license and include guard. Thanks

> I attempted to fix all this issues but I would appreciate if you had a
> look
> at the files and:
> a. paid attention to the formating style (minor)
> 
> b. added comments when marked with FIXME (it would be useful if
> non-trivial
> static functions would have comments as well). This would make
> debugging/auditing by other persons easier.
> 
I'll do that when I get the code from CVS

> Uff.... Anyway, this patch is in. Thanks!
> 
Thanky you for your troubles.

Cheers,

Melanie