Re: [Goocanvas-devel] GooCanvasTable: Grid lines between rows and columns

[Armin B. <ar...@ar...>]

On Wed, 2008-03-19 at 14:29 +0000, Damon Chaplin wrote:
> I've read through most of the patch in detail now, and I've spotted a
> few problems:
> 
> 1) goo_canvas_update_grid_line_visibility() is called before/after
>    changing child properties, to set/clear the grid lines around the
>    child. I don't think this will work, since it may set grid lines
>    that cross over other children (it doesn't take other children
>    into account).
> 
>    I think the solution is to calculate all of the grid lines in the
>    update function, e.g. set them all then step through each child
>    clearing them like goo_canvas_update_grid_line_visibility() does.

Ah. I didn't know that items can "share" cells.

> 2) goo_canvas_table_update_dimensions() has become a bit too
>    complicated, trying to figure out which arrays need renewing,
>    freeing or enlarging.
> 
>    I'd rather it just set a flag like "update_grid_arrays" if needed
>    and then the update function freed all the grid arrays and started
>    from scratch.

I don't think it's too complicated. It just makes sure that the
dynamically allocated structures are large enough for all the
rows/columns in the table. Previously, it just enlarged the spacings
array, now it additionally enlarges the grid line visibility array.
Since that array is two-dimensional, it needs to be enlarged in both
directions, though.

I don't think freeing everything and starting from scratch would make
the code that much simpler. The "Update visibility arrays for the
existing rows/columns." code would have to be replaced by code that
frees the grid line visibility and needs to be moved a bit upwards. I
don't think this get us much regarding code complexity.

Correct me if I'm wrong, but would we really need the
"update_grid_arrays" flag? Isn't the number of rows/columns having
increased (which is already used) indicating exactly this?

> And some more minor points:
> 
> 3) I'm not sure I like the property names "row-border-spacing",
>    "column-border-spacing", "row-grid-line-width", and
>    "column-grid-line-width". I find them slightly confusing.

>    Maybe "x-border-spacing", "y-border-spacing",
>    "vert-grid-line-width", "horz-grid-line-width" is more obvious.

Changed, although I normally prefer longer names over abbrevations
(horizontal-grid-line-width vs. horz-grid-line-width)

> 4) There is a minor issue with integer layout - it doesn't ensure that
>    the grid lines are on integer boundaries. Does this matter?

Hm. Would it be enough to round the grid line width to the nearest
integer for this, i.e. is everything else in the size allocation process
correctly rounded? We would need to store the grid line width still
twice then (the actual property value and the one used in calculations
which is potentially rounded), right?

Thanks for the review. New patch is at
http://arbur.net/code/goocanvas/20080325-openismus-goocanvas-gridlines3.patch.

> Damon

Armin