GNOME Bugzilla – Bug 610370
[cairo] Add a cairo module
Last modified: 2010-03-02 18:53:15 UTC
Adds a Context and ImageSurface wrappers and integration with the overrides. Adds a couple of example. This is far from complete, it's merely demostration roughly what a cairo binding will look like
Created attachment 154143 [details] [review] [cairo] Add a cairo module
*** Bug 571633 has been marked as a duplicate of this bug. ***
Design/overview review would be appreciated. I'm also looking for what kind of coverage we should except before pushing this to master. Adding unittests and valgrind checking is also something that still needs to be done.
Created attachment 154449 [details] [review] Add a cairo module This is a squash of the cairo branch for easier reviewing. See http://git.gnome.org/browse/gjs/log/?h=cairo for all the individual commits. This patch adds examples, documentation, patterns and many more methods to context.
Review of attachment 154449 [details] [review]: Generally looks good. (Much easier to review than I expected.) Various leaks, complaints about whitespace, etc, below. I think the biggest question would be StudlyCaps vs. separated_words - I don't have a big opinion ... just want to make sure we give this maximum thought before we decide a year from now that it really should have been consistent with the introspection bound modules. (I guess how DBus is handled is another data point.) I haven't reviewed: - How it relates to introspection (how we decide how to handle a cairo_t as a cairo_t, etc.) - The macros that you are using to define classes - I just assumed they worked as they looked like there were supposed to work. I've seen discussion on both of those on other bugs, so hopefully you have the necessary feedback. ::: Makefile-modules.am @@ +75,3 @@ +cairoNative_la_CFLAGS = \ + $(JS_NATIVE_MODULE_CFLAGS) \ + $(GJS_CAIRO_CFLAGS) \ Hmm: - I really think the start of the line should be lined up here - I'm pretty strongly in favor of keeping the \'s lined up too. (Easy in Emacs, with C-c C-\, harder in other editors, perhaps.) gjs seems like it was good for this at one point ,but some of the recent additions have been sloppy. This patch adds much more sloppiness (and isn't even internally consistent...) ::: doc/cairo.txt @@ +12,3 @@ + +cairo_move_to is wrapped to Cairo.Context.moveTo() +cairo_surface_write_to_png to Cairo.Context.writeToPNG(). Hmm, it's going to confuse people that all the gi imported bindings are _ but this one is studlyCaps. Not a strong preference on my part, maybe opinions from other people would be good. @@ +24,3 @@ +========================== + +Prototype hierarchya typo @@ +37,3 @@ +Creating an ImageSurface from a PNG is done by calling a static method: + +gjs> let surface = Cairo.ImageSurface.createFromPNG("filename.png"); This doesn't seem to be implemented? @@ +51,3 @@ +gjs> let cr = new Cairo.Context(surface); + +gjs> let cr = new Gdk.cairo_create(...); new looks extraneous ::: examples/cairo/fill.js @@ +1,3 @@ +const Cairo = imports.cairo; + +let surface = new Cairo.ImageSurface(Cairo.Format.ARGB32, 120, 120); Probably should have a 'const SIZE = 120' @@ +3,3 @@ +let surface = new Cairo.ImageSurface(Cairo.Format.ARGB32, 120, 120); +let cr = new Cairo.Context(surface); +cr.scale(120, 120); I really dislike using cr.scale() like this - graphics are going to come out better if you work in pixel coordinates - you can't get pixel alignment right if you are working in scaled coordinates. People who copy examples who use a cr.scale() like this will have to rewrite at some point. ::: examples/cairo/paint.js @@ +6,3 @@ + +cr.setSourceRGB(0.0, 0.0, 0.0); +cr.paintWithAlpha(0.5); Hmm, if this examples are supposed to be instructive, I'm not sure how this one helps - it sort of seems that the examples are more about making sure you have everything bound correctly. Maybe they should be pared down a bit before committing, and the remaining ones commented? I think it's useful to keep examples as examples and not just random bits of code that were useful when developing. (We eventually fixed this in GTK+ with gtk-demo after years of pointing to people to the morass that was testgtk) ::: examples/cairo/poppler-gdk.js @@ +29,3 @@ + let cr = Gdk.cairo_create(window.window); + //let page = this._document.get_page(0); + //page.render(cr); Obviously needs to be fixed before landing. ::: modules/cairo-context.c @@ +31,3 @@ +#define _GJS_CAIRO_CONTEXT_DEFINE_FUNC_BEGIN(mname) \ +static JSBool \ +mname##_func(JSContext *context, \ Gigantic macros get so much easier to read when the \'s are lined up - you can actually see the whole thing as a block @@ +46,3 @@ +_GJS_CAIRO_CONTEXT_DEFINE_FUNC_BEGIN(method) \ + if (argc > 0) { \ + gjs_throw(context, "Context." #method "() requires no arguments"); \ 'requires no arguments' is a bit funny english, though 'requires 2 arguments' is fine. 'takes no arguments' would be better. Or use the wording from gjs_parse_args() - 'Error invoking %s: Expected %d arguments, got %d' @@ +49,3 @@ + return JS_FALSE; \ + } \ + cr = gjs_cairo_context_get_cr(context, obj); \ probably could be done as a patch on top, but you really should be checking cairo_status (cr) and throwing after every call - the 'objects shut down' behavior is really a C-only behavior to keep error handling minimal in C. It's not friendly to programmers to have a bug in some place show up by everything subsequent not working. @@ +156,3 @@ + void *dummy; + JSContext *context; + JSObject *object; More funky spaces @@ +183,3 @@ +static JSBool +gjs_cairo_context_constructor(JSContext *context, + JSObject *obj, Indentation is off @@ +523,3 @@ + return NULL; + + _gjs_cairo_context_construct_internal(context, object, cr); Don't you need to reference cr here? (But see comment about _construct() methods in cairo-private.h) ::: modules/cairo-pdf-surface.c @@ +39,3 @@ + jsval *retval) +{ + char *filename; You leak this, it appears (careful about the early return when fixing) ::: modules/cairo-private.h @@ +37,3 @@ + +jsval gjs_cairo_context_create_proto(JSContext *context, JSObject *module, + const char *proto_name, JSObject *parent); Messy indentation - GJS seems to follow the GTK+ standard of having things lined up nicely. @@ +39,3 @@ + const char *proto_name, JSObject *parent); +cairo_t *gjs_cairo_context_get_cr(JSContext *context, + JSObject *object); This should be _get_context(), not get_cr(), cr is just a conventional variable name. (So, like you wouldn't call a function that was 'get_index()' 'get_i()' @@ +47,3 @@ +jsval gjs_cairo_surface_create_proto(JSContext *context, JSObject *module, + const char *proto_name, JSObject *parent); +void gjs_cairo_surface_construct(JSContext *context, JSObject *obj, cairo_surface_t *surface); Maybe all the _construct() functions should reference their argument, I think the scope where adopting an argument is OK is really confined to static methods within a single file. @@ +53,3 @@ + +/* image surface */ +#ifdef CAIRO_HAS_IMAGE_SURFACE Actually, the image surface isn't optional ... it's just in CAIRO_FEATURES_H because of the use of some repeated boilerplate. (Had to ask on IRC) @@ +68,3 @@ + void *dummy; + JSContext *context; + JSObject *object; lining up in a structure isn't required, certainly don't half line up just two members ::: modules/cairo-surface.c @@ +80,3 @@ + surface = gjs_cairo_surface_get_surface(context, obj); + if (!surface) + return JS_FALSE; Leaks filename
(In reply to comment #5) > Review of attachment 154449 [details] [review]: > > I haven't reviewed: > > - How it relates to introspection (how we decide how to handle a cairo_t as a > cairo_t, etc.) I will do the cairo_t/introspection integration in a separate patch, it should be covered by bug 610357 The intention is that a prototype from the cairo module will be used when a method returns a cairo_t (or another cairo type) and that a method accepts the wrapped object when expecting an introspection/native type. > ::: doc/cairo.txt > @@ +12,3 @@ > + > +cairo_move_to is wrapped to Cairo.Context.moveTo() > +cairo_surface_write_to_png to Cairo.Context.writeToPNG(). > > Hmm, it's going to confuse people that all the gi imported bindings are _ but > this one is studlyCaps. Not a strong preference on my part, maybe opinions from > other people would be good. The limited API in Javascript is actually studlyCaps, so it makes sense to follow that convention for a natively wrapped function. We've done the same in most other non-introspection API. DBus/tweener/lang etc. Personally I'd like to see the introspection api use studlyCaps as well for consistencey, but it's too late for that and it would also create confusion. We just have to accept that some apis are separated by _ and other are studlyCaps. > @@ +37,3 @@ > +Creating an ImageSurface from a PNG is done by calling a static method: > + > +gjs> let surface = Cairo.ImageSurface.createFromPNG("filename.png"); > > This doesn't seem to be implemented? It is in my tree, will add a few extra things to the documentation. > ::: examples/cairo/fill.js These examples were ported from the tutorial at cairographics.org. I intend to re-write the whole tutorial to javascript and publish it somewhere, that's why I want these examples. Perhaps the gjs module is not the right place for these kind of examples though. > ::: examples/cairo/poppler-gdk.js [..] > Obviously needs to be fixed before landing. I'll remove the example, see above. > + cr = gjs_cairo_context_get_cr(context, obj); \ > > probably could be done as a patch on top, but you really should be checking > cairo_status (cr) and throwing after every call - the 'objects shut down' > behavior is really a C-only behavior to keep error handling minimal in C. It's > not friendly to programmers to have a bug in some place show up by everything > subsequent not working. Some abstraction api is needed on top of that, will do. Javascript is a bit messy since you can't get out the status from the error if we add exceptions. But I guess most errors should really be handled by the programmer instead of catching exceptions. > @@ +523,3 @@ > + return NULL; > + > + _gjs_cairo_context_construct_internal(context, object, cr); > > Don't you need to reference cr here? (But see comment about _construct() > methods in cairo-private.h) > Maybe all the _construct() functions should reference their argument, I think > the scope where adopting an argument is OK is really confined to static methods > within a single file. Sure, I'll reference and dereference things here where appropriate. I'll post a follow-up patch later today or tomorrow.
Created attachment 154807 [details] [review] [cairo] Add a cairo module Updates since last revision * Fixed indentation of sources/headers * Fixed tabs/spaces in makefiles * Changed ref counting for internal API * Check status after all cairo calls and raise exceptions * Add PSSurface and SVGSurface * Add SolidPattern * Add a bunch of context methods * Document internal API * Add a bunch of extra enums * Plugged a bunch of string leaks * Rename cr -> context * Improve giant macro readability * Add ImageSurface.createFromPNG
I wonder if we use javaCaps when we're hand-rolling an API in the sense that we're designing a new API for JS, and under_score when we're mechanically and exactly translating the API from C. Whether the mechanical/exact translation from C uses g-i or not is perhaps not the real issue. The question is whether JS-specific "sugar and adaptation" is allowed. I don't know though.
Havoc: We're actually not doing a mechanical translation, at least not on a struct/object level. An exact mapping would be "cairo.scale(cairo.create(), x, y)" etc, which is not really what we want in an OO language. Either way the mapping is going to be weird for someone, as we already use both studlyCaps and under_score kind of apis. I'd say that we keep the currently implemented API as it's the least amount of work and it fits better with other pure javascript apis (such as dom). The Cairo API is sort of similar to the Canvas element, but not exactly. I have future plans to implement a wrapper on top of the cairo bindings so you can easily move canvas code between gjs and the web.
Review of attachment 154807 [details] [review]: I don't see SolidPattern in here - am I missing something? Don't see any major problems with what is here. Various things noted below. ::: modules/cairo-context.c @@ +751,3 @@ +JSObject * +gjs_cairo_context_from_context(JSContext *context, + cairo_t *cr) This shouldn't adopt ownership either - since from_pattern and from_surface don't adopt, perhaps an oversight here. ::: modules/cairo-image-surface.c @@ +86,3 @@ + return JS_FALSE; + + surface = cairo_image_surface_create_from_png(filename); filename is leaked ::: modules/cairo-pattern.c @@ +128,3 @@ + * @object: object to finalize + * + * Destroys the resources assoicated with a pattern wrapper. associated ::: modules/cairo-surface-pattern.c @@ +111,3 @@ + + if (argc > 0) { + gjs_throw(context, "Context.getExtend() requires no arguments"); This is a SurfacePattern method not a Context method @@ +118,3 @@ + extend = cairo_pattern_get_extend(pattern); + + if (!gjs_cairo_check_status(context, cairo_pattern_status(pattern), "pattern")) Hmm, should getters check status - they never set it. I guess if you catch an exception, then try to access the error'ed surface, it's good to get exceptions, but they will be a bit confusing, even if the messgae in check_status() is fixed. @@ +160,3 @@ + + if (argc > 0) { + gjs_throw(context, "Context.getFilter() requires no arguments"); Same here ::: modules/cairo-surface.c @@ +75,3 @@ + return JS_FALSE; + } + cairo_surface_write_to_png(surface, filename); You need to check status here @@ +159,3 @@ + * @object: object to finalize + * + * Destroys the resources assoicated with a surface wrapper. 'associated' ::: modules/cairo.c @@ +33,3 @@ +{ + if (status != CAIRO_STATUS_SUCCESS) { + gjs_throw(context, "Could not create %s: %s", 'Could not create' seems inappropriate in many usages.
(In reply to comment #10) > Review of attachment 154807 [details] [review]: > > I don't see SolidPattern in here - am I missing something? Oh, typo, I meant SurfacePattern. I didn't do SolidPattern yet since I didn't get a clear answer on how to do the constructor. I guess I'll do SolidPattern.fromRGB() and SolidPattern.fromRGBA() with no default constructor unless someone complains. > Don't see any major problems with what is here. Various things noted below. Thanks! I'll fix up the issues mentioned below and push. I'll probably rip out the examples and put in the cairo wiki instead. > @@ +118,3 @@ > + extend = cairo_pattern_get_extend(pattern); > + > + if (!gjs_cairo_check_status(context, cairo_pattern_status(pattern), > "pattern")) > > Hmm, should getters check status - they never set it. I guess if you catch an > exception, then try to access the error'ed surface, it's good to get > exceptions, but they will be a bit confusing, even if the messgae in > check_status() is fixed. I think a confusing exception is better than no exception, so I'll keep the error checking for all getters.
(In reply to comment #11) > (In reply to comment #10) > > Review of attachment 154807 [details] [review] [details]: > > > > I don't see SolidPattern in here - am I missing something? > > Oh, typo, I meant SurfacePattern. I didn't do SolidPattern yet since I didn't > get a clear answer on how to do the constructor. Oh. sorry, I thought I tried to be clear on that - I discussed it with Carl and filed a patch for the language bindings guide at: https://bugs.freedesktop.org/show_bug.cgi?id=26758 > I guess I'll do > SolidPattern.fromRGB() and SolidPattern.fromRGBA() with no default constructor > unless someone complains. SolidPattern.createRGB(), SolidPattern.createRGBA() - the basic rule of the cairo language binding guide is no improvisation - you can't deviate from a straightforward mapping of the C names just because you have a better name.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > Review of attachment 154807 [details] [review] [details] [details]: > > > > > > I don't see SolidPattern in here - am I missing something? > > > > Oh, typo, I meant SurfacePattern. I didn't do SolidPattern yet since I didn't > > get a clear answer on how to do the constructor. > > Oh. sorry, I thought I tried to be clear on that - I discussed it with Carl and > filed a patch for the language bindings guide at: > > https://bugs.freedesktop.org/show_bug.cgi?id=26758 Thanks! > > I guess I'll do > > SolidPattern.fromRGB() and SolidPattern.fromRGBA() with no default constructor > > unless someone complains. > > SolidPattern.createRGB(), SolidPattern.createRGBA() - the basic rule of the > cairo language binding guide is no improvisation - you can't deviate from a > straightforward mapping of the C names just because you have a better name. That's what I meant, I'm not completely familiar with the exact api naming in cairo yet :-). I just wanted to point out that I'm going to create two static/class methods and no default constructor.
I pushed this upstream with unittests and a bunch of implementation fixes found by actually testing all the code. Thanks!