Re: goocanvas notes

[Damon Chaplin <damon karuna eclipse co uk>]

On Sat, 2007-04-28 at 14:42 -0400, Havoc Pennington wrote: 
> Hi,
> 
> I read over the GooCanvas code and wrote down everything I thought of, 
> some of it is half-baked or quarter-baked, but hopefully helpful in some 
> way.

Thanks for the comments. I've replied to a lot of them below, though
I've skipped some for now as it was taking ages (I might get back to
them later). 



> - GooCanvas object could have private fields, also e.g ItemSimple has 
> public fields... I think it's nicer to always use accessors, they have 
> to be written for language binding use anyway

The GooCanvas struct is completely private at present. Everything is set
via properties or accessors.

Most of GooCanvasItemSimple and GooCanvasItemSimpleData are available
via properties or GooCanvasItem functions, e.g.
goo_canvas_item_get/set_canvas/parent/model/bounds/style/transform() and
the properties "pointer-events", "visibility", "visibility-threshold",
"can-focus", "clip-fill-rule", "clip-path".

There are only a few fields left that aren't accessible that way -
need_update, need_entire_subtree_update, own_style. Those are set as
side-effects of other changes, so shouldn't be set directly, though we
could add _get() functions.


> - set_bounds() seems to imply the bounds of the whole canvas can't just 
> be "the bounds of all the contained items" - suggest that the bounds are 
> always the contained items, but that there's a "clip" or "viewport" item 
> available that has a fixed size and clips its child items

Yes, I have thought of making the bounds settings a bit more flexible. I
think there are probably 3 main variations wanted:

 o Explicit bounds, just as we have now.
 o Implicit bounds + optional border width.
 o Implicit bounds starting from the origin + optional border width.

We could just add a function like:
  goo_canvas_set_bounds_implicit (gboolean from_origin,
                                  gdouble  border_width);


> - the "protected" functions on GooCanvas seem to imply that items have 
> to know about their containing canvas widget. This creates a pretty 
> tight linkage to the widget and makes it easier to write items that 
> can't be printed or can't be rendered elsewhere; I kind of like the 
> CanvasContext/CanvasContainer abstraction in HippoCanvas for this reason

We do have a function to render to a given cairo_t, so that should work
OK. Are there any other compelling reasons to use a canvas context? Or
could we just assume that the GooCanvas widget is the context (it
doesn't have to be displayed on screen)?


> - it seems a little clunky that CanvasItem(Model) has optional 
> interfaces embedded in it (container, transformable, stylable) - this 
> will map poorly to languages such as Java.

I suppose in Java you have to implement every interface method?

In that case I agree it might be better to have a separate container
interface. Though I'm not so sure on transformable or stylable as they
only need a few methods.


> - in HippoCanvas I'm experimenting with avoiding the visibility flag 
> (which in GTK gets checked all over the place) and instead just allow 
> 0x0 items (hidden) or items that choose not to draw (invisible). This 
> makes hidden items "just work" In a lot of cases. (I guess more 
> accurately, HippoCanvas moves the visibility flag to the container, so 
> item implementations don't have to check it. The downside of this is 
> that you can't set visibility on an item when it's outside of a container.)

GooCanvas allows items to get events even if they aren't painted, so
that wouldn't work quite as well there.


> - PathCommand seems worth making opaque
> - LineDash also

Yes.


> - it's a little strange to me to have 
> get_requested_area/get_requested_height rather than 
> get_requested_width/get_requested_height - then implement get_area by 
> just calling width then height

The disadvantage there is that items would have to keep their requested
width & height somewhere, or they'd have to recompute them for each
call.


> - should RectData (and similar such as ImageData) be in the public API?

I don't think so. Most of it should be available via GObject properties.


> - the x,y prop e.g. on CanvasText seems like it should be a child 
> property in Group rather than a property of the item ?

It could be, though you'd be complicating things a bit for people who
aren't using layout containers - they'd have to use g_object_set() for
most properties and goo_canvas_item_set_child_properties() for x and y.

You'd also be forcing all items to support explicit "x" and "y"
properties for positioning, rather than letting them determine their own
positions (e.g. relative to other items).

I think I'd rather just say that any x and y properties are ignored if a
layout container is used.


>- I have mixed feelings on the thing where the model is optional and you 
> can just use views directly ... it allows skipping the model-view 
> complexity in using the canvas if you want, but then it seems to 
> complicate the canvas item implementations

Yes, it complicates them a little, but not too much. I think it is worth
it to get the optional model-view support.

(It only complicates items if you want them to work in a simple canvas
or a model-view canvas. For people writing custom items for applications
they will choose one or the other so won't need to do that.)


> - get_requested_area would be nicer if items could return their bounds 
> in their own coordinates - in general I think it's nicer if items only 
> ever see item coordinates, there is no reason they need to know about 
> any other coordinates unless they "break the abstraction" and know about 
> the GdkWindow they are drawing to for some reason (such as doing GDK 
> calls). Certainly coordinate conversions were confusing and messy in 
> GnomeCanvas, GooCanvas cleans up most of it but it looks like 
> get_requested_area remains

I think get_requested_area() is OK. It returns coords in its parent's
space, which is its own coordinate space modified by its own transform.
(The alternative is for the item to return coords in its own coordinate
space, then for the parent to request the item's transform and convert
them, which would be less efficient and not really useful.)

However, items do store their bounds in device space. That is done for
performance reasons, so that GooCanvas doesn't need to do lots of
transformations of coords when painting or doing hit-testing.


> - simple_paint() takes a "bounds" arg but e.g. polyline and text don't 
> do anything with it; is this needed? (if it's a clip, couldn't the 
> parent just clip the cairo_t?)

More complicated items may want to skip painting parts of themselves
based on the bounds. (e.g. the audio editor and genome viewer apps I
mentioned use very large items, so will want to know exactly which
pieces they need to repaint.)


> - having x,y,width,height on canvas widget seems odd ... again x,y seem 
> like they are properties of a GtkFixed/GooCanvasGroup type of container, 
> and width/height seem redundant with size request so should be generic 
> to all items

I agree width & height are redundant.


> - I think GtkMisc having xalign/yalign as a double is sort of dumb; in 
> HippoCanvasBox I changed it to an enum fill/start/center/end which 
> results in more readable application code and a clearer API. I have 
> _never_ seen anyone use anything but 0.0, 0.5, and 1.0 as values here. 
> And adding "fill" as one of the values is also handy. Anyhow not sure I 
> would have the Table item copy GtkMisc in this oddity.

I used 0.3 for yalign in Glade once! Though it wasn't too important.
Maybe we can compromise on using an 1-byte integer percentage instead of
a double for this.


> - I'm not groking the memory management on items - shouldn't there be a 
> sink(), or use GInitiallyUnowned? I am probably missing something

Currently it uses basic reference counting, except if you create an item
with a given parent then the parent takes ownership of the initial
reference (just like GnomeCanvas).

I suppose it could use sink() etc. (though I'm not really a big fan of
the floating flag).


> - I think a Link/URL item, and/or enabling urls in the markup in the 
> Text item, would be extremely useful

Yes.


> - a gradient item would be handy

You can use gradients now, by creating a cairo pattern and setting the
"fill-pattern" or "stroke-pattern" properties of items.


> - background and border color props would be handy - this is perhaps one 
> argument for putting border in the items, instead of child props in the 
> table, though I guess the border color could be a child prop too

Though you'd probably also want border-width, x-pad & y-pad, and maybe
x-radius & y-radius, so you'd end up with quite a few properties for this.
Using a separate rect item would be simpler, though more work for the app
developer. I'm not sure about this one.


> - an "image button" item would be handy (make a button from a set of 
> images representing the button states)
> 
> - supporting prelighting on the Image item might be nice too, either as 
> a transform (lighten the pixels) or by setting a second image

Yes, I think a button item with different children for different states
would be good (maybe with optional automatic prelighting).


> - a markup for building trees of items would certainly be handy, just as 
> glade is handy

Ideally GtkBuilder would support this. Most of the standard items are
configured with GObject properties. The only issue is that GObject
doesn't have generic support for child properties, so GtkBuilder would
need special code to handle them.

Damon