Created attachment 270226 [details] [review]
Adds the constraint classes

Created attachment 270227 [details] [review]
Wraps action classes

Created attachment 270228 [details] [review]
Wraps Effect classes

Created attachment 270229 [details] [review]
Wraps Actor and Actor-Meta class

Created attachment 270230 [details] [review]
Several classes supporting Clutter::Actor wrapped

Created attachment 270231 [details] [review]
Animation Framework, and Stage classes.

Created attachment 270232 [details] [review]
Layout Classes wrapped

Created attachment 270235 [details] [review]
Several general classes including Intervals, transitions, paint, text.

Created attachment 270236 [details] [review]
Backend and Device management classes

Created attachment 270237 [details] [review]
Drawing classes: Canvas, Color, Content and Image

Created attachment 270238 [details] [review]
Includes Codegen files, filelist.am

Created attachment 270239 [details] [review]
clutter::main files, event and types.  Also filelist.am

Created attachment 270240 [details] [review]
Several examples.

Created attachment 270241 [details] [review]
Defs files.  This is apparently too big to add as a patch, so this is the first 30k lines...

Created attachment 270242 [details] [review]
..and this is the second half. 

My apology; I wasn't aware of the size constraint, and don't want to have to rebase the patches (yet again); to my understanding there's no way of chopping up a patch that won't mess up the git log without doing that.

Created attachment 270244 [details] [review]
jhbuild moduleset file modifications

Modifies the 3 jhbuild moduleset files required to build cluttermm and cluttergtkmm appropriately.  Points to the git repo's.

Review of attachment 270226 [details] [review]:

>  so there is code in the methods.defs file that reflects
this change that is not in this commit.

Could you please modify this patch/commit to include all the necessary changes. For instance, you could include just the relevant .defs changes via git add -p .

Then it will be more obvious why that "code in methods.defs" is there.

Review of attachment 270227 [details] [review]:

> Note that there is
> other commits required for complete functionality, including the one
> that touches filelist.am and the vfuncs.defs

Again, that makes it seem like this commit is incomplete.

Review of attachment 270228 [details] [review]:

> flatten-effect is included, but is not currently wrapped as
> it is not included in the clutter library as yet.

Do you meant that it's not public API, or that the clutter developers plan to provide this API some time in the future. Either way, there's no reason for it to be in cluttermm.

Review of attachment 270244 [details] [review]:

> These files were using deprecated sources; I've posted my modified
> versions that allow building the latest trunk.

Do you mean that the cairo and clutter versions from jhbuild are too old? They don't seem to old. What do you need from new version of cairo, and what do you need from new versions of clutter?

There also seem to be some white-space changes here, and a changer to hamster-applet. They should not be in the same patch.

Review of attachment 270238 [details] [review]:

The files should be added to the build in the same commit that adds those files, please.

(In reply to comment #17)
> Review of attachment 270226 [details] [review]:
> 
> >  so there is code in the methods.defs file that reflects
> this change that is not in this commit.
> 
> Could you please modify this patch/commit to include all the necessary changes.
> For instance, you could include just the relevant .defs changes via git add -p
> .
> 
> Then it will be more obvious why that "code in methods.defs" is there.

Thanks.

The problem is that the whole API has changed.  I'm unable to post a single self-contained commit because the library is completely different: the API for painting actors, layout, actions, effects and the backend are all using new classes, or classes with significant modifications from the API that is currently wrapped.  The way I approached this was to start at the first file and work through to the last; teasing out the individual changes might be possible but it's going to be almost as much work as wrapping the library was in the first place.

In this case, regenerating the methods.defs file deprecates functions throughout the library.  I elected to post the new file rather than have to manually go through it and split up the changes into individual classes.

(In reply to comment #19)
> Review of attachment 270228 [details] [review]:
> 
> > flatten-effect is included, but is not currently wrapped as
> > it is not included in the clutter library as yet.
> 
> Do you meant that it's not public API, or that the clutter developers plan to
> provide this API some time in the future. Either way, there's no reason for it
> to be in cluttermm.

FlattenEffect is a ClutterEffect that looks like it will become part of the library soon.  Unfortunately it's not yet able to be wrapped, despite appearing to be so in the documentation for Clutter.  I've left it in because I'm expecting it to become available in the next few weeks to months.

(In reply to comment #20)
> Review of attachment 270244 [details] [review]:
> 
> > These files were using deprecated sources; I've posted my modified
> > versions that allow building the latest trunk.
> 
> Do you mean that the cairo and clutter versions from jhbuild are too old? They
> don't seem to old. What do you need from new version of cairo, and what do you
> need from new versions of clutter?
> 
> There also seem to be some white-space changes here, and a changer to
> hamster-applet. They should not be in the same patch.

Changes in jhbuild: cairo was pointing to the cvs repo.  I've pointed it to the freedesktop.org git repo.  I can't remember what the conflict was now, but from memory using an old version of cairo was stopping the build of clutter.

The unblocking change in clutter-gtkmm is an #include for a deprecated cluttermm class (clutter-texture) that I've removed.  I elected to remove deprecated API because it reduced the workload by about 15 classes; I'm aware that's not normally appropriate, but it was a time-management decision.  (Clutter does seem to have broken ABI since I last used it, so I was simply following the parent project ).  Also, given its current state there can't be anyone using the cluttermm library so I wouldn't be breaking anything.  The tarball for 1.3.3 is still available; this would only be available to people building from jhbuild, and it's pretty obvious there's no-one doing that at present.  From a functional point of view this is a rewrite, not an update.  I contemplated making it a 2.0 build, but clutter is about to jump to 2.0 so I'm guessing it makes more sense to wait for that.

The hamster-applet change: I can't recall what that was about, but from memory there were dependency problems during the build and that was one of them.  I updated the moduleset list to gnome 3.12 and it broke the build.  

Sorry about the whitespace changes; I'm still learning about posting patches (if that wasn't obvious enough already).  

I'll fix the patch, but should I be posting it here or to the jhbuild list?

Created attachment 270319 [details] [review]
jhbuild patch to build latest version of cluttermm and clutter-gtkmm

removed whitespace and hamster-applet changes.

(In reply to comment #22)
> In this case, regenerating the methods.defs file deprecates functions
> throughout the library.

I guess you mean "removes" rather than "deprecates". Yes, I can see how that would make things difficult.

I can live with a couple of initial commits to
1. Use the new gstreamer library version.
2. Update the .defs files.

But after that commmits should please be as self-contained as possible, even if it would take a few commits until things build again. At the very least, files should not be added if they are not added to the build in the same commit.

Otherwise, it would be very difficult for anybody to track the changes to the project and its API. Once you've got into this habit automatically I'd be very glad to approve git commit access for you.

(In reply to comment #23)
> FlattenEffect is a ClutterEffect that looks like it will become part of the
> library soon.  Unfortunately it's not yet able to be wrapped, despite appearing
> to be so in the documentation for Clutter.  I've left it in because I'm
> expecting it to become available in the next few weeks to months.

Then, sorry, then it doesn't belong in git master yet.

(In reply to comment #24)
> Changes in jhbuild: cairo was pointing to the cvs repo.  I've pointed it to the
> freedesktop.org git repo.  I can't remember what the conflict was now, but from
> memory using an old version of cairo was stopping the build of clutter.

Sorry, but you'd need a clearer reason to update the cairo version in jhbuild. It's a very basic external dependency that the whole GNOME project uses, intentionally via a tarball.

> The unblocking change in clutter-gtkmm is an #include for a deprecated
> cluttermm class (clutter-texture) that I've removed.
[snip]

Sorry, I don't understand what jhbuild moduleset change you are referring to. It would be easier to understand if it was a simple commit with an explanatory commit message.

> The hamster-applet change: I can't recall what that was about, but from memory
> there were dependency problems during the build and that was one of them.
[snip]

That change looks harmless, though the glibmm dependency shouldn't need to be explicitly stated, and I wonder if the cairomm dependency isn't necessary because it's already brought into by pangmm. I'd happily commit a small patch for you that just adds the cairomm dependency. The jhbuild developers are not likely to spend time on that.

Should I now add the new (hopefully) improved patches to this bug, create another bug, or a set of bugs to post them?

Please add them here.

But feel free to submit just one or two to begin with.

Created attachment 270882 [details] [review]
Updates the clutter_methods.defs file.

output from h2def.py only.

Created attachment 270883 [details] [review]
updates the clutter_enums.defs file.

Created attachment 270884 [details] [review]
updates the clutter_signals.defs file, generated output from glibmm toolset.

Created attachment 270886 [details] [review]
clutter_signals has a series of hand-patches.  These are reapplied.

Modifications to Clutter::Actor Event handling.  

The signals.defs file has some hand-written modifications to allow specific event types to be handled that were lost on regenerating the file; these are replaced and extended to the new signal types.

Change to Actor::event() method where the ClutterEvent passed needs a const qualifier.

Review of attachment 270884 [details] [review]:

Presumably you changed configure.ac to make cluttermm link against a different (parallel-installable) clutter version to be able to generate this. I'd prefer to commit that change first, or as part of this commit.

Review of attachment 270886 [details] [review]:

The .defs file changes are fine, but the other change shouldn't be here.

I think gtkmm, which now makes this easier by having tools/gen_scripts/, has a patch that gets applied automatically whenever we regenerate the .defs file, because GTK+ has the same problem with event types.

::: clutter/src/actor.hg
@@ -248,3 @@
   _WRAP_METHOD(void transform_stage_point(float x, float y, float& x_out, float& y_out) const, clutter_actor_transform_stage_point)
 
-  _WRAP_METHOD(bool event(ClutterEvent* event, bool capture), clutter_actor_event)

This change seems to be unrelated to the signals defs changes. Therefore they do not belong in the same commit.

Comment on attachment 270884 [details] [review]
updates the clutter_signals.defs file, generated output from glibmm toolset.

Sorry, I was wrong about this needing a change to configure.ac. I didn't realise that cluttermm was already using clutter-1.0.

Created attachment 270940 [details] [review]
patches clutter_signals to pass specific subtypes of ClutterEvent

I need advice.  The library now won't wrap because Clutter::Alpha is deprecated (and removed) in clutter.  There's going to be a number of these because the API has moved so far since the last wrapping was done.

What's the best way forward?  Should I remove the deprecated classes from generate_extra_defs and filelist.am, remove all references to them in one set of patches, and then rebuild on the (minimal) library that's left?

(In reply to comment #40)
> I need advice.  The library now won't wrap because Clutter::Alpha is deprecated
> (and removed) in clutter.
>  There's going to be a number of these because the
> API has moved so far since the last wrapping was done.

ClutterAlpha is deprecated in clutter-1.0 and removed in clutter-2.0. You appear to be working against clutter-1.0 for now, which seems wise.

Deprecated API should be no problem for you. If it's causing some specific build problem, please mention that so I can help you around it. Maybe you just need to disable deprecation in some hg/ccg files. We do this in gtkmm frequently.

> What's the best way forward?  Should I remove the deprecated classes from
> generate_extra_defs and filelist.am, remove all references to them in one set
> of patches, and then rebuild on the (minimal) library that's left?

There should be no need to remove C++ API just because the underlying C API is deprecated. The C++ API should be deprecated.

This is the (second) error that breaks the build currently: 

alpha.cc: In constructor 'Clutter::Alpha::Alpha(const
Glib::RefPtr<Clutter::Timeline>&, const SlotAlphaFunc&)':
alpha.cc:72:24: error: 'set_timeline' was not declared in this scope
   set_timeline(timeline);
Within clutter all the deprecated classes have been moved to a separate folder;
I'm guessing gmmproc has no visibility of them, as this is in 
deprecated/clutter_alpha.h:
CLUTTER_DEPRECATED_IN_1_12
void             clutter_alpha_set_timeline     (ClutterAlpha     *alpha,
                                                 ClutterTimeline  *timeline);

This was the first error;

../cluttermm/actor.h: In member function 'void
Clutter::Actor::set_shader_param(const Glib::ustring&, const ParamType&)':
../cluttermm/actor.h:2542:9: error: 'class Clutter::Actor' has no member named
'set_shader_param_value'
   this->set_shader_param_value(param, param_value);
  
 the set_shader_param() declaration has disappeared from clutter_actor.h, and isn't in deprecated/clutter_actor.h.  I've commented out the wrap( a hand-coded template) for now.

Yes, we needed to tell h2defs.py about the new headers in clutter/deprecated. I've regenerated the .defs and made some more minor changes. It now builds, for both "make all" and "make check".

You might now provide patches for the new API that you've been working on.

Maybe then you'd like to move on to wrapping clutter-2.0, so you could then just remove deprecated API. If so, you should first change configure.ac to use the clutter-2.0 library.

Hi,
I'm not getting it to build.
The minor changes you refer to seem to be the ClutterMatrix -> CoglMatrix, and a set_shader_param function that won't build, and a dynamic cast _CONVERSION for all the event types back to ClutterEvent*, but then the build is failing for me with this sort of error:

behaviour-depth.cc: In static member function 'static Glib::RefPtr<Clutter::BehaviourDepth> Clutter::BehaviourDepth::create(const Glib::RefPtr<Clutter::Alpha>&, int, int)':
behaviour-depth.cc:136:88: error: no matching function for call to 'Clutter::BehaviourDepth::BehaviourDepth(const Glib::RefPtr<Clutter::Alpha>&, int&, int&)'
   return Glib::RefPtr<BehaviourDepth>( new BehaviourDepth(alpha, start_depth, end_depth) );

As the behaviour-* classes are deprecated (along with Clutter::Alpha), I have zero interest in altering them so it compiles.  My solution originally was to remove these classes from the filelist.am for this reason.

(In reply to comment #44)
> Hi,
> I'm not getting it to build.

Do you mean git master of clutterm with no local changes? You could check for locale changes by doing
  git diff origin/master

If you mean that you can't get your own changes to build, but you have no interest in making your own changes (for this) to build, I guess I can try to fix your patch if you upload it.

Created attachment 271349 [details] [review]
regenerated clutter_enums.defs, including the deprecated classes and functions.

Created attachment 271398 [details] [review]
Adds documentation for the Action abstract base class.  Class is otherwise up to date.

Can you please check that I'm documenting the class appropriately.  Looking at the gtkmm docs I don't see that I should be doing anything differently in terms of symbols to create crosslinks, and this seems to build with the appropriate links.

Comment on attachment 271398 [details] [review]
Adds documentation for the Action abstract base class.  Class is otherwise up to date.

Thanks. Pushed. And I made a commit with small improvements to it.

You are right that you don't need to do anything special to make doxygen link to API that you mention.

Created attachment 271402 [details] [review]
Clutter::Point is a basic type (that seems to have replaced Clutter::Knot).

The next time I'm working in it, will it be OK if I go through the convert_clutter.m4 file and put the conversions into alphabetical (class) order?  It's a bit of a random guess as to where things have been put in it at present, and I find it easier to work on if I can see what's already there.

Review of attachment 271402 [details] [review]:

Some minor comments. Sorry.

::: clutter/src/types.hg
@@ +84,3 @@
+/** A Clutter::Point is a 2D point in space.
+ *
+ * It is used to return points, either in absolute coordinates or relative

There should be a comma after "relative", I think.

@@ +85,3 @@
+ *
+ * It is used to return points, either in absolute coordinates or relative
+ * depending on the function.

Maybe "function" should be changed to "method".

@@ -83,0 +83,6 @@
+class Point
+/** A Clutter::Point is a 2D point in space.
+ *
... 3 more ...

You only need a */ on that last line.

@@ +91,3 @@
+  _IGNORE(clutter_point_init, clutter_point_copy, clutter_point_free)
+
+  public:

Please use the same indentation as the existing code in this file.

@@ +95,3 @@
+     * Returns a Clutter::Point.
+     * */
+    Point(float x = 0, float y = 0);

This should have an explicit keyword to avoid an implicit conversion from 0 to Point.

Also, "returns a" isn't great in the documentation for a constructor. It instantiates rather than returns. And it's kind of obvious that it's a constructor, so I wouldn't personally bother saying that.

@@ +101,3 @@
+    #m4end
+
+    _IGNORE(clutter_point_equals)

Why have you ignored this. Maybe you could mention the reason in a comment for the next person who sees the file.

@@ -83,0 +83,23 @@
+class Point
+/** A Clutter::Point is a 2D point in space.
+ *
... 20 more ...

There should be a space between the & and the b.
This should be const.

(In reply to comment #50)
> The next time I'm working in it, will it be OK if I go through the
> convert_clutter.m4 file and put the conversions into alphabetical (class)
> order?

OK, but please do that as a separate commit/patch.

Created attachment 271410 [details] [review]
Adds the ClickAction class to the build

ClickAction class.
- Should I have put both the enums in this patch, or should the ModifierType enum be on its own (since it doesn't belong purely to this class)?
- The handwrapped function has a static_cast in it.  Is there a better way of getting a float from a gfloat?

Review of attachment 271410 [details] [review]:

Just some small fixes needed.

::: clutter/src/click-action.ccg
@@ +22,3 @@
+{
+
+Point ClickAction::get_coords()

Is there any particular reason to change the method signature compared to the C function? That reason should be mentioned in a comment.

Also, this should be const.

@@ +24,3 @@
+Point ClickAction::get_coords()
+{
+	gfloat x;

Please use two spaces for indentation.

@@ +27,3 @@
+	gfloat y;
+	clutter_click_action_get_coords(gobj(), &x, &y);
+	return Point(static_cast<float> (x), static_cast<float> (y));

The static_cast<>s should not be necessary.

::: clutter/src/click-action.hg
@@ +27,3 @@
+
+/**
+ * ClickAction is a sub-class of Clutter::Action that implements the logic for

Please use the C++ names rather than the C names in documentation.

@@ +46,3 @@
+ * LongPressState values; to handle long presses you should connect to
+ * the “long-press” signal and handle the different states.
+ * */

You only need */ here.

@@ +61,3 @@
+public:
+
+	_WRAP_CREATE()

Please use just two spaces for indentation.

@@ +63,3 @@
+	_WRAP_CREATE()
+
+	_WRAP_METHOD(guint get_button(),  clutter_click_action_get_button)

This should be const.

@@ +64,3 @@
+
+	_WRAP_METHOD(guint get_button(),  clutter_click_action_get_button)
+	_WRAP_METHOD(ModifierType get_state(), clutter_click_action_get_state)

This should be const too.

Created attachment 271414 [details] [review]
Point class version 2

Don't be sorry.  I get the rationale.

The reason I wrapped the clutter_point_equals how I did: that's what happens in every BOXEDTYPE class I could find that had an equals method.  I'm not 100% sure of the reason, particularly because we seem to get around it by using #m4 code.

Review of attachment 271414 [details] [review]:

::: clutter/src/types.ccg
@@ +98,3 @@
+Point::Point(float x, float y)
+{
+	ClutterPoint* point = clutter_point_alloc();

Indentation.

::: clutter/src/types.hg
@@ -83,0 +83,18 @@
+class Point
+/** A Clutter::Point is a 2D point in space.
+ *
... 15 more ...

I don't know why, or if, the #m4begin and #m4end are needed, but they are indeed used elsewhere.

@@ -83,0 +83,22 @@
+class Point
+/** A Clutter::Point is a 2D point in space.
+ *
... 19 more ...

This still needs to be const.

Created attachment 271508 [details] [review]
Adds the Point class.

Not sure about the wording on the second constructor's comment.  I got a bit concerned with the alterations, and wrote a very simple test file (it's coming); that revealed a problem with the constructor, which I've fixed by adding an explicit constructor for int parameters.

Also, should the float& conversion functions be in the convert_clutter.m4 file or only local?

Created attachment 271512 [details] [review]
Adds the point class.

Previous patch fails when trying to create a point with double values due to ambiguous constructor.  This adds an explicit constructor for doubles.

Created attachment 271515 [details] [review]
adds a test file for the Point class.

I can't see anywhere in the *mm that uses the gtk test harness, so I'm assuming it's not useful.  Is there a better way of doing this?

Comment on attachment 271512 [details] [review]
Adds the point class.

Well done. That's annoying but I can't see a way around it. Demanding the use of only floats in the application code would be annoying and awkward.

It is OK to keep most conversions in the .m4 file so they can be reused. We only make them local when they are not correct in all cases.

Comment on attachment 271515 [details] [review]
adds a test file for the Point class.

Thanks. This was definitely useful to show the constructor issues. I fixed another problem by modifying the test a little:
https://git.gnome.org/browse/cluttermm/commit/?id=1cc41f641d5356542a2c6672809db7aef4b5de2b

By the way, constructors only need "explicit" if they could be called with just one parameter.

(In reply to comment #59)
> I can't see anywhere in the *mm that uses the gtk test harness, so I'm assuming
> it's not useful.  Is there a better way of doing this?

I don't think anyone has ever investigated to see whether it would be useful, so feel free if you are interested.

I also notice that we don't actually run any of these tests during "make check" so we cannot even catch crashes due to null pointer dereferences.

Created attachment 271807 [details] [review]
Updates the tests/Makefile to run tests, and updates the interval tests for this.

I'll leave looking at the gtk test harness due to the risk of getting sucked into too many things without finishing any of them.
This patch adds a TESTS parameter to the Makefile, and adds the tests to this.  test-interval-creation.cc is then updated so that failure of a test will create some output.

Created attachment 271822 [details] [review]
updates gitignore for the test output files.

broke the previous patch into it's separate components.

Created attachment 271823 [details] [review]
tests/Makefile.am modified to allow tests easier.

renames the executables so they are able to be selected as a glob in .gitignore that doesn't include the source files.
Adds a TESTS variable so the tests run automatically on make_check.

Created attachment 271824 [details] [review]
reworks itnterval test file so output is useful with make check.

Created attachment 271825 [details] [review]
Point class test file updated to use make_check, adds a test for int constructor equality.

Created attachment 271826 [details] [review]
Adds the ClickAction class to the build

Created attachment 271829 [details] [review]
updates the Stage class.

Any comments about the use of Point?  There's a number of functions in other Clutter classes that return an x and a y value; my impression is that the "C++ way" of doing this is to return a Point, rather than have to create two parameters to pass in as a reference.

Created attachment 271830 [details] [review]
Basic Clutter::Stage responding to clicks.

I hope nobody minds me borrowing the code from the "programming with cluttermm" book, but the initial ones at least form good tests of function.

Review of attachment 271823 [details] [review]:

Sorry, but I'd rather keep the executable names the same as the source file names, and I prefer a prefix to a suffix.

Comment on attachment 271824 [details] [review]
reworks itnterval test file so output is useful with make check.

Pushed. This seems to fail for me, though I have not tried to check that clutter is working generally here:

[murrayc@murrayc-ThinkPad-X220 cluttermm (master *)]$ ./tests/test-interval-creation 
Computing a value with factor = 0.5: 15

Creating Clutter::Interval with Glib::Value<double> values: 10.35, 20.73
Error computing a value with factor = 0.7: 17.616; correct value is 15.54

Comment on attachment 271826 [details] [review]
Adds the ClickAction class to the build

This has too many unrelated changes and it's not clear what it is meant to do.

Comment on attachment 271829 [details] [review]
updates the Stage class.

Is this meant to do anything more than just add the documentation and the _IGNORE()? If not, the commit message should say what the commit does.

I'd generally prefer not to change the API much from the C API, but this seems fine and you get to nudge things to how you like them because you are doing the work.

Created attachment 271840 [details] [review]
had a value transposed incorrectly.  Corrected.

Review of attachment 271830 [details] [review]:

::: examples/example-stage.cc
@@ +1,1 @@
+// c++ -Wall -g `pkg-config --cflags --libs cluttermm-1.0` -o stage test-stage.cc

Please don't remove the copyright/license header:
https://git.gnome.org/browse/cluttermm_tutorial/tree/examples/stage/main.cc

@@ +29,3 @@
+
+    // Get the stage and set its size and color:
+    Glib::RefPtr<Clutter::Stage> stage = Clutter::Stage::create();

I get this compiler error when building this.

/opt/gnome/include/glibmm-2.4/glibmm/refptr.h: In instantiation of ‘Glib::RefPtr<T_CppObject>::RefPtr(const Glib::RefPtr<T_CastFrom>&) [with T_CastFrom = Clutter::Group; T_CppObject = Clutter::Stage]’:
example-stage.cc:31:65:   required from here
/opt/gnome/include/glibmm-2.4/glibmm/refptr.h:237:32: error: invalid conversion from ‘Clutter::Group*’ to ‘Clutter::Stage*’ [-fpermissive]
   pCppObject_ (src.operator->())


The old code in cluttermm_tutorial used Clutter::Stage::get_default(), probably because that was from a time when Clutter only allowed on stage. But Clutter::Stage::create() doesn't exist, so your code calls Group::create().

Clutter::Stage::get_default() still exists. I guess this should just call that for now.

@@ +46,3 @@
+   std::cerr << "Exception: " << error.what() << std::endl;
+    return EXIT_FAILURE;
+  }

The indentation is wrong here. I think you used tabs instead of two spaces. Please also put spaces before and after <<, and please use std::cerr for errors.

You might also use normal sentence capitalization, such as "Returned an error.", though I personally find these lines superfluous.

@@ +47,3 @@
+    return EXIT_FAILURE;
+  }
+	std::cout<< "returned no error."<<std::endl;

Indentation.

Comment on attachment 271840 [details] [review]
had a value transposed incorrectly.  Corrected.

Thanks.

Please don't forget to do a "git pull --rebase".

Created attachment 271852 [details] [review]
Wraps the new API for the Stage class

Review of attachment 271852 [details] [review]:

::: clutter/src/stage.hg
@@ -30,1 @@
 

These indentation changes are unnecessary and unrelated.

@@ -34,3 @@
 public:
-  Perspective(Cogl::Fixed fovy, Cogl::Fixed aspect, Cogl::Fixed z_near, Cogl::Fixed z_far);
-

Good. It's weird that this changed in the C API though I would have thought it was an ABI break:
https://git.gnome.org/browse/clutter/commit/clutter/clutter-stage.h?h=clutter-1.12&id=52811b240feba30ad1640294655a9f3086fe07d6

@@ -62,3 +63,3 @@
 class Stage :
   public Group
 {

If the class "implements" the interface then it should also derive from it. You can see this lots in the gtkmm .hg files.

@@ -68,0 +72,11 @@
+/**
+* Creates a new stage. Every Clutter::Actor must be placed on a stage, and the
+* stage first created automatically occupies the window generated when
... 8 more ...

This never returns a NULL.

@@ -68,0 +72,13 @@
+/**
+* Creates a new stage. Every Clutter::Actor must be placed on a stage, and the
+* stage first created automatically occupies the window generated when
... 10 more ...

Please use our @newin thing - for instance, @newin{0,8}

@@ -89,2 +114,3 @@
   _WRAP_METHOD(bool get_user_resizable() const, clutter_stage_get_user_resizable)
 
+  _WRAP_METHOD(void set_accept_focus(bool value), clutter_stage_set_accept_focus)

We like to give an = true default value for simple boolean set_*() methods.

@@ -97,2 @@
   _WRAP_METHOD(void set_key_focus(const Glib::RefPtr<Actor>& actor), clutter_stage_set_key_focus)
-  void set_key_focus();

I don't see any reason to remove the set_key_focus() method overload. It can still take NULL:
https://developer.gnome.org/clutter/stable/ClutterStage.html#clutter-stage-set-key-focus

@@ +163,3 @@
+  //TODO: C++ify, although does this have a use in application code?
+  // n.b. RectangleInt isn't wrapped in cairomm
+  _WRAP_METHOD(void get_redraw_clip_bounds( cairo_rectangle_int_t* clip) const,clutter_stage_get_redraw_clip_bounds)

If we must wrap this, let's at least use a reference rather than a pointer, and we should use the cairomm Rectangle type:
http://cairographics.org/documentation/cairomm/reference/namespaceCairo.html#a5a956a3226f8c0cece695c49f453e124

Created attachment 271859 [details] [review]
stage example

In my defence, I cut and pasted the example, and only adjusted the functions that needed updating- but I know now not to assume the indentation will be up to scratch!
The file header: Sorry about the oversight.  None of the test/ example files seem to have one- is that on purpose?  I'm happy adding one to each new file, and don't care who gets their email on it.  For files that are completely reworked, do I just leave it alone, or should I be changing the date?

(In reply to comment #80)
> Created an attachment (id=271859) [details] [review]
> stage example
> 
> In my defence, I cut and pasted the example, and only adjusted the functions
> that needed updating- but I know now not to assume the indentation will be up
> to scratch!

OK. I'll push this once we've added Stage::create().

> The file header: Sorry about the oversight.  None of the test/ example files
> seem to have one- is that on purpose?  I'm happy adding one to each new file,
> and don't care who gets their email on it.  For files that are completely
> reworked, do I just leave it alone, or should I be changing the date?

No problem. Yes, we should add those headers to all files some time, but don't worry about it for existing files for now.

(In reply to comment #79)
> Review of attachment 271852 [details] [review]:
.
> @@ -34,3 @@
>  public:
> -  Perspective(Cogl::Fixed fovy, Cogl::Fixed aspect, Cogl::Fixed z_near,
> Cogl::Fixed z_far);
> -
> 
> Good. It's weird that this changed in the C API though I would have thought it
> was an ABI break:
> https://git.gnome.org/browse/clutter/commit/clutter/clutter-stage.h?h=clutter-1.12&id=52811b240feba30ad1640294655a9f3086fe07d6
I think the plan is to get rid of the class in 2.0
> 
> @@ -62,3 +63,3 @@
>  class Stage :
>    public Group
>  {
> 
> If the class "implements" the interface then it should also derive from it. You
> can see this lots in the gtkmm .hg files.
Yes.  I don't derive here because Clutter::Group derives from these two interfaces, so they're inherited through that.  I was uncertain whether to add _IMPLEMENTS_INTERFACE, but as they're mentioned in the C docs thought it was probably necessary.
> 
> @@ -68,0 +72,11 @@
> +/**
> +* Creates a new stage. Every Clutter::Actor must be placed on a stage, and the
> +* stage first created automatically occupies the window generated when
> ... 8 more ...
> 
> This never returns a NULL.
I cut and pasted the C docs with some modifications.  The problem is that the docs are inconsistent with the code; Stage::get_default is deprecated, but the docs continue to refer to it.  Current state seems to be that you need to keep track of your stages within the application code; Clutter no longer does it for you.
I'm not sure what happens if a program calls create() a second time, but I suspect if the C function returns null it's not likely to be pretty.

Created attachment 271950 [details] [review]
wraps Clutter::Stage.

Comments required:
1) I've left the generated get_minimum_size(guint&, guint&) and added the handwrapped version returning a Point.  Is this more acceptable?  
2) As per the code comments, clutter_feature_available needs to be wrapped.  Does the code go in clutter_main.h?
3) I've ignored get_redraw_clip_bounds- it removes this classes' dependency on cairomm, and I can't think of a use case in application code.

(In reply to comment #82)
> Yes.  I don't derive here because Clutter::Group derives from these two
> interfaces, so they're inherited through that.  I was uncertain whether to add
> _IMPLEMENTS_INTERFACE, but as they're mentioned in the C docs thought it was
> probably necessary.

Ah, OK, good, then I don't think the _IMPLEMENTS_INTERFACEs are necessary here either. Feel free to add a comment about them being in the base classes already.

> I cut and pasted the C docs with some modifications.  The problem is that the
> docs are inconsistent with the code; Stage::get_default is deprecated, but the
> docs continue to refer to it.

Well, that could be a bug in the cluttermm documentation. Maybe we can fix that later. Let's assume that get_default() is really deprecated and move on.

> I'm not sure what happens if a program calls create() a second time, but I
> suspect if the C function returns null it's not likely to be pretty.

I'm sure it's OK now to create two stages. I guess that NULL only happens in strange cases. But we certainly can't say that a constructor can return NULL, because a C++ constructor can't do that. Let's add a TODO to investigate how to deal with a NULL from the C code.

(In reply to comment #83)
> Created an attachment (id=271950) [details] [review]
> wraps Clutter::Stage.
> 
> Comments required:
> 1) I've left the generated get_minimum_size(guint&, guint&) and added the
> handwrapped version returning a Point.  Is this more acceptable?

I suppose that's OK.

> 2) As per the code comments, clutter_feature_available needs to be wrapped. 
> Does the code go in clutter_main.h?

Yes, I guess it could go there. But you can leave that for later.

> 3) I've ignored get_redraw_clip_bounds- it removes this classes' dependency on
> cairomm, and I can't think of a use case in application code.

OK. Doing things later is OK.

Created attachment 272087 [details] [review]
updates Stage.

Comment on attachment 272087 [details] [review]
updates Stage.

Thanks. I've pushed that minus the (apparently) irrelevant whitespace changes to the documentation block, and I've pushed a small commit to correct the whitespace in some of the other changes.

Comment on attachment 271859 [details] [review]
stage example

Pushed, thanks, but I had to massage the examples/Makefile.am part to apply it. Please make any general change to the structure of examples/Makefile.am in a separate commit, if you like.

Created attachment 272254 [details] [review]
adds Animatable class, and supporting requirements to maintain the build.

I'd be interested in opinion on the templates.  Is there a better way of doing this?

Also, there's a lot of changes.  I've tried to keep the patch to the absolute minimum required to add the class.  If there's a better way to add the class without wrecking the build, I need some guidance as to how I should break the patch up.

Created attachment 272256 [details] [review]
reorganisation of convert_clutter.m4 so the class macros are in alphabetical order.

I suspect this falls into whitespace changes, but from my perspective it's a functional problem: I can't find what conversion macros are missing easily.

Created attachment 272357 [details] [review]
Adds a TESTS variable to the /tests/Makefile.am, allowing automated testing

Created attachment 272358 [details] [review]
add rules to .gitignore to prevent any files in /tests except source and Makefile from version control

Automake tests generates a log file for each test, and an overall test log, as well as the test executables themselves.  This adds to .gitignore all files in /tests except source code and the Makefile.am

Comment on attachment 272254 [details] [review]
adds Animatable class, and supporting requirements to maintain the build.

Pushed, though I had to make the .m4 changes manually because the patch would not apply cleanly with "git am".

I made some improvements in subsequent commits.

This did not break the build and was generally fine.

Review of attachment 272256 [details] [review]:

I'd gladly push this if I could "git am" it against the current git master.

Review of attachment 272357 [details] [review]:

This causes GUI windows to be opened during "make check". And even worse, it needs use input to close them so that "make check" can finish. That would be a problem in most test environments such as package builders. Maybe there are some tests that we can run during "make check" and some that we should not.

Comment on attachment 272358 [details] [review]
add rules to .gitignore to prevent any files in /tests except source and Makefile from version control

Pushed, but again I had to make some changes manually because this patch was not against git master.

Created attachment 272428 [details] [review]
as per file name.

My apology; I think this one should apply cleanly.

Created attachment 272433 [details] [review]
Adds a TESTS variable, but only adds to it tests that will run without user input.

Created attachment 272453 [details] [review]
updates the ActorBox class, including moving the code to types.h

Created attachment 272454 [details] [review]
Test file for ActorBox class, also updates /tests/Makefile.am to add the test run.

Created attachment 272466 [details] [review]
Adds the Size basic type.

Created attachment 272467 [details] [review]
Test file for Size class, and associated modifications to tests/Makefile.am

The tests exposed a limitation with overloading the constructor to take a double- an object created with double parameters stores the values as floats, so the stored value does not compare equal to the original.  I'm uncertain whether this means I should be removing the Size(double, double) constructor, as that creates its own set of problems.  I've left it in, as the lesser of two evils.  
Comment?
This will also apply to the Point class, of course.

Comment on attachment 272428 [details] [review]
as per file name.

Thanks. Maybe you could use the filename that git format-patch produces as the "Description" here. That makes thing slightly simpler.

Review of attachment 272453 [details] [review]:

Your commit message mentions a change to an .m4 file but there is no such change in the commit, and nothing that would need that change.

The commit message also mentions a change to the Geometry constructors, but I see only a change in the use of the constructors.

::: clutter/src/actor.ccg
@@ -52,3 @@
 Geometry Actor::get_geometry() const
 {
-  Geometry geom;

This change should not be necessary. I don't see anything here that should stop gmmproc from generating the Geometry default constructor.

::: clutter/src/actor.hg
@@ -37,3 @@
 class Animation;
 
-class ActorBox

If ActorBox is now used somewhere other than Actor (where?) then maybe it should just go in its own actorbox.hg/ccg files.

Comment on attachment 272466 [details] [review]
Adds the Size basic type.

Again, I think this should probably go in its own .hg\ccg files.

That's probably true of some other stuff that's already in types.hg so it's not your fault for following its lead.

(In reply to comment #104)
> Review of attachment 272453 [details] [review]:
> 
> Your commit message mentions a change to an .m4 file but there is no such
> change in the commit, and nothing that would need that change.

There's an m4 conversion missing then.  Trying to build without it fails.

> 
> The commit message also mentions a change to the Geometry constructors, but I
> see only a change in the use of the constructors.

Sorry, message was too vague then.  Moving the ActorBox class breaks the build because the Geometry constructor no longer works with no parameters.  I didn't look into it because Geometry is deprecated, and all its methods are replaced in new code; adding a couple of parameters fixed the build.

> 
> ::: clutter/src/actor.ccg
> @@ -52,3 @@
>  Geometry Actor::get_geometry() const
>  {
> -  Geometry geom;
> 
> This change should not be necessary. I don't see anything here that should stop
> gmmproc from generating the Geometry default constructor.
> 
> ::: clutter/src/actor.hg
> @@ -37,3 @@
>  class Animation;
> 
> -class ActorBox
> 
> If ActorBox is now used somewhere other than Actor (where?) then maybe it
> should just go in its own actorbox.hg/ccg files.

I was following clutter on the location of the class definition.  Currently it's used in the LayoutManager::allocate method (and related vfunc) as well.

My view is that the clutter code appears to periodically replace these basic classes- it's gone from Knot to Point and Size, and Geometry to Rect - and/ or duplicating function, as in Rect, ActorBox and to a lesser degree PaintVolume.  The backend code might have improved, but there's no significant difference in the API between classes.  I think cluttermm should just mimic what clutter is doing for ease of maintaining the wrapper.

Created attachment 272511 [details] [review]
updates clutter_docs.xml

Autogenerated output from docextract-to-xml.py

I'd also like to update the Doxyfile- it outputs a message indicating this needs doing every time it runs, and also the default font is no longer shipped with doxygen.  However, doxyfile is currently in .gitignore, and I'm not sure why.

Created attachment 272542 [details] [review]
Adds to .gitignore /examples/* except source code and Makefile.am

Created attachment 272543 [details] [review]
Adds Doxyfile to source control.

Created attachment 272544 [details] [review]
Adds the updated Doxyfile.

Comment on attachment 272543 [details] [review]
Adds Doxyfile to source control.

Doxyfile is a generated file. Doxyfile.in is the source.

Comment on attachment 272544 [details] [review]
Adds the updated Doxyfile.

Doxyfile is a generated file. Doxyfile.in is the source.

(In reply to comment #106)
> (In reply to comment #104)
> > Review of attachment 272453 [details] [review] [details]:
> > 
> > Your commit message mentions a change to an .m4 file but there is no such
> > change in the commit, and nothing that would need that change.
> 
> There's an m4 conversion missing then.  Trying to build without it fails.

I modified your commit to add it. Actually, I used the fairly-new (in gmmroc) output parameter feature, using {>>} and _INITIALIZATION(). Otherwise we'd have to hand-code that method to handle the output parameter.

> > The commit message also mentions a change to the Geometry constructors, but I
> > see only a change in the use of the constructors.
> 
> Sorry, message was too vague then.  Moving the ActorBox class breaks the build
> because the Geometry constructor no longer works with no parameters.
[snip]

That doesn't make much sense. I've removed that change from the commit.

I also removed the change of parameter names from x1 to x_1, etc.

I don't know if we should remove these getters and setters for x2 and y2, but let's see how it goes.

I made these changes while the class was still in actor.hg/ccg, so we could make the actual changes more obvious. This is what I committed:
https://git.gnome.org/browse/cluttermm/commit/?id=61abca2a3dbc2c9ff964fe8bb180db87b213d6f5

I made the constructor explicit in a subsequent commit.

 
> > If ActorBox is now used somewhere other than Actor (where?) then maybe it
> > should just go in its own actorbox.hg/ccg files.
> 
> I was following clutter on the location of the class definition.  Currently
> it's used in the LayoutManager::allocate method (and related vfunc) as well.

layout-manager.h already includes actor.h, so the move wasn't really necessary. However, I like to have one class per file, so I moved it to its own file:
https://git.gnome.org/browse/cluttermm/commit/?id=38fa575d93dd2aa91b104b04cc57db1116ce7b22

> My view is that the clutter code appears to periodically replace these basic
> classes- it's gone from Knot to Point and Size, and Geometry to Rect - and/ or
> duplicating function, as in Rect, ActorBox and to a lesser degree PaintVolume. 
> The backend code might have improved, but there's no significant difference in
> the API between classes.  I think cluttermm should just mimic what clutter is
> doing for ease of maintaining the wrapper.

Clutter doesn't need to tell use where to put our class definitions.

I agree that the API churn in clutter seems a bit arbitrary, but I haven't investigated that in detail, and it's not relevant to this particular issue.

As much as possible, please put new things, such as the Doxfile, changes in a separate bug report. I hope that we can close this big messy bug down soon but that will be hard as long as there are changes that we haven't committed yet.

Comment on attachment 272466 [details] [review]
Adds the Size basic type.

Pushed, but I moved the class to its own .hg/ccg files in a subsequent commit.

I'm seeing some linker errors when running the new tests, but I'm willing to believe that it's something on just my system:

/home/murrayc/checkout/gnome/cluttermm/tests/.libs/lt-test-actor-box: symbol lookup error: /home/murrayc/checkout/gnome/cluttermm/tests/.libs/lt-test-actor-box: undefined symbol: _ZNK7Clutter8ActorBox5get_xEv

After a git pull, the tests are still passing with no errors or warnings on mine.

Comment on attachment 272543 [details] [review]
Adds Doxyfile to source control.

I update the Doxyfile.in file instead, with doxygen -u.

Comment on attachment 272544 [details] [review]
Adds the updated Doxyfile.

I updated the Doxyfile.in file instead, with doxygen -u.

(In reply to comment #119)
> (From update of attachment 272544 [details] [review])
> I updated the Doxyfile.in file instead, with doxygen -u.


Thanks.  I'd looked at it, but doxygen -u adds all the comments back in that the Doxyfile.in didn't have, so I was assuming I'd have to do it manually- and that could wait for another bug.

Created attachment 272669 [details] [review]
Updates ActorMeta class.  

The vfunc doesn't wrap for me.

Review of attachment 272669 [details] [review]:

Pushed, with these small changes.

::: clutter/src/actor-meta.hg
@@ -26,3 +26,3 @@
 class Actor;
 
-//TODO: Write a version of the large description from here: http://clutter-project.org/docs/clutter/stable/ClutterActorMeta.html#ClutterActorMetaMeta.description
+/*********************************************************************//**

You only need /** here.

@@ -28,1 +28,8 @@
-//TODO: Write a version of the large description from here: http://clutter-project.org/docs/clutter/stable/ClutterActorMeta.html#ClutterActorMetaMeta.description
+/*********************************************************************//**
+ * Clutter::ActorMeta is an abstract class providing a common API for modifiers
+ *  of Clutter::Actor activity, appearance or layout.
... 5 more ...

I've corrected this from TRUE to true.

(In reply to comment #117)
> After a git pull, the tests are still passing with no errors or warnings on
> mine.

Yes, it's working for me now too. Sorry.

I'm also trying to mark all the deprecated API, so we can be more sure about what API needs to be added, and to make it even easier to remove the deprecated API when we branch.

Created attachment 272742 [details] [review]
Updates Animatable interface and adds new functions called by the vfuncs.

How much/ little do you want in one patch?  There are lots of changes here; I've tried to keep the patchset to one group of related changes.  I'm guessing updating the class all in one go will be too much.

Created attachment 272743 [details] [review]
Adds ActorAlign methods and enum

Created attachment 272744 [details] [review]
Adds methods related to modifying the Margin around the actor.

Created attachment 272745 [details] [review]
Adds Margin accessors.  Previous version had const in wrong methods.

Review of attachment 270229 [details] [review]:

I have pushed a small part of the changes to Actor here:
https://git.gnome.org/browse/cluttermm/commit/?id=422ac681abaddf02ccf19a746009416f50ed6aa1

The rest still needs to be split up, without the removal (yet) of the deprecated API.

Comment on attachment 272743 [details] [review]
Adds ActorAlign methods and enum

Sorry. This is the part that I just extracted for you.

Comment on attachment 272742 [details] [review]
Updates Animatable interface and adds new functions called by the vfuncs.

Pushed.

I would prefer just one patch per class, or one class and some related changes in other classes. Changes should be related.

I wouldn't mind a big patch to add lots of completely new classes as long as those are not full of many separate questions that we must resolve.

Comment on attachment 272745 [details] [review]
Adds Margin accessors.  Previous version had const in wrong methods.

Pushed. Thanks.

But what does the IM in the comment mean?

Comment on attachment 270229 [details] [review]
Wraps Actor and Actor-Meta class

I've added the add/insert/remove() methods from this patch, with const corrections:
https://git.gnome.org/browse/cluttermm/commit/?id=2fc5078cbeb37abc52f498507d2978b39e6d1d5e

(In reply to comment #131)
> (From update of attachment 272745 [details] [review])
> Pushed. Thanks.
> 
> But what does the IM in the comment mean?

Sorry.  I've put comments in my local code so I can see what I want to go back and review.  That one slipped through.
Evidently I'm having problems getting my head around git; I didn't intend to post a patch for the whole file.

(In reply to comment #133)
> Evidently I'm having problems getting my head around git; I didn't intend to
> post a patch for the whole file.

In case you don't know it already, git add -p is rather wonderful.

Hi,
I'm not sure where, but Clutter::Group got deprecated.  However, while it's disappeared from Actor, it has become an ancestor of Clutter::Stage.  I got the impression when I looked at it that it's in the process of being removed but not there yet.

(In reply to comment #135)
> Hi,
> I'm not sure where, but Clutter::Group got deprecated.  However, while it's
> disappeared from Actor, it has become an ancestor of Clutter::Stage.  I got the
> impression when I looked at it that it's in the process of being removed but
> not there yet.

Yes:
https://git.gnome.org/browse/clutter/commit/clutter/clutter-group.h?h=clutter-1.18&id=6237eb7892f9b14d4e42d7ac2262c0a42e5f8433

Thanks for pointing that out. So I've made this change:
https://git.gnome.org/browse/cluttermm/commit/?id=49fe98848326a7826af05a236a605597dd091853

If we have the same problem somewhere else then we'll probably notice it in our examples and tests, now that I've prevented them from using deprecated cluttermm API:
https://git.gnome.org/browse/cluttermm/commit/?id=7741838255e89b549f329ef32a51a521addd13d3

I'm gradually updating the examples and tests code so they can build again with that change. Hopefully that's not too awkward for you in the meantime.

I've got a series of examples built using new code:  I used the first few examples from the manual as a base for the very simple ones, then the "flowers" example from the ClutterActor class documentation, and one that used the Image class to load a file image (based on code from the ClutterBinLayout class documentation).  I heavily modified the Interval class to use templates, and have an example that runs on that also.  Probably best is I'll post them as a patch in the next 24h, on a separate bug report so they're at least visible and will save you some work, as they won't run on current state.  Now that the actor class new API is present the more basic examples should run.

Review of attachment 270237 [details] [review]:

I have pushed the Content and Image parts of this with the corrections:
https://git.gnome.org/browse/cluttermm/commit/?id=2d7119e3049a3a6039b37b1120d972e59b352d5d
and corrected myself again:
https://git.gnome.org/browse/cluttermm/commit/?id=b63c817ca3f418fda009ae8b8d06aaa078aa4936

::: clutter/src/content.hg
@@ +44,3 @@
+public:
+
+	#m4 _CONVERSION(`float&',`float*',`&($3)')

The indentation is wrong in all these files.

@@ +54,3 @@
+	#m4 _INITIALIZATION(`float&',`gfloat',`&($3)')
+  _WRAP_VFUNC(bool get_preferred_size(float& width{>>}, float& height{>>}) const, clutter_content_get_preferred_size)
+  _WRAP_VFUNC(void invalidate(),  clutter_content_invalidate)

The vfunc names are, for instance, invalidate. clutter_content_validate() is a method, for use with _WRAP_METHOD().

@@ +62,3 @@
+   */
+  void detached(const Glib::RefPtr<Actor>& actor);
+

We would need a clearly-stated reason to add methods that are not in the C API.

::: clutter/src/image.hg
@@ +31,3 @@
+
+class Image : public Glib::Object
+{

This should derive from Content too.

@@ +51,3 @@
+
+	_WRAP_METHOD_DOCS_ONLY(clutter_image_set_bytes)
+	bool set(Glib::Bytes data, CoglPixelFormat pixel_format, int width, int height, int row_stride);

You can use _WRAP_METHOD() for these, with the errthrow option.

Also, you have change the guint parameters to int, without any explanation.

And I guess that the Glib::Bytes should be const.

@@ +54,3 @@
+	/**
+	 * Sets the Image from a file location.*/
+	bool set_from_png(const Glib::ustring&	file_location);

This is another method that is not in the C API. We would need a good reason.

Also, file paths should be std::string, because they are of unknown encoding.

If we really wanted to do this then we'd need a version that took a URI (usting) too.

Review of attachment 270228 [details] [review]:

I have pushed this with several corrections, such as these, here:
https://git.gnome.org/browse/cluttermm/commit/?id=92b7cc9bcaa87469b1eea2e63b3b4e8a9ec01660

I forgot to update the commit message.

As before, I've also added them to filelist.am, of course.

I have not yet added the vfuncs definitions in the .defs file.

::: clutter/src/brightness-contrast-effect.hg
@@ +38,3 @@
+	_WRAP_METHOD(void set_brightness(float brightness), clutter_brightness_contrast_effect_set_brightness)
+	_WRAP_METHOD(void set_brightness_full(float red, float green, float blue), clutter_brightness_contrast_effect_set_brightness_full)
+	#m4 _INITIALIZATION(`float&',`float',`&($3)')

These initializations for simple types should not be necessary.

::: clutter/src/colorize-effect.hg
@@ +32,3 @@
+  #m4 _CONVERSION(`Color*' , `ClutterColor*',`$3 = Glib::wrap($4)')
+  #m4 _INITIALIZATION( `ClutterColor*',`Color' ,__RP2P)
+	_WRAP_CTOR(ColorizeEffect(Color tint), clutter_colorize_effect_new)

Color should be passed by const reference.

@@ +38,3 @@
+	_WRAP_CREATE(Color tint)
+
+	_WRAP_METHOD(void set_tint(const Glib::RefPtr<Color> tint), clutter_colorize_effect_set_tint)

Color can't be passed by RefPtr.

::: clutter/src/deform-effect.hg
@@ +46,3 @@
+	_IGNORE(clutter_deform_effect_get_back_material)
+	//, refreturn)
+	_WRAP_METHOD(void set_n_tiles(int x_tiles, int y_tiles), clutter_deform_effect_set_n_tiles)

These should be guint, not int.

@@ +58,3 @@
+	 * @return the number of tiles to deform in that direction.
+	 * */
+	guint get_n_tiles(bool xdim = true);

This is rather odd. I have wrapped it automatically like the C function instead.

Extra stuff like this can be considered separately later once the easy stuff is done.

::: clutter/src/flatten-effect.hg
@@ +20,3 @@
+namespace Clutter
+{
+	//Jan 2014: this is not implemented in clutter.h yet, so I haven't wrapped it.

Yes, this does not seem to be installed, so I have just removed these .hg/.ccg files.

::: clutter/src/offscreen-effect.hg
@@ +53,3 @@
+  _WRAP_METHOD(void paint_target(), clutter_offscreen_effect_paint_target)
+
+  _WRAP_METHOD(bool get_target_rect(Rect& rect), clutter_offscreen_effect_get_target_rect)

I doubt that this compiled without an initialization for the Rect. And I don't think we wrap ClutterRect as Rect yet. I've left this as a TODO for now.

::: clutter/src/page-turn-effect.hg
@@ +40,3 @@
+	_WRAP_METHOD(double get_angle( ), clutter_page_turn_effect_get_angle)
+	_WRAP_METHOD(void set_radius( float radius), clutter_page_turn_effect_set_radius)
+	_WRAP_METHOD(float get_radius( ), clutter_page_turn_effect_get_radius)

The get_*() methods should be const.

::: clutter/src/shader-effect.hg
@@ +36,3 @@
+  _CLASS_GOBJECT(ShaderEffect, ClutterShaderEffect, CLUTTER_SHADER_EFFECT, Clutter::OffscreenEffect, ClutterOffscreenEffect)
+
+  //_WRAP_ENUM(ShaderType, ClutterShaderType)

This doesn't seem to be wrapped anywhere else, and I needed to add a _CONV_ENUM() line in the .m4 file too.

I've learnt a lot about the codebase since writing that code.  For one, I'm sure the indentation will be all over the place.


> These initializations for simple types should not be necessary.
They seemed to be when I was working with the code.  
class Rect- patch coming in a second.

Created attachment 272900 [details] [review]
Adds the Rect basci class to types.hg

There's a custom wrap for clutter_rect_equals; I'm not sure if I was missing something simple to be able to wrap it automatically.

(In reply to comment #138)
> Review of attachment 270237 [details] [review]:
> 
> I have pushed the Content and Image parts of this with the corrections:
> https://git.gnome.org/browse/cluttermm/commit/?id=2d7119e3049a3a6039b37b1120d972e59b352d5d
> and corrected myself again:
> https://git.gnome.org/browse/cluttermm/commit/?id=b63c817ca3f418fda009ae8b8d06aaa078aa4936
> 
> ::: clutter/src/content.hg
> @@ +44,3 @@
> +public:
> +
> +    #m4 _CONVERSION(`float&',`float*',`&($3)')
> 
> The indentation is wrong in all these files.
I know now.  Sorry.

> 
> @@ +54,3 @@
> +    #m4 _INITIALIZATION(`float&',`gfloat',`&($3)')
> +  _WRAP_VFUNC(bool get_preferred_size(float& width{>>}, float& height{>>})
> const, clutter_content_get_preferred_size)
> +  _WRAP_VFUNC(void invalidate(),  clutter_content_invalidate)
> 
> The vfunc names are, for instance, invalidate. clutter_content_validate() is a
> method, for use with _WRAP_METHOD().
Yes.
> 
> @@ +62,3 @@
> +   */
> +  void detached(const Glib::RefPtr<Actor>& actor);
> +
> 
> We would need a clearly-stated reason to add methods that are not in the C API.
It is, but it wasn't wrapping cleanly at the time I was working on it.  Evidently that was a problem somewhere else in the code.

> 
> ::: clutter/src/image.hg
> @@ +31,3 @@
> +
> +class Image : public Glib::Object
> +{
> 
> This should derive from Content too.
> 
> @@ +51,3 @@
> +
> +    _WRAP_METHOD_DOCS_ONLY(clutter_image_set_bytes)
> +    bool set(Glib::Bytes data, CoglPixelFormat pixel_format, int width, int
> height, int row_stride);
> 
> You can use _WRAP_METHOD() for these, with the errthrow option.
Thanks. 

> 
> Also, you have change the guint parameters to int, without any explanation.
Stemmed from being wary of guint parameters in general.  I've had code fail before because it needs a guint passed in, not an int.  Point taken though.

> 
> And I guess that the Glib::Bytes should be const.
> 
> @@ +54,3 @@
> +    /**
> +     * Sets the Image from a file location.*/
> +    bool set_from_png(const Glib::ustring&    file_location);
> 
> This is another method that is not in the C API. We would need a good reason.

I realise it varies from the C methods.  I was trying to get to a simple way of loading a file without having to create and pass around a Byte array in user code, which set() requires.  In retrospect, the code is too complex for what it needs to be, and probably would work as a set_from_file() method anyway.  This would replace the deprecated Clutter::Texture::set_from_file method.

Created attachment 272906 [details] [review]
convenience constructor for Image class that abstracts away managing the pixbuf.

Sorry, that attachment (272906) fails to build.  The dependency on gdkmm causes problems.

Created attachment 272916 [details] [review]
Custom constructor that constructs an image using a filename.

Created attachment 272918 [details] [review]
Removes the test-actor example (uses deprecated methods); adds a basic-actor example.

I threw in a file header, but aren't sure about the copyright assignment and date given it's pretty much its a copy from the manual, only updated.

Created attachment 272919 [details] [review]
new Layout methods added to actor.hg

Created attachment 272920 [details] [review]
Adds get_ and set_z_position.

Created attachment 272935 [details] [review]
Updates LayoutManager class and expands.

Created attachment 272941 [details] [review]
copy of second example in the manual; a static actor rotated.

Created attachment 272943 [details] [review]
color class updated.

Review of attachment 272900 [details] [review]:

Pushed with these fixes. Thanks.

::: clutter/src/types.hg
@@ +119,3 @@
 };
 
+class Rect

I've added a TODO for us to move this into it's own files later.

I also added a note about how we consider clutter's normalization of input parameter as retaining constness. I filed a bug to make this simpler in clutter by hiding the struct definition: https://bugzilla.gnome.org/show_bug.cgi?id=727081

@@ +125,3 @@
+  _IGNORE(clutter_rect_alloc, clutter_rect_copy, clutter_rect_free)
+
+   public:

We don't indent public/protected/private.

@@ +128,3 @@
+  _CUSTOM_DEFAULT_CTOR
+  /** Create a new Rect object, optionally specifying its dimensions.*/
+  Rect(float x = 0, float y = 0, float width = 0, float height = 0);

This needs to be explicit.

@@ +131,3 @@
+
+  // clutter_rect_equals won't wrap as it uses nonconst values only.
+  _WRAP_METHOD_DOCS_ONLY(clutter_rect_equals)

I worked around this by creating a local cluttermm_rect_equals() that we can use instead. That keeps the code simpler and more like other operator==() in our code:

@@ +135,3 @@
+
+  _WRAP_METHOD(Rect normalise(), clutter_rect_normalize)
+  _WRAP_METHOD(Rect normalise() const, clutter_rect_normalize, constversion)

Either this should be const, or not. I think it does in fact change the this instance, so it should not be const:
https://developer.gnome.org/clutter/unstable/clutter-Base-geometric-types.html#clutter-rect-normalize

@@ +138,3 @@
+
+  _WRAP_METHOD(void get_center(Point& center), clutter_rect_get_center)
+  // alternate version that returns the centre:

Let's have one or the other.

@@ +142,3 @@
+
+  _WRAP_METHOD(bool contains_point(Point& point) const, clutter_rect_contains_point)
+  _WRAP_METHOD(bool contains_rect(Rect& b) const, clutter_rect_contains_rect)

The Point and Rect should be const here because they are not changed by the function.

@@ +143,3 @@
+  _WRAP_METHOD(bool contains_point(Point& point) const, clutter_rect_contains_point)
+  _WRAP_METHOD(bool contains_rect(Rect& b) const, clutter_rect_contains_rect)
+  _WRAP_METHOD(void rect_union(Rect& b, Rect& res) const, clutter_rect_union)

b should be const, and res is actually an output variable, which we should turn into a return.

Also, union() would be the correct method name.

@@ +144,3 @@
+  _WRAP_METHOD(bool contains_rect(Rect& b) const, clutter_rect_contains_rect)
+  _WRAP_METHOD(void rect_union(Rect& b, Rect& res) const, clutter_rect_union)
+  _WRAP_METHOD(bool intersection(Rect& b, Rect& res) const, clutter_rect_intersection )

Likewise here, thugh we need the bool to indicate if there was any intersection. I've use the {>>} syntax with an INITIALIZATION().

::: codegen/m4/convert_clutter.m4
@@ +100,3 @@
+_CONVERSION(`ClutterRect*',`Rect&',`Rect($3)')
+_CONVERSION(`ClutterRect*',`Rect',`Rect($3)')
+_CONVERSION(`Rect&',`ClutterRect*',`($3).gobj()')

Not all of these conversions were actually used. That risks strange conversions happening in some later code when we get something wrong.

Review of attachment 272919 [details] [review]:

Pushed with these fixes.
I also changed the commit message to mention the class that is changed in the commit.

::: clutter/src/actor.hg
@@ +157,3 @@
   _WRAP_METHOD(bool get_y_expand() const, clutter_actor_get_y_expand)
   _WRAP_METHOD(void set_y_expand(bool expand = true), clutter_actor_set_y_expand)
+  _WRAP_METHOD(bool needs_expand(), clutter_actor_needs_expand)

This should be const. And it takes an Orientation parameter, so this did not build.

@@ +160,3 @@
+
+  _WRAP_METHOD(void set_layout_manager(const Glib::RefPtr<LayoutManager>& manager),clutter_actor_set_layout_manager)
+  _WRAP_METHOD(Glib::RefPtr<LayoutManager> get_layout_manager(), clutter_actor_get_layout_manager)

This should have a const version.

Also, LayoutManager was not declared so this didn't build.

Review of attachment 272935 [details] [review]:

::: clutter/src/actor.hg
@@ +160,3 @@
   _WRAP_METHOD(bool get_y_expand() const, clutter_actor_get_y_expand)
   _WRAP_METHOD(void set_y_expand(bool expand = true), clutter_actor_set_y_expand)
+  _WRAP_METHOD(bool needs_expand(Orientation orientation), clutter_actor_needs_expand)

This is the correction to your previous commit, which I've already fixed.

::: clutter/src/layout-manager.hg
@@ +30,3 @@
 class Alpha;
+class ActorBox;
+class Actor;

Why are these necessary? There are no other changes in this file that seem to need this.

::: clutter/src/types.hg
@@ +40,3 @@
 _WRAP_GERROR(InitError, ClutterInitError, CLUTTER_INIT_ERROR)
 
+_WRAP_ENUM(AllocationFlags, ClutterAllocationFlags)

You've added this enum but I don't see anythiing in this patch that uses it.

Review of attachment 272943 [details] [review]:

Pushed, with these corrections.

::: clutter/src/color.hg
@@ +53,3 @@
   // TODO: this could fail, just return a 'default' color or throw an exception?
   explicit Color(const Glib::ustring& color);
+#m4 _CONVERSION(`StaticColor',`ClutterStaticColor',`($2)($3)')

A _CONV_ENUM() in the .m4 file is simpler.

@@ +54,3 @@
   explicit Color(const Glib::ustring& color);
+#m4 _CONVERSION(`StaticColor',`ClutterStaticColor',`($2)($3)')
+#m4 _INITIALIZATION(`Clutter::StaticColor',`ClutterStaticColor',`($2)($3)')

No _INITIALIZATION should be necessary here.

@@ +98,3 @@
+  #m4 _INITIALIZATION(`Glib::RefPtr<Color>',`ClutterColor',`&($3).gobj()')
+  #m4 _INITIALIZATION(`Color&',`ClutterColor',`($2)')
+  _WRAP_METHOD(void interpolate(const Color& final, double progress, Color& result), clutter_color_interpolate)

This should be const.

You shouldn't need both INITIALIZATION lines.

However, I change it to return the Color in the return result instead.

::: codegen/m4/convert_clutter.m4
@@ +40,3 @@
 
+_CONVERSION(`Color',`ClutterColor*',`($3).gobj()')
+_CONVERSION(`Color&',`ClutterColor*',`($3).gobj()')

These conversions are not necessary and not used.

Created attachment 273030 [details] [review]
Adds easing methods to actor.hg

Created attachment 273042 [details] [review]
Transitions class (base class for transitions) added, along with conversions required for build.

Created attachment 273044 [details] [review]
Adds templates for set_initial_value and set_final_value so there is no requirement to use Glib::ValueBase in user code.

Created attachment 273045 [details] [review]
Adds the PropertyTransition class

Comment please:
I'm not sure I like the way this class works, because to create a PropertyTransition means trawling the Actor class to identify what properties are available to be animated, and how they are named.  I wonder if the best way of doing this is to add a list to the class description in Clutter::Actor; the properties are included in the class documentation there, but it's not an intuitive way of working out how to create an animation.

Created attachment 273052 [details] [review]
Matrix typedef and associated methods and conversions added.

Created attachment 273053 [details] [review]
Adds Content class to generate_extra_defs.cc and cluttermm.h

Created attachment 273055 [details] [review]
Content methods in Actor class added, also associated enums and necessary conversion macros.

Created attachment 273056 [details] [review]
Adds the OffscreenRedirect methods and enum.

Created attachment 273057 [details] [review]
Adds Transition methods and required conversions.

Created attachment 273059 [details] [review]
Adds the has_key_focus method.

Created attachment 273060 [details] [review]
Adds transition class.  Previous patch was missing the include in actor.ccg

Created attachment 273061 [details] [review]
Constraint base class added.

Created attachment 273062 [details] [review]
AlignConstraint class.

Created attachment 273064 [details] [review]
BindConstraint class added.

Some functions in this (and the other constraint classes) wouldn't process properly with gmmproc, so are handwrapped.  I'm not certain why.

Created attachment 273065 [details] [review]
PathConstraint class added.

Once again, a number of methods wrapped by hand because they aren't parsed correctly by gmmproc.

Created attachment 273066 [details] [review]
SnapConstraint added.

Review of attachment 273042 [details] [review]:

Pushed with these commits.

It would be really great if you could avoid making the same mistakes again. I'm grateful for your work, but I'd like to be able to just push these patches without fixing so much simple stuff, and I hope that you can get to the point where it's safe for you to just commit directly.

::: clutter/src/transition.hg
@@ +1,1 @@
+/* Copyright (C) 2007 The cluttermm Development Team

I corrected the year here.

@@ +67,3 @@
+  @param value the value to set the final value to.
+  * @newin{1,12}
+  */

I corrected the whitespace a little on all these documentation blocks.

@@ +72,3 @@
+
+  _WRAP_METHOD(void set_interval(const Glib::RefPtr<Interval>& interval), clutter_transition_set_interval)
+  _WRAP_METHOD(Glib::RefPtr<Interval> get_interval(), clutter_transition_get_interval)

This needed refreturn and it needed a const method overload.

@@ +74,3 @@
+  _WRAP_METHOD(Glib::RefPtr<Interval> get_interval(), clutter_transition_get_interval)
+ _WRAP_METHOD(void set_animatable(const Glib::RefPtr<Animatable>& animatable), clutter_transition_set_animatable)
+  _WRAP_METHOD(Glib::RefPtr<Animatable> get_animatable() const, clutter_transition_get_animatable)

Here too. In fact, here you have a const method returning a non-const reference to this's Animatable, allowing the method to be used to change this.

@@ +84,3 @@
+
+  //vfuncs:
+  _WRAP_VFUNC(void attached(Glib::RefPtr<Animatable> animatable), attached)

These RefPtr parameters should be passed by const reference.

You have not added any vfunc definitions to the .defs file, so these _WRAP_VFUNC lines are not used yet anyway. When they are, we'll have to add appropriate _CONVERSION lines here.

I suggest leaving that for now because broken vfuncs can be very hard to debug. gmmproc warns about the missing definition, so we won't forget.

@@ +90,3 @@
+
+};
+template <class ValueType>

Please try to separate things by at least one empty line.

@@ +93,3 @@
+  void Transition::set_from(const ValueType& value)
+{
+

The empty line here is odd.

@@ +100,3 @@
+  clutter_transition_set_from_value(gobj(), set_val.gobj());
+
+  return;

This return is not necessary.

@@ +113,3 @@
+  clutter_transition_set_to_value(this->gobj(), to_val.gobj());
+
+  return;

Here too.

Review of attachment 273042 [details] [review]:

Pushed with these commits.

It would be really great if you could avoid making the same mistakes again. I'm grateful for your work, but I'd like to be able to just push these patches without fixing so much simple stuff, and I hope that you can get to the point where it's safe for you to just commit directly.

::: clutter/src/transition.hg
@@ +1,1 @@
+/* Copyright (C) 2007 The cluttermm Development Team

I corrected the year here.

@@ +67,3 @@
+  @param value the value to set the final value to.
+  * @newin{1,12}
+  */

I corrected the whitespace a little on all these documentation blocks.

@@ +72,3 @@
+
+  _WRAP_METHOD(void set_interval(const Glib::RefPtr<Interval>& interval), clutter_transition_set_interval)
+  _WRAP_METHOD(Glib::RefPtr<Interval> get_interval(), clutter_transition_get_interval)

This needed refreturn and it needed a const method overload.

@@ +74,3 @@
+  _WRAP_METHOD(Glib::RefPtr<Interval> get_interval(), clutter_transition_get_interval)
+ _WRAP_METHOD(void set_animatable(const Glib::RefPtr<Animatable>& animatable), clutter_transition_set_animatable)
+  _WRAP_METHOD(Glib::RefPtr<Animatable> get_animatable() const, clutter_transition_get_animatable)

Here too. In fact, here you have a const method returning a non-const reference to this's Animatable, allowing the method to be used to change this.

@@ +84,3 @@
+
+  //vfuncs:
+  _WRAP_VFUNC(void attached(Glib::RefPtr<Animatable> animatable), attached)

These RefPtr parameters should be passed by const reference.

You have not added any vfunc definitions to the .defs file, so these _WRAP_VFUNC lines are not used yet anyway. When they are, we'll have to add appropriate _CONVERSION lines here.

I suggest leaving that for now because broken vfuncs can be very hard to debug. gmmproc warns about the missing definition, so we won't forget.

@@ +90,3 @@
+
+};
+template <class ValueType>

Please try to separate things by at least one empty line.

@@ +93,3 @@
+  void Transition::set_from(const ValueType& value)
+{
+

The empty line here is odd.

@@ +100,3 @@
+  clutter_transition_set_from_value(gobj(), set_val.gobj());
+
+  return;

This return is not necessary.

@@ +113,3 @@
+  clutter_transition_set_to_value(this->gobj(), to_val.gobj());
+
+  return;

Here too.

Created attachment 273067 [details] [review]
Previous SnapConstraint patch didn't build; further problems with hand-wrapped methods.  Fixed or removed.

Apologies.  I thought I had built this before I posted previously.

Review of attachment 273044 [details] [review]:

::: clutter/src/interval.hg
@@ +101,3 @@
+  initial_val.init(Glib::Value<ValueType>::value_type());
+  initial_val.set(value);
+  set_initial_value(initial_val);

This would call itself indefinitely. The templated method and the non-templated methods need to have different names. Maybe rename the regular set_final_value() to set_final_value_value().

Review of attachment 273045 [details] [review]:

Pushed with the usual year, whitespace, and constness fixes.

(In reply to comment #159)
> Created an attachment (id=273045) [details] [review]
> Adds the PropertyTransition class
> 
> Comment please:
> I'm not sure I like the way this class works, because to create a
> PropertyTransition means trawling the Actor class to identify what properties
> are available to be animated, and how they are named.  I wonder if the best way
> of doing this is to add a list to the class description in Clutter::Actor; the
> properties are included in the class documentation there, but it's not an
> intuitive way of working out how to create an animation.

How would this list be different from the current list of Actor's properties?

Maybe we could let PropertyTransition::set_property_name() take a Glib::PropertyProxy or something similar, so we could do:
  propertyTransition.set_property(actor.property_something());

Review of attachment 273052 [details] [review]:

Pushed with these corrections.

::: clutter/src/actor.hg
@@ +114,3 @@
                clutter_actor_create_pango_layout)
 
+  #m4  _INITIALIZATION(`Matrix&',`ClutterMatrix*',`$3 = Glib::wrap($4)')

This isn't needed because you hand-coded the method instead.

::: clutter/src/types.hg
@@ +24,3 @@
 typedef CoglAngle Angle;
 typedef CoglFixed Fixed;
+

This change was unrelated.

::: codegen/m4/convert_clutter.m4
@@ +148,3 @@
 _CONVERSION(`Fog&',`ClutterFog*',`&($3)')
+_EQUAL(ClutterMatrix,Matrix)
+_CONVERSION(`Matrix&',`ClutterMatrix*',`&($3)')

This isn't used, so it shouldn't be here.

Review of attachment 273053 [details] [review]:

Pushed, with a corrected commit message.

Review of attachment 273055 [details] [review]:

Sorry. I added most of this last night so this didn't apply to git master. I added just the get_content_box() method from this patch.

Review of attachment 273056 [details] [review]:

::: clutter/src/actor.hg
@@ +197,3 @@
   _WRAP_METHOD(guint8 get_opacity() const, clutter_actor_get_opacity)
+  _WRAP_METHOD(void set_offscreen_redirect(OffscreenRedirect redirect),clutter_actor_set_offscreen_redirect)
+  _WRAP_METHOD(OffscreenRedirect get_offscreen_redirect(),clutter_actor_get_offscreen_redirect)

This needed to be const.

Also, please try to put a space after commas, to be consistent with the surrounding code.

Review of attachment 273060 [details] [review]:

Pushed with this fix.

::: clutter/src/actor.hg
@@ +421,3 @@
 
+  // Transitions.  Used for more complex animation handling.
+  _WRAP_METHOD(Glib::RefPtr<Transition> get_transition(const Glib::ustring& name ) const, clutter_actor_get_transition )

This shouldn't be const. And there should be a const overload.

Review of attachment 273061 [details] [review]:

Pushed with these fixes.

::: clutter/src/constraint.hg
@@ +35,3 @@
+* size of the Actor to which it is applied, by updating the actor's
+* allocation. Each Constraint can change the allocation of the actor to which
+* they are applied by overriding the Constraint::update_allocation() virtual

The actual generated vfunc is called update_allocation_vfunc(). We had to do that because there are so many "methods" in the C API that have the same name as the vfunc that they call.

@@ +54,3 @@
+
+protected:
+   _WRAP_VFUNC(void update_allocation(Glib::RefPtr<Actor>& actor, ActorBox& allocation), update_allocation)

I think the actor parameter should be a const reference here. But again, this line is not used yet anyway because this vfunc is not in a .defs file.

Review of attachment 273062 [details] [review]:

Pushed with these fixes.

::: clutter/src/align-constraint.hg
@@ +49,3 @@
+
+  _WRAP_METHOD(void set_source(const Glib::RefPtr<Actor>& source),  clutter_align_constraint_set_source)
+  _WRAP_METHOD(Glib::RefPtr<Actor> get_source() const, clutter_align_constraint_get_source)

constness.

@@ +55,3 @@
+  _WRAP_METHOD(float get_factor() const, clutter_align_constraint_get_factor )
+
+protected:

I don't see a reason to make the properties protected. vfuncs should probably be protected, when there are some.

::: clutter/src/filelist.am
@@ +19,1 @@
 	alpha.hg		\

whitespace.

Review of attachment 273064 [details] [review]:

Pushed with fixes.

::: clutter/src/bind-constraint.hg
@@ +55,3 @@
+  void set_source(const Glib::RefPtr<Actor>& source);
+
+  _WRAP_METHOD(Glib::RefPtr<Actor> get_source() const, clutter_bind_constraint_get_source)

constness and refreturn.

@@ +58,3 @@
+
+  _WRAP_METHOD_DOCS_ONLY(clutter_bind_constraint_set_coordinate)
+  void set_coordinate(BindCoordinate coordinate);

For some reason the .defs were using define-function instead of define-method. I changed the .defs by hand for now and later we can figure out why h2defs.py is doing that.

@@ +70,3 @@
+  _WRAP_PROPERTY("coordinate", BindCoordinate)
+  _WRAP_PROPERTY("offset", float)
+  _WRAP_PROPERTY(  "source", Glib::RefPtr<Actor>)

Random whitespace.

Review of attachment 273065 [details] [review]:

Pushed with the same fixes.

Comment on attachment 273067 [details] [review]
Previous SnapConstraint patch didn't build; further problems with hand-wrapped methods.  Fixed or removed.

Pushed, with the same fixes.

Thanks, but from now on please let's just focus on getting one patch at a time completely correct.

Created attachment 273124 [details] [review]
Adds ClutterConstraint to the method description.


> For some reason the .defs were using define-function instead of define-method.
> I changed the .defs by hand for now and later we can figure out why h2defs.py
> is doing that.

I wonder if it's something to do with the constraint methods for ClutterActor being declared in the ClutterConstraint header.  The methods using a constraint have to have it manually added in the .defs- patch attached.

(In reply to comment #184)
> Review of attachment 273062 [details] [review]:
> 
> Pushed with these fixes.
> 
> ::: clutter/src/align-constraint.hg
> @@ +49,3 @@
...
This doesn't seem to have made it to git/master.

Sorry. It's there now.

Created attachment 273389 [details] [review]
Adds templates for set_initial_value and set_final_value so there is no requirement to use Glib::ValueBase in user code.

Comment on attachment 273389 [details] [review]
Adds templates for set_initial_value and set_final_value so there is no requirement to use Glib::ValueBase in user code.

Pushed. Thanks.

Created attachment 273448 [details] [review]
Adds Constraint methods.

Adds Constraint methods, necessary conversions and the required Constraint property to Actor.hg

Review of attachment 273448 [details] [review]:

There are some things to improve here, and I also get this build error:

actor.cc:3629:38: error: too few arguments to function 'void clutter_actor_add_constraint(ClutterActor*, ClutterConstraint*)'
   clutter_actor_add_constraint(gobj());

I guess it's the same old function/method problem in the .defs file.

::: clutter/src/actor.hg
@@ +447,3 @@
+  #m4 _CONVERSION(`GList*',`std::vector< Glib::RefPtr< Constraint > >',`Glib::ListHandler< Glib::RefPtr< Constraint > >::list_to_vector($3, Glib::OWNERSHIP_SHALLOW)')
+  // Clutter docs: Use g_list_free() when done.
+  _WRAP_METHOD( std::vector< Glib::RefPtr< Constraint > > get_constraints() const, clutter_actor_get_constraints )

This should probably have const and non-const versions.

The const version would return a std::vector< Glib::RefPtr<const Constraint> >

@@ +449,3 @@
+  _WRAP_METHOD( std::vector< Glib::RefPtr< Constraint > > get_constraints() const, clutter_actor_get_constraints )
+ _IGNORE(clutter_actor_get_constraints)
+  _WRAP_METHOD(const Glib::RefPtr<const Constraint> get_constraint(const Glib::ustring& name) const, clutter_actor_get_constraint, refreturn, constversion)

get_constraint() should have a const and non-const version.

Also, the first const here is useless.

@@ +488,3 @@
   _WRAP_PROPERTY("clip", Geometry)
   _WRAP_PROPERTY("clip-to-allocation", bool)
+  _WRAP_PROPERTY("constraints", Glib::RefPtr<Constraint>)

This is a bit strange. It looks like, from the documentation, as if setting the constraints property actually _adds_ a constraint. I'd prefer not to wrap that. Please leave a comment like this to explain why it's commented out.

::: codegen/m4/convert_clutter.m4
@@ +45,3 @@
 _CONVERSION(`const Color&',`const ClutterColor*',`($3).gobj()')
 
+_CONVERSION(`ClutterConstraint*',`const Glib::RefPtr<const Constraint>',`Glib::wrap($3)')

The first const is useless.

Review of attachment 273448 [details] [review]:

There are some things to improve here, and I also get this build error:

actor.cc:3629:38: error: too few arguments to function 'void clutter_actor_add_constraint(ClutterActor*, ClutterConstraint*)'
   clutter_actor_add_constraint(gobj());

I guess it's the same old function/method problem in the .defs file.

::: clutter/src/actor.hg
@@ +447,3 @@
+  #m4 _CONVERSION(`GList*',`std::vector< Glib::RefPtr< Constraint > >',`Glib::ListHandler< Glib::RefPtr< Constraint > >::list_to_vector($3, Glib::OWNERSHIP_SHALLOW)')
+  // Clutter docs: Use g_list_free() when done.
+  _WRAP_METHOD( std::vector< Glib::RefPtr< Constraint > > get_constraints() const, clutter_actor_get_constraints )

This should probably have const and non-const versions.

The const version would return a std::vector< Glib::RefPtr<const Constraint> >

@@ +449,3 @@
+  _WRAP_METHOD( std::vector< Glib::RefPtr< Constraint > > get_constraints() const, clutter_actor_get_constraints )
+ _IGNORE(clutter_actor_get_constraints)
+  _WRAP_METHOD(const Glib::RefPtr<const Constraint> get_constraint(const Glib::ustring& name) const, clutter_actor_get_constraint, refreturn, constversion)

get_constraint() should have a const and non-const version.

Also, the first const here is useless.

@@ +488,3 @@
   _WRAP_PROPERTY("clip", Geometry)
   _WRAP_PROPERTY("clip-to-allocation", bool)
+  _WRAP_PROPERTY("constraints", Glib::RefPtr<Constraint>)

This is a bit strange. It looks like, from the documentation, as if setting the constraints property actually _adds_ a constraint. I'd prefer not to wrap that. Please leave a comment like this to explain why it's commented out.

::: codegen/m4/convert_clutter.m4
@@ +45,3 @@
 _CONVERSION(`const Color&',`const ClutterColor*',`($3).gobj()')
 
+_CONVERSION(`ClutterConstraint*',`const Glib::RefPtr<const Constraint>',`Glib::wrap($3)')

The first const is useless.

Created attachment 273566 [details] [review]
Adds Constraint methods to Actor class

The build failure is because there's another patch to clutter_methods.defs in the queue that is required first.  I suspect the problem is that the actor methods are declared in clutter-constraint.h, not in clutter-actor.h; in clutter_methods.defs they require a ClutterConstraint parameter to be added.

Comment on attachment 273566 [details] [review]
Adds Constraint methods to Actor class

Pushed. Thanks, though I did remove some of the unnecessary whitespace.

Is there any more API we should add before we do a tarball release and then branch so we can switch to the new clutter API/ABI (in a new bug if it needs patches) and remove all the deprecated stuff?

Yes; sorry, work is a bit busy at present.
As a minimum the Canvas class needs adding, and the missing actor vfuncs.  Canvas is the desired method for creating custom actors, so forms a reasonably central part of the API.  I'm currently (if I get a chance) trying to figure out why it's not drawing.
I assume the other missing Actor properties are nonessential given there's accessor functions for them all.
There's a number of other classes that can be added later- all the subclasses of Effect for one.

Comment on attachment 270237 [details] [review]
Drawing classes: Canvas, Color, Content and Image

I have pushed the Canvas part, with some corrections, so I think everything useful has been taken from this patch.

Comment on attachment 270237 [details] [review]
Drawing classes: Canvas, Color, Content and Image

I have pushed the Canvas part of this, with some corrections. So I think everything useful has been taken from this patch.

Comment on attachment 270238 [details] [review]
Includes Codegen files, filelist.am

I've pushed the useful parts of this:
https://git.gnome.org/browse/cluttermm/commit/?id=e69e7a634f0c18d65c0f9da09a7964bc027cb551

Comment on attachment 270229 [details] [review]
Wraps Actor and Actor-Meta class

I think we've used everything useful from this patch.

Sounds reasonable.

General question:

If the constructor for a class is protected, is it possible to derive from it usefully?  In particular, I've been looking at the Canvas example in clutter.  The sensible option is to subclass and put the on_draw override function in the subclassed Canvas; this has two problems.

First, the constructor for the derived class has to be protected.  Second, sigc takes either a pointer or a reference, and as far as I can tell doesn't seem to accept a Glib::RefPtr.  That means that anything needing to be connected via a signal handler must be created in the global namespace, or else connected via the C signals.

I'd been assuming the design decision in cluttermm to only allow access via refptr was part of the wrap.  I'm starting to wonder if it's only appropriate for abstract base classes, where it wouldn't make sense to derive from it.

(In reply to comment #204)
> (From update of attachment 270229 [details] [review])
> I think we've used everything useful from this patch.

The TODO about wrapping EventSequence:
EventSequence is a pointer to list of Events, and as C++ event objects aren't used in gtkmm or cluttermm I'm wary of touching them.  I wondered about modifying the patch in bug 135978, but that's going to have to wait until the rest of existing cluttermm is wrapped and I've got time.  Even then, EventSequence is going to be a challenge.

I notice the patch didn't use _INITIALIZATION and {>>} for InputDevice::get_device_coords (among others).  Is there a place to use it and a place not to, or is it suitable for all output parameters?

Comment on attachment 270230 [details] [review]
Several classes supporting Clutter::Actor wrapped

This was an awful mess, but I've corrected and pushed the useful stuff here:
https://git.gnome.org/browse/cluttermm/commit/?id=d2c36377003782753d23dd6a1dee25b18de6798f

and here:
https://git.gnome.org/browse/cluttermm/commit/?id=ef8c4b5451347fb288f1672277d914fdc4959fa3

Comment on attachment 270232 [details] [review]
Layout Classes wrapped

I've pushed the FlowLayout changes, with some corrections here:
https://git.gnome.org/browse/cluttermm/commit/?id=7c231227018ac425832ed02341d6bc54f8b2a565

(In reply to comment #205)
> Sounds reasonable.
> 
> General question:
> 
> If the constructor for a class is protected, is it possible to derive from it
> usefully?

Yes. Not if it's private.

[snip]
> First, the constructor for the derived class has to be protected.

Well, that's true of anything that should be used via RefPtr.

>  Second, sigc
> takes either a pointer or a reference, and as far as I can tell doesn't seem to
> accept a Glib::RefPtr.

On the contrary, that should be no problem for a parameter. But maybe you mean for the this. That can be tricky. But I am surprised that you need to worry about it. I'd have to see your code, when it's otherwise ready.

(In reply to comment #206)

> The TODO about wrapping EventSequence:
> EventSequence is a pointer to list of Events, and as C++ event objects aren't
> used in gtkmm or cluttermm I'm wary of touching them.
[snip]

Yes. It's just an opaque type that we should typedef like Matrix and PaintVolume:
https://developer.gnome.org/clutter/stable/clutter-Events.html#ClutterEventSequence

> I notice the patch didn't use _INITIALIZATION and {>>} for
> InputDevice::get_device_coords (among others).  Is there a place to use it and
> a place not to, or is it suitable for all output parameters?

It's needed for more complex types, but converting from float& to float* doesn't need any special trick. Use it if gmmproc seems to need it, I guess.

Comment on attachment 270235 [details] [review]
Several general classes including Intervals, transitions, paint, text.

I have pushed the KeyframeTransition:
https://git.gnome.org/browse/cluttermm/commit/?id=179f803669fc748d7be0cb17ad696ff908d698e1
and Text/TextBuffer
https://git.gnome.org/browse/cluttermm/commit/?id=58b6b4efba27e2c49cdd7cb9aaafef3beccebc5c
changes from this patch.

The PropertyTransition was added in a previous commit.

I have not dealt with the Path::Nodes changes from this patch.

Comment on attachment 270235 [details] [review]
Several general classes including Intervals, transitions, paint, text.

I added the GridLayout here:
https://git.gnome.org/browse/cluttermm/commit/?id=26c59eebe22c4bfcc5f2ed9c1d23e85b7155b50c

and the ScrollActor here:
https://git.gnome.org/browse/cluttermm/commit/?id=0330b11495ace9f28671755926183943431fabba

I don't think anything else from this patch is still not in git master or is still useful, though I could be wrong.

Comment on attachment 270236 [details] [review]
Backend and Device management classes

I pushed the Backend changes here with improvements:
https://git.gnome.org/browse/cluttermm/commit/?id=a9e37ede492ae0ddd4b8b9c8205239079f684e5b

The DeviceManager changes didn't seem useful, but I did simplify those files anyway:
https://git.gnome.org/browse/cluttermm/commit/?id=eb4911eddeef23e960c4d2134b4fd9b54441ca01

I pushed most of the InputDevice changes here:
https://git.gnome.org/browse/cluttermm/commit/?id=1137138adf31e2f0fbc4b2694a495272897d1a9b

The child-meta.[hg|ccg] additions seem useful. There is alrady childmeta.[hg|ccg] whose code is correct. Feel free to rename the files via a git commit.

Comment on attachment 273059 [details] [review]
Adds the has_key_focus method.

This method seems to have been added already along the way.

Comment on attachment 270239 [details] [review]
clutter::main files, event and types.  Also filelist.am

This is too much of a mess for me to deal with, though I think some of the new API has been added in the meantime.

I'll do a tarball release. I'm sure we've missed some API from your patches, but please use new bugs to submit patches for that. Thanks.

Thanks for the help.

I have now branched cluttermm, and tried to wrap cluttermm-2.0 (which still has no tarball releases) on git master. I've removed all the deprecated API without giving it much detaild thought. I can't clutter-2.0 to build without cogl linker errors (I haven't investigated) but maybe you can. Patches welcome in another bug report (please CC me).

I'm using jhbuild to build.  jhbuild is using <branch revision="clutter-1.18"/> in gnome-suites-core-deps-3.14.modules so I haven't been building against 2.0 as yet.  Looking at clutter's git log the patches seem to be coming in against 1.18- most recently 2 days ago.

Would you like to work on making our examples/ directory look like the one in clutter/examples, creating C++ versions of each of example?
https://git.gnome.org/browse/clutter/tree/?h=clutter-1.18