Hello Ed,
Thank you for your work. Below are a few comments:
- you patch does not add the new header file x_icons.h to the
HEADERS list of include/Makefile.
- please add a space before and after ==, != and the likes to be
consistent with the rest of the code. Not a written rule - as far
as I know - but everybody does it like that.
- as you note in the comments, x_icons_init() uses w_current only to
extract bitmap_directory. What about passing it directly instead
of the structure?
- in the loop inside x_icons_init(), it would be better to free the
filename as early as possible and to keep this freeing inside the
loop (i.e. only one g_free() following gdk_pixbuf_new_from_file()).
- in the loop inside x_icons_init(), the GError is only set if
gdk_pixbuf_new_from_file() returns NULL. What about clearing it in
the block of the test before the 'continue'? (in the best case 2
g_clear_error() saved).
- in x_icons_init(), please use g_build_filename() instead of the
g_strconcat().
- you may want to rename x_icons_stock_pixmap() to something
exposing the fact that it is meant for the toolbar (the size of
the image returned). May be used only temporarily but still a good
thing to do.
- please remove the doxygen \par from x_icons_init() and keep the
format of the comment consistent with other function descriptions.
- the name of the parameter is wrong in the doxygen comment for
x_icons_stock_pixmap(). And please add an explicit direction to
the params.
- if you call x_icons_init() from x_window_setup() you are going to
add an icon factory every time a new window is created, most of
the time (if bitmap directory is unchanged) only duplicating
it. Do we really need one icon factory per window? The code makes
the change of the bitmap directory from a configuration file
loaded after the system config file but I do not think this is
desirable and ultimately should be changed. What are other people
thinking about that?
Regards,
Patrick
_______________________________________________
geda-dev mailing list
[email protected]
http://www.seul.org/cgi-bin/mailman/listinfo/geda-dev
