linusromer / harmonize-tunnify-inflection Goto Github PK

This isn't a "real" issue -- I'm using this as a context for some code feedback.

Please keep in mind that while all code feedback winds up sounding like a list of criticisms that's not really what I'm doing here. In particular I'm not listing things that would be required for an eventual integration of the code in to FontForge -- I suspect such a list would be shorter. I'm just making whatever observations might be handy.

First, if you download the zip in fontforge/fontforge#3991 (comment) you'll find a sort of "model" for keeping this sort of code inside a namespace. (Given that this is python I'm sure its only one of many such models). The reason that might interest you is because it would allow your code to be used any of three ways, two of which could be useful. Suppose if at the bottom instead of running registerMenuItem() directly you were to put:

if __name__ == '__main__':
    if fontforge.hasUserInterface():
        [foo (like fontforge.registerUserInterface())]
    else:
        [bar]

In that case if your file is loaded as a python module someone could use its functionality in their own script. Then if you put code in place of [bar] it would execute when the script is run from the command line. And finally if you put code in place of foo it would run when loaded via the python config directory.

Code comments:

1. All of the "we do not care whether the points are selected in the UI" comments appear to come from an earlier implementation
2. Just passing None in place of are_contours_selected should have the same effect -- when there is no enable_function the option should always be enabled.
3. If you want this to be a contrib-esque tool rather than a "one-off" you might consider having a sub-menu. So instead of ...,"Harmonize");, ...,"Harmonize handles"); etc. you could have ...,"Your tool name", "Harmonize");, ...,"Your tool name", "Harmonize handles"); etc.
4. Your script makes no use of the fontforge.point.type field. Given how it works that may be fine, but I would recommend considering a looser test in on_same_line for smooth points (rounding can result in a larger angle divergence than .05). (Also -- and this is really picky -- given that trig functions are expensive why not base this criterion on the cross product divergence as in side?)
5. You might consider displaying an error if a "selected-for-modification" point or spline can't be adjusted rather than just skipping over it.
6. When adjusting by bulk (that is, when not using point selection) I would also recommend considering sensitivity to point type. Just because the handles on a corner point are in-line doesn't mean the user intends it to be smooth -- that could be a coincidence.
7. Your code only works for cubic contours. Nothing requires you to support quadratic contours, but it would be better to check and display a warning up front rather than relying on the structure of the data. (As it happens I made a change in a recent release to always include interpolated points on a quadratic contour, but before that you could have gotten false positives when a single interpolated point on a quadratic contour would look like [on_curve, !on_curve, !on_curve, on_curve]).
8. In the case of harmonize, wouldn't it be better to use selection of the point to be adjusted as the selected-for-modification criterion, rather than selection of it and the points on either side? The latter criterion means that six points selected in a row will result in five adjusted points when the user might only have wanted to adjust two.
9. There's a similar difficulty with spline selection, and therefore tunnify and add inflection. One option is to use selection of off-curve points to disambiguate which splines are to be adjusted. i.e.: at least one control point selected on the spline <=> this spline is to be adjusted. (The selected field works for these points as of a recent release.)
10. In terms of code organization, I would think it would be better to have your various "substantial" functions keyed off of contour and offset rather than iterating through the contour themselves. That way you can separate the questions "what points/splines are to be operated on?" from "How do we operate on this point/spline?". That also means someone using your code as a module (if that were to be possible) could call your function in the same way rather than fiddling around with point selection. (You could still leave the "is this point valid to adjust" part of the question in the operate-on code.
11. None of your contour loops are attending to c.closed. The data structure will normally save you here, but someone could add two off-curve points to the end of an open contour and you could wind up with a false positive.
12. Why the transform/psMat code on ll. 260-263? Points have an x and a y -- if you're worried about needing more than one line just write a little python function that takes a point and the two deltas as arguments.

Those are my thoughts for the time being.