Code comments

[skef]

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 https://github.com/fontforge/fontforge/pull/3991#issuecomment-546970598 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.

[linusromer]

@skef I am about to solve all the issues you mentioned (but probably in a new github project as the compatibility with ff2017 will break). However, I fear that I have to ignore No. 10: I already have low level methods that operate with coordinates of points. I have worked on a intermediate level method (operating on a point living in a contour) but realized, that I will not use the intermediate level method in the high level method (operating on the whole contour) because I either had to compute many things again and again (like the length of the contour and on_curve checks) or I have to work with additional arguments (such as the length of the contour) which are actually dependent on other arguments.

[skef]

@linusromer That's perfectly fine -- this is just the stuff, as I was looking at the code, that I thought might be helpful on one level or another. That particular comment had to do with generality of application, and one can't always make code entirely general.

I just hope the feedback was more helpful than schoolmarmish.

If this winds up being integrated into a FontForge contrib directory there could be more feedback from other quarters. Obviously some folks prefer that any tool applicable to quadratic contours (in this case just harmonize, I think) be made to work with them. (I personally think that's a bit much to ask of contributed work.) Right now anything added to FontForge's contrib winds up in all menus, with no way to remove it, but if these function have their own sub-menu I doubt that would raise much concern. All of this is stuff that has been requested at one point or another.

The documented python hotkeys API is broken because it is superseded by the hotkeys file mechanism. Have you played around with any hotkeys for your own use? If this does wind up in contrib we could consider assigning default hotkeys for it.

[linusromer]

@skef Your feedback is very helpful and not schoolmarmish at all.

Regarding quadratic bezier splines: The algorithm that I am using for cubic bezier splines reduces to a simple 3-point-average for quadratic bezier splines. Just as in Fontlab, the nodes of quadratic splines will change again, if "harmonize" is executed again (the nodes will tend to the 2-point-average of the neighbour handles). On the other hand, "harmonize handles" has no meaning for quadratic bezier splines. I play with the thought of assigning the 2-point-average of the neighbour handles to "harmonize handles" for quadratic splines (and renaming "harmonize handles" to "harmonize (variant)" ). Tunnify has no meaning for quadratic bezier splines, as the handles are already balanced (so I will not print any warnings for this).

Regarding feedback from other quarters: I opened a thread at typedrawers for harmonize-tunnify-inflection.py which led to some feedback . I will update the thread as soon as I have published the new version. Maybe I should ask there for a suitable menu name and a suitable hotkey. Would you have any favorites? The provisional name of the new version is "Curvatura", because the tools are all related to curvature and "Curvatura" is understandable in many languages but still not as overused as "curvature" yet.

[skef]

I'm glad it's helpful.

I'm afraid I thought about the toolname question when I wrote that note and didn't come up with anything. ("Marketing" isn't my strong suit.)

With the hot keys obviously you don't want to grab something already taken in the defaults here, and you presumably want to match up the font-level hot keys and the char-level ones. Beyond that I would advise, maybe counter-intuitively, not using a combination that's too convenient, as FontForge might want shorter combos for future use. So I guess the ideal compromise is a set of double-modifier hotkeys not already in use that have some mnemonic relationship to the commands, however tenuous.

[linusromer]

@skef I did not get any help at typedrawers on the name "Curvatura". I took the name "Curvatura" and I have published the new version of the plugin at github.com/linusromer/curvatura.

Answers to your comments (same number):

1. No, that is not the case. The line break before that sentence may have been a bit misleading. I hope now the comments are better understandable:

The boolean is_glyph_variant is true iff the point selection in the UI does not matter.

2. Implemented now in Curvatura.py.

3. Implemented now in Curvatura.py.

4. Older versions of FontForge had a bug for fontforge.point.type, which was the only reason I did not use it. Curvatura.py now uses it.

5. With Curvatura a path that cannot be adjusted also should not be adjusted. Therefore no error message is printed.

6. Implemented now in Curvatura.py.

7. "Harmonize" and "Harmonize (variant)" both now work in Curvatura.py for quadratic bezier splines, too.

8. Implemented now in Curvatura.py.

9. Implemented now in Curvatura.py.

10. Not implemented (for the reasons mentioned before).

11. Implemented now in Curvatura.py.

12. Implemented now in Curvatura.py.

If you have any further suggestions on Curvatura.py, I would be glad to read and implement them!

[linusromer]

@skef Regarding the hotkeys: Indeed, I have played with the hotkeys. Many combinations are already taken by default. I used:

CharView.Menu.Tools.Curvatura.Harmonize:Ctrl+Shift+P
CharView.Menu.Tools.Curvatura.Harmonize (variant):Ctrl+Shift+Q
CharView.Menu.Tools.Curvatura.Add points of inflection:Ctrl+Shift+Y
CharView.Menu.Tools.Curvatura.Tunnify (balance):Ctrl+Shift+U
FontView.Menu.Tools.Curvatura.Harmonize:Ctrl+Shift+P
FontView.Menu.Tools.Curvatura.Harmonize (variant):Ctrl+Shift+Q
FontView.Menu.Tools.Curvatura.Add points of inflection:Ctrl+Shift+Y
FontView.Menu.Tools.Curvatura.Tunnify (balance):Ctrl+Shift+U

I thought that "p" and "q" are recognizable as partner. "Y" is just as in "Ynflection" and "U" as in "tUnnify".

[skef]

(I hope that) I've kicked off a discussion that could lead to this tool being released with FontForge, which right now is the best hope for wide use. (The FontForge ecosystem is pretty centralized at present. Users communicate to get help and share advice but there doesn't seem to be much tool sharing.) The idea would be to add it to the centralized (not per-user) contrib directory, which means it would be present under the Tools menu from that release until whenever it is superseded or otherwise removed. So basically part of "Default FontForge".

If and when that happens there will probably be some further change requests. I haven't looked at the new code in depth yet but I'll try to. What I have read looks very good.