On Oct 14, 2008, at 6:52 PM, Peter Clifton wrote: > Nice patch series. I'll push those into the main repository if you're > asserting that they are tested, and don't cause any regressions in > hatching output. I had a few issues though - which you could fix, or I > could fix as I applied them:
Yes, I've tested these patches. However, this patch series has not been tested on a slow machine. Also, this patch series may require adjustments to improve the performance for on-screen graphics. It's been stress tested to an object with about 10k hatch lines. It may be possible to break it with larger values, but I think this would be unrealistic. > BTW.. there is no great need to add things like: > > + g_return_if_fail(toplevel != NULL); > + g_return_if_fail(fp != NULL); > > To the various functions called internally within libgeda. > > I'm not so against it that I'd take it out, but it would be nice to > define somewhere, just where we're adding these tests, and which > code-paths assume the input is sensible. > > This function is called from inside libgeda only IIRC, so we should > take > its input as not needing testing. APIs called from outside libgeda > "might" need a little more validation. I've always put in some sort of assertions. These statements are cheap in terms of execution time. I've never worried about it. Let me know if you want them to go away. > Seems there is an unrelated change here. Was this the old behaviour? > > + /* Avoid printing line widths too small */ > + if (fill_width <= 1) fill_width = 2; > > If not, please don't change it - certainly not in an unrelated patch. Those two lines were in o_box_basic.c, but missing from o_circle_basic.c. So, to be consistent, copied the code from the box to the circle. I don't really know the history of the two lines, but I've used programs that send line widths of "0," to a printer at work, and they can't be read. As I continue work, I was thinking of making the value configurable as the minimum line width. > If I applied the patches (baring the line-width change), I'd probably > merge the first two together - does that seem sensible? I didn't know if someone had applied the first patch to a branch already. The two separate patches document the changes. And if someone branched saw a problem, that it may have gotten fixed. > If you want to be able to log the progression between one and the > other > in the code-base, I'd also be OK with applying them separately, so > long > as they both compile and "basically" work. The first patch needs the second patch to work. No problems except for the appearance of hatched shapes. Just visual problems, no crashing. > As I've mentioned before, I really like this refactor you're doing. A > natural extension is to use this for rendering other objects in the > GUI. > Could we talk before you spend too much time writing that code > though.. > I'm hoping cairo is the way forward, so it would be good to coordinate > on top of that. However.. keeing GDK (for now) is also on the table. > > If we do use this code to support the current GDK rendering, out of > selfishness, I'd like to know what is happening with the code so it > doesn't cause too much unexpected breakage against the cairo rendering > branch. Yes, I kept the first two patches separate for this reason. Also, before continuing with more modifications, I'll post the function prototypes as an API first. Cheers, Ed _______________________________________________ geda-dev mailing list [email protected] http://www.seul.org/cgi-bin/mailman/listinfo/geda-dev
