GNOME Bugzilla – Bug 761847
Update cluttermm build from cluttermm-1-24 branch
Last modified: 2016-08-21 08:45:26 UTC
Created attachment 320844 [details] [review] configure.ac requires a version bump see bug 760309; this series of patches starts from the 1.24 branch rather than master.
Created attachment 320845 [details] [review] insert_child_at_index has a {?} parameter for the index which blocked the build. the {?} parameter defaults to a null pointer, which causes a build failure. Changed to normal parameter with index number required.
Created attachment 320846 [details] [review] separates the cluttermm definitions of Image::set() and Image::set_data() Image::set_data was wrapped to set() on the basis that the compiler would detect the difference between a guint8* and a Glib::Bytes& parameter, which is the only difference between clutter_image_set() and clutter_image_set_data(). However, this causes a runtime segfault when loading a Gdk::Pixbuf, so the wrapped methods are now separated into set() and set_data() methods in C++ as well.
Created attachment 320847 [details] [review] make Content::invalidate() public Content::invalidate() was wrapped as a private method, but needs to be called manually at times to force a redraw. Moved from protected to public.
Created attachment 321165 [details] [review] transition.hg corrections: added Activatable as interface, and corrected template code Clutter::Transition inherits from Activatable, but also needs to recognise it as an interface. set_to and set_from initialised the Glib::Value as the appropriate type, but failed to set the value; now fixed.
Created attachment 321166 [details] [review] Changes Clutter::Timeline parameters from guint to unsigned int.
Created attachment 321167 [details] [review] Adds a class comment to Clutter::Actor, loosely following the Clutter comment.
Review of attachment 320844 [details] [review]: 1.23 would be better. An even number indicates API and ABI stability.
Comment on attachment 320845 [details] [review] insert_child_at_index has a {?} parameter for the index which blocked the build. It's nice that C++11 and nullptr caught a real error.
Comment on attachment 320846 [details] [review] separates the cluttermm definitions of Image::set() and Image::set_data() Thanks. I have pushed this, though I had to edit your commit message to not have a huge first line. I'm trusting you that this is useful, though it doesn't make sense. It shouldn't be hard for the compiler to tell a pointer from a reference. A small test case, maybe even in the tests, would have shown if there is a real problem.
Review of attachment 320847 [details] [review]: Please give an example, in a comment in the code, of when this might need to be called from outside the implementation. The C documentation doesn't rule that our, but also doesn't suggest it: https://developer.gnome.org/clutter/stable/ClutterContent.html#clutter-content-invalidate
Comment on attachment 321165 [details] [review] transition.hg corrections: added Activatable as interface, and corrected template code Pushed, though I would really have preferred this to be 2 separate commits, because they are unrelated. Thanks.
Review of attachment 321166 [details] [review]: Why? guint is just a short way of saying unsigned int. We use guint in gtkmm and friends. There doesn't seem to be any corresponding change in the C API: https://developer.gnome.org/clutter/stable/ClutterTimeline.html#clutter-timeline-set-duration
(In reply to Murray Cumming from comment #9) > I'm trusting you that this is useful, though it doesn't make sense. This also broken "make check", though I've fixed that now. It would have made sense to change the example/test in the same commit as the API change.
Review of attachment 321167 [details] [review]: Thanks. Surprisingly, the markdown (?) syntax seems to work in Doxygen. But I still see some stray "|[" in the generated HTML.
(In reply to Murray Cumming from comment #9) > Comment on attachment 320846 [details] [review] [review] > separates the cluttermm definitions of Image::set() and Image::set_data() > > Thanks. I have pushed this, though I had to edit your commit message to not > have a huge first line. > > I'm trusting you that this is useful, though it doesn't make sense. It > shouldn't be hard for the compiler to tell a pointer from a reference. A > small test case, maybe even in the tests, would have shown if there is a > real problem. I've got a content example working using both an image and a Cairo::Context; without invalidating the surface during construction the context wouldn't get drawn on. I was planning on putting examples into the manual rather than the build; for one, it stops cluttermm from depending on too much, but happy to take advice on that. Does it make sense to put a minimalist example that checks for the problem in tests? And is there any FAQ on the Gtk GUI test environment? I haven't been able to find anything useful. The reason I've not posted the example to the manual is I'm trying to work my way through it, and have been stuck on containers for a week or two. I've got a solution, but I'm not happy with it yet; I think it's better posted as a separate bug and will do that shortly.
> (In reply to Murray Cumming from comment #14) > > Thanks. Surprisingly, the markdown (?) syntax seems to work in Doxygen. But > > I still see some stray "|[" in the generated HTML. I had a number of attempts at getting the code in comments to read as code, and this is as good as it got. Doxygen doesn't seem to recognise "language=C++" the same as it does "language=C", which may be part of the problem. The "|[" are an unwanted anomaly, but when I take them out the generated text is formatted as normal text, which I find much harder to read.
(In reply to Ian Martin from comment #15) > (In reply to Murray Cumming from comment #9) [snip] > > A small test case, maybe even in the tests, would have shown if there is a > > real problem. > [snip] > Does it make sense to put a minimalist > example that checks for the problem in tests? Yes, but a (unit) test is not an example. > And is there any FAQ on the > Gtk GUI test environment? I haven't been able to find anything useful. [snip] There are already a couple of tests in tests/. You can just do what is don so far. We can have useful tests, particularly for whether stuff even compiles, without testing actual UI behaviour. However, in this case, you can't put a test in the project itself for API that you want to change anyway. But it would be nice to see a small test case her in this bug report to show that your API change is really necessary.
(In reply to Ian Martin from comment #16) > > (In reply to Murray Cumming from comment #14) [snip] > The "|[" are an unwanted anomaly, but when I take them out the > generated text is formatted as normal text, which I find much harder to read. In gtkmm we just use @code and @endcode and that seems to work fine: https://git.gnome.org/browse/gtkmm/tree/gtk/src/dialog.hg#n145 Could you try that, please.
Created attachment 323041 [details] [review] Version bump to 1.23.1 As requested
Created attachment 323042 [details] Content implementation (In reply to Murray Cumming from comment #9) > Comment on attachment 320846 [details] [review] [review] [snip] > I'm trusting you that this is useful, though it doesn't make sense. It > shouldn't be hard for the compiler to tell a pointer from a reference. A > small test case, maybe even in the tests, would have shown if there is a > real problem. File attached that uses the two Content classes to fill Actors. The problems with creating a test case: 1) Image::set() was failing silently, so it all compiled fine; it was just no image was being displayed. 2) Without Context::invalidate(), the code is all fine, but draw() was never called, so again the only way to tell if it was working was to see if the image displayed or not. Currently I don't understand how to use the Gtk test harness to create GUI tests (if it can).
Created attachment 323043 [details] [review] Updated Actor class comment with code blocks correctly labelled original patch, plus changed code blocks to use @code and @endcode
I've created bug 763073 for a challenging (for me at least) Actor::allocate problem. Should I continue pushing other patches to this bug or start another?
(In reply to Ian Martin from comment #20) > File attached that uses the two Content classes to fill Actors. The > problems with creating a test case: > 1) Image::set() was failing silently, so it all compiled fine; it was just > no image was being displayed. It works fine for me when I change set_data() back to set() and use that instead. I can see no reason why the set(const Glib::Bytes& data, ...) override would be used instead when calling set() with guint* from Gdk::Pixbuf::get_pixels(). I plan to change this back. > 2) Without Context::invalidate(), the code is all fine, but draw() was never > called, so again the only way to tell if it was working was to see if the > image displayed or not. Yes, that invalidate call does seem to be necessary, though I've used it like so until make the chang in cluttermm: clutter_content_invalidate(CLUTTER_CONTENT(canvas->gobj())); However, the clutter_content_invalidate() documentation clearly says that this should not be necessary outside of the ClutterCanvas class: https://developer.gnome.org/clutter/stable/ClutterContent.html#clutter-content-invalidate So please try to create a small C test case/example that shows that this is necessary, and then file a clutter bug. In that way, we might discover that it's a cluttermm bug.
(In reply to Murray Cumming from comment #12) > Review of attachment 321166 [details] [review] [review]: > > Why? guint is just a short way of saying unsigned int. We use guint in gtkmm > and friends. > There doesn't seem to be any corresponding change in the C API: > https://developer.gnome.org/clutter/stable/ClutterTimeline.html#clutter- > timeline-set-duration You didn't reply to this. I assume that you agree so I've rejected that particular parth.
(In reply to Murray Cumming from comment #24) > (In reply to Murray Cumming from comment #12) > > Review of attachment 321166 [details] [review] [review] [review]: > > > > Why? guint is just a short way of saying unsigned int. We use guint in gtkmm > > and friends. > > There doesn't seem to be any corresponding change in the C API: > > https://developer.gnome.org/clutter/stable/ClutterTimeline.html#clutter- > > timeline-set-duration > > You didn't reply to this. I assume that you agree so I've rejected that > particular patch. Sorry. Yes, correct; if guint is appropriate in gtkmm then leave it in.
(In reply to Murray Cumming from comment #23) > (In reply to Ian Martin from comment #20) > > File attached that uses the two Content classes to fill Actors. The > > problems with creating a test case: > > 1) Image::set() was failing silently, so it all compiled fine; it was just > > no image was being displayed. > > It works fine for me when I change set_data() back to set() and use that > instead. I can see no reason why the set(const Glib::Bytes& data, ...) > override would be used instead when calling set() with guint* from > Gdk::Pixbuf::get_pixels(). I plan to change this back. > > > 2) Without Context::invalidate(), the code is all fine, but draw() was never > > called, so again the only way to tell if it was working was to see if the > > image displayed or not. > > Yes, that invalidate call does seem to be necessary, though I've used it > like so until make the change in cluttermm: > clutter_content_invalidate(CLUTTER_CONTENT(canvas->gobj())); > However, the clutter_content_invalidate() documentation clearly says that > this should not be necessary outside of the ClutterCanvas class: > > https://developer.gnome.org/clutter/stable/ClutterContent.html#clutter- > content-invalidate > So please try to create a small C test case/example that shows that this is > necessary, and then file a clutter bug. In that way, we might discover that > it's a cluttermm bug. OK, if Image::set() works for you, it's not needed. It appeared to be a required change when I was working on it. Looking again, the approved alternative to making Content::invalidate() public is to call Canvas::set_size() passing the current width and height as parameters. I'd argue that's not very intuitive (that's one reason why it took me a while to find it), but if that's the best way of keeping invalidate() private I guess it should be done. When I get time I'll add a class comment to make sure it's documented.
(In reply to Ian Martin from comment #26) > Looking again, the approved alternative to making Content::invalidate() > public is to call Canvas::set_size() passing the current width and height as > parameters. Based on what documentation or conversation?
Based on some code I can no longer find. The documentation you mention appears to have changed for clutter_content_invalidate()- there's no longer any reference to limiting its use. The clutter examples - https://git.gnome.org/browse/clutter/tree/examples/canvas.c?h=clutter-1.18 https://git.gnome.org/browse/clutter/tree/examples/rounded-rectangle.c?h=clutter-1.18 use clutter_content_invalidate(). So it looks like invalidate() should be public API.
It still says "This function should be called by ClutterContent implementations" though it explicitly doesn't forbid other use. https://developer.gnome.org/clutter/stable/ClutterContent.html#clutter-content-invalidate I would be very surprised if the ClutterCanvas API demanded its use. Please see my comment #23 about creating a test case and a bug.
So, I think I've dealt with all these patches, though comment #23 and #29 might still be interesting. If they are, someone might create a new bug about them.