Created attachment 241650 [details] [review]
Add a function to ensure types are registered

Review of attachment 241650 [details] [review]:

Thanks for the patch, I have some comments below:

::: Makefile.am
@@ +210,3 @@
        $(NULL)
 Gd_1_0_gir_INCLUDES = $(LIBGD_GIR_INCLUDES)
+Gd_1_0_gir_FILES = $(libgd_la_SOURCES) $(nodist_libgd_la_SOURCES)

I guess this is needed because you import gd.h in gd.c? If so I'd rather you included the headers directly (you also need #ifdefs for each at that point I believe).

::: libgd/gd.c
@@ +16,3 @@
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */

I would rename this to something like gdtypescatalog.c.

@@ +58,3 @@
+#ifdef LIBGD__HEADER_BUTTON
+  g_type_ensure (GD_TYPE_HEADER_SIMPLE_BUTTON);
+  g_type_ensure (GD_TYPE_HEADER_MENU_BUTTON);

Missing GdHeaderToggleButton and GdHeaderRadioButton here

(In reply to comment #2)
> I guess this is needed because you import gd.h in gd.c? If so I'd rather you
> included the headers directly (you also need #ifdefs for each at that point I
> believe).
> 

May I ask why? duplicating all the ifdefs seem kind of pointless when the prototype of the function itself is in that header. Besides all the other libs I checked included public headers in the gir_FILES. What am I missing?

To be honest I do not quite understand why the source files are split into a nodist var in the first place...

> 
> I would rename this to something like gdtypescatalog.c.
> 

Seems kind of long and I do not like having .c files without a matching header... should I also have a separate .h file? I also guess that it would be gd-types-catalog with dashes for consistency with the other files.

Also, Zeeshan in #698146 proposes to call the function gd_init(). I have no strong opinion about that

Created attachment 241775 [details] [review]
Try 2

Here's an updated patch that adds ensure's for the missing GdHeaderToggleButton and GdHeaderRadioButton and renames gd_ensure_types() to gd_init()

@cosimo: I'm not experienced enough with the autoconf stuff to comment on your first remark, but gd.c/gd_init() as Paolo and Zeeshan suggested seems fine to me.

Created attachment 241782 [details] [review]
Try 3

Incorporated all suggested changes. 

I really hope I did it this time. I feel unsure about the #includes inside gd_ensure_types() but it all seems to work.

Review of attachment 241782 [details] [review]:

Looks better, thanks. I have another comment.

::: libgd/gd-types-catalog.c
@@ +33,3 @@
+# include <libgd/gd-styled-text-renderer.h>
+# include <libgd/gd-toggle-pixbuf-renderer.h>
+# include <libgd/gd-two-lines-renderer.h

This seems to be missing a matching ">" at the end.

But generally, please move all the includes at the top out of the function body, and use #include "gd-foo-bar.h" instead of #include <libgd/gd-foo-bar.h> (headers will be in the same directory, and it's consistent with what GTK and other files in libgd do already).

If no one gets in before me I will fix this tomorrow. Thanks for the review!

I amended the patch and pushed

(I do not 100% agree on moving the includes at the top since now we have the ifdefs in *three* places instead of just one, but oh well...)

Can someone please update the status of patches?

Comment on attachment 241782 [details] [review]
Try 3

A version of this modified for review comments was committed.