Created attachment 107444 [details] [review]
Initial patch

This is a patch that adds the missing functionality. It needs to specialize Glib::Value<> for certain cairo types so they can be used with _WRAP_PROPERTY and the property mechanism of GooCanvasStyle, but that's probably OK since Goocanvas already wraps them into the GObject system.

Great. Some comments:

1. In enums.ccg, please add comments saying what those get_type() things are for.


2. I think this may be ambigious or lead to the wrong method being called sometime:
void set_align(const Glib::RefPtr<ItemModel>& child, double xalign = 0.0, double yalign = 0.0);
+  void set_align(const Glib::RefPtr<ItemModel>& child, Gtk::AlignmentEnum xalign = Gtk::ALIGN_LEFT, Gtk::AlignmentEnum yalign = Gtk::ALIGN_TOP);
We do it in gtkmm but we apparently should not. Please keep this for now but add a link to this bug in a TODO comment:
  http://bugzilla.gnome.org/show_bug.cgi?id=142849


3. What is this for in the .ccg?

+++ goocanvas/src/style.ccg	(working copy)
@@ -22,8 +22,23 @@
 namespace Goocanvas
 {
 
+// These trigger an internal compiler error on my machine (g++ 4.2.2). Note
+// we can't store these by value since in goocanvas, the ids are initialized
+// in class_init.
+#if 0
+const GQuark& Style::stroke_pattern_id = goo_canvas_style_stroke_pattern_id;

Likewise this in the .hg. Don't leave #if 0 in files with no comments:
#if 0
+  static const GQuark& stroke_pattern_id;

 
4. I'm a little worried about vfuncs that return RefPtrs, such as 
+  _WRAP_VFUNC(Glib::RefPtr<Item> get_parent(), get_parent)
We need to know if the C vfunc implementations should return a reference. If not then we need to take a reference somehow.

5. We can probably make the is_*() and get_*() vfuncs const when they return simple types. 

6. In items.ccg, or anywhere where you hand-code the vfunc, please add comments saying why you have done it. Well done for figure this stuff out, by the way.

7. This seems superfluous. Isn't _CTOR_DEFAULT() the same?
+ TableModel::TableModel()
+:
+  _CONSTRUCT()
+{
+}

8. You changed examples/table/examplewindow.cc to use the model classes. I would like to keep this example the same (well, without the models), but I would like a separate example that does use the models. Ideally it would show what they are good for.

Created attachment 107448 [details] [review]
Second patch

> 3. What is this for in the .ccg?
> 
> +++ goocanvas/src/style.ccg     (working copy)
> @@ -22,8 +22,23 @@
>  namespace Goocanvas
>  {
>
> +// These trigger an internal compiler error on my machine (g++ 4.2.2). Note
> +// we can't store these by value since in goocanvas, the ids are initialized
> +// in class_init.
> +#if 0
> +const GQuark& Style::stroke_pattern_id = goo_canvas_style_stroke_pattern_id;
> 
> Likewise this in the .hg. Don't leave #if 0 in files with no comments:
> #if 0
> +  static const GQuark& stroke_pattern_id;

These are to be used with Style::get_property or Style::set_property, respectively. I wanted a C++ developer to type Goocanvas::Style::stroke_pattern_id instead of goo_canvas_style_stroke_pattern_id. But, as stated in the comment, this produced an internal compiler error for me which is why I commented them out. Hm. Perhaps we can use just (inline?) functions instead of const references.

> 4. I'm a little worried about vfuncs that return RefPtrs, such as 
> +  _WRAP_VFUNC(Glib::RefPtr<Item> get_parent(), get_parent)
> We need to know if the C vfunc implementations should return a reference. If
> not then we need to take a reference somehow.

I assumed the get_ vfuncs to not return a new reference in C. The only exception is ItemModel::create_item_vfunc for which I wrote extra conversion functions.

> This seems superfluous. Isn't _CTOR_DEFAULT() the same?
> + TableModel::TableModel()
> +:
> +  _CONSTRUCT()
> +{
> +}

I copied this from table.ccg which is why it was in. I replaced it by _CTOR_DEFAULT in both classes.

> 8. You changed examples/table/examplewindow.cc to use the model classes. I
> would like to keep this example the same (well, without the models), but I
> would like a separate example that does use the models. Ideally it would show
> what they are good for.

Oh. That wasn't by intention. I changed it to use the TableModel to test its functionality and forgot to revert the change. I can perhaps come up with another example showing two views representing the same model.

I also fixed the other concerns you raised.

> I wanted a C++ developer to type
> Goocanvas::Style::stroke_pattern_id instead of
> goo_canvas_style_stroke_pattern_id. But, as stated in the comment, this
> produced an internal compiler error for me which is why I commented them out.
> Hm. Perhaps we can use just (inline?) functions instead of const references.

OK. Let's leave this for later. I'll try to take a look.

> I assumed the get_ vfuncs to not return a new reference in C. The only
> exception is ItemModel::create_item_vfunc for which I wrote extra conversion
> functions.

So we still need to check that we take a reference. Othewise we will unref a reference that we don't own.


Thanks. Please commit.

> > I assumed the get_ vfuncs to not return a new reference in C. The only
> > exception is ItemModel::create_item_vfunc for which I wrote extra conversion
> > functions.

> So we still need to check that we take a reference. Othewise we will unref a
> reference that we don't own.

Is there a way to check this?

> Is there a way to check this?

Just by looking at the code.

But if we don't then there will be a crash at runtime when using that class.

Armin, did you forget about this?

I had a look. The create_item() vfuncs return a new item and expect the caller to take the initial reference, such as most *_new functions. So I think the code is correct as it is.

I also added the new properties for the grid lines to Goocanvas::Table and Goocanvas::TableModel, and added an example for TableModel that displays two canvases showing the same model.