[reportlab-users] Path.roundRect (patch)

[Peter Johnson]

On Mon, Aug 6, 2012 at 8:34 AM, Robin Becker <robin at reportlab.com> wrote:

> I am less enthusiastic about this patch, but not for any specific reason.

>

> First off it is not obvious which object/class should implement these

> shapes. Should canvases implement the primitives, in this case the primitive

> path operations moveTo/lineTo/curveTo/close and then have a more abstract

> class to do the ellipses/arcs rounded rectangles etc. If we are going to

> move the roundRect code into the PDFPathObject I would prefer that it should

> look like this

>

<snip>

>

> ie it should re-use its own primitives; the same could be said about all the

> other complex operations. However, the direct code in the canvas that used

> to stuff the canvas code is now being made two levels down so the initial

> moveTo that was originally

<snip>

> so there is more overhead.

>

> If we do this properly then presumably all of the other cases in the canvas

> code which can be replaced by an equivalent pathobject alternative should

> also be treated the same.

>

> I don't think I have any objection to putting a roundRect into the

> PDFPathObject, but should we be removing the faster code from Canvas?


I'm fully supportive of using the other Path primitives in roundRect.
In fact my original (external to the Path class) implementation did
exactly what you suggest but when submitting it as a patch I reverted
it to be the same as the original canvas code to not rock the boat too
much.  In my opinion the additional overhead in this case would be
miniscule and avoiding it definitely a case of premature optimization.

In general I think ReportLab could be cleaner and more extensible
throughout if it used more of its own primitive functions.. yes, they
add some amount of overhead but I bet that's overstated or can be
fixed in other ways.  So I'd be supportive of e.g. reimplementing more
Canvas functions in terms of the Path-level primitives.

As to moving roundRect specifically out to some kind of graphics
utilities module... my opinion is that it's not a bad idea but I
didn't want to propose that in the limited scope of this
backwards-compatible patch.  It's hard to justify doing this for just
one function though; what other functions might fall into this
category?

-Peter