diff --git a/Lib/fontTools/pens/reverseContourPen.py b/Lib/fontTools/pens/reverseContourPen.py index ae782da78..a3756ab17 100644 --- a/Lib/fontTools/pens/reverseContourPen.py +++ b/Lib/fontTools/pens/reverseContourPen.py @@ -62,7 +62,7 @@ def reversedContour(contour, outputImpliedClosingLine=False): if closed: # for closed paths, we keep the starting point yield firstType, firstPts - if outputImpliedClosingLine or firstOnCurve != lastOnCurve: + if firstOnCurve != lastOnCurve: # emit an implied line between the last and first points yield "lineTo", (lastOnCurve,) contour[-1] = (lastType, tuple(lastPts[:-1]) + (firstOnCurve,)) diff --git a/Tests/pens/reverseContourPen_test.py b/Tests/pens/reverseContourPen_test.py index 3b0dd9b26..c5911326e 100644 --- a/Tests/pens/reverseContourPen_test.py +++ b/Tests/pens/reverseContourPen_test.py @@ -12,6 +12,7 @@ TEST_DATA = [ ("lineTo", ((3, 3),)), # last not on move, line is implied ("closePath", ()), ], + False, # outputImpliedClosingLine [ ("moveTo", ((0, 0),)), ("lineTo", ((3, 3),)), @@ -25,30 +26,85 @@ TEST_DATA = [ ("moveTo", ((0, 0),)), ("lineTo", ((1, 1),)), ("lineTo", ((2, 2),)), - ("lineTo", ((0, 0),)), # last on move, no implied line + ("lineTo", ((3, 3),)), # last line does not overlap move... ("closePath", ()), ], + True, # outputImpliedClosingLine [ ("moveTo", ((0, 0),)), + ("lineTo", ((3, 3),)), ("lineTo", ((2, 2),)), ("lineTo", ((1, 1),)), + ("lineTo", ((0, 0),)), # ... but closing line is NOT implied ("closePath", ()), ], ), ( [ ("moveTo", ((0, 0),)), - ("lineTo", ((0, 0),)), ("lineTo", ((1, 1),)), ("lineTo", ((2, 2),)), + ("lineTo", ((0, 0),)), # last line overlaps move, explicit line ("closePath", ()), ], + False, [ ("moveTo", ((0, 0),)), ("lineTo", ((2, 2),)), ("lineTo", ((1, 1),)), - ("lineTo", ((0, 0),)), - ("lineTo", ((0, 0),)), + ("closePath", ()), # closing line implied + ], + ), + ( + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((1, 1),)), + ("lineTo", ((2, 2),)), + ("lineTo", ((0, 0),)), # last line overlaps move... + ("closePath", ()), + ], + True, + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((2, 2),)), + ("lineTo", ((1, 1),)), + ("lineTo", ((0, 0),)), # ... but line is NOT implied + ("closePath", ()), + ], + ), + ( + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((0, 0),)), # duplicate lineTo following moveTo + ("lineTo", ((1, 1),)), + ("lineTo", ((2, 2),)), + ("closePath", ()), + ], + False, + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((2, 2),)), + ("lineTo", ((1, 1),)), + ("lineTo", ((0, 0),)), # extra explicit lineTo is always emitted to + ("lineTo", ((0, 0),)), # disambiguate from an implicit closing line + ("closePath", ()), + ], + ), + ( + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((0, 0),)), # duplicate lineTo following moveTo + ("lineTo", ((1, 1),)), + ("lineTo", ((2, 2),)), + ("closePath", ()), + ], + True, + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((2, 2),)), + ("lineTo", ((1, 1),)), + ("lineTo", ((0, 0),)), # duplicate lineTo is retained also in this case, + ("lineTo", ((0, 0),)), # same result as with outputImpliedClosingLine=False ("closePath", ()), ], ), @@ -58,21 +114,37 @@ TEST_DATA = [ ("lineTo", ((1, 1),)), ("closePath", ()), ], + False, [ ("moveTo", ((0, 0),)), ("lineTo", ((1, 1),)), ("closePath", ()), ], ), + ( + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((1, 1),)), + ("closePath", ()), + ], + True, + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((1, 1),)), + ("lineTo", ((0, 0),)), + ("closePath", ()), + ], + ), ( [ ("moveTo", ((0, 0),)), ("curveTo", ((1, 1), (2, 2), (3, 3))), - ("curveTo", ((4, 4), (5, 5), (0, 0))), + ("curveTo", ((4, 4), (5, 5), (0, 0))), # closed curveTo overlaps moveTo ("closePath", ()), ], + False, [ - ("moveTo", ((0, 0),)), + ("moveTo", ((0, 0),)), # no extra lineTo added here ("curveTo", ((5, 5), (4, 4), (3, 3))), ("curveTo", ((2, 2), (1, 1), (0, 0))), ("closePath", ()), @@ -82,12 +154,44 @@ TEST_DATA = [ [ ("moveTo", ((0, 0),)), ("curveTo", ((1, 1), (2, 2), (3, 3))), - ("curveTo", ((4, 4), (5, 5), (6, 6))), + ("curveTo", ((4, 4), (5, 5), (0, 0))), # closed curveTo overlaps moveTo ("closePath", ()), ], + True, + [ + ("moveTo", ((0, 0),)), # no extra lineTo added here, same as preceding + ("curveTo", ((5, 5), (4, 4), (3, 3))), + ("curveTo", ((2, 2), (1, 1), (0, 0))), + ("closePath", ()), + ], + ), + ( [ ("moveTo", ((0, 0),)), - ("lineTo", ((6, 6),)), # implied line + ("curveTo", ((1, 1), (2, 2), (3, 3))), + ("curveTo", ((4, 4), (5, 5), (6, 6))), # closed curve not overlapping move + ("closePath", ()), + ], + False, + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((6, 6),)), # the previously implied line + ("curveTo", ((5, 5), (4, 4), (3, 3))), + ("curveTo", ((2, 2), (1, 1), (0, 0))), + ("closePath", ()), + ], + ), + ( + [ + ("moveTo", ((0, 0),)), + ("curveTo", ((1, 1), (2, 2), (3, 3))), + ("curveTo", ((4, 4), (5, 5), (6, 6))), # closed curve not overlapping move + ("closePath", ()), + ], + True, + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((6, 6),)), # the previously implied line (same as above) ("curveTo", ((5, 5), (4, 4), (3, 3))), ("curveTo", ((2, 2), (1, 1), (0, 0))), ("closePath", ()), @@ -101,6 +205,7 @@ TEST_DATA = [ ("curveTo", ((5, 5), (6, 6), (7, 7))), ("closePath", ()), ], + False, [ ("moveTo", ((0, 0),)), ("lineTo", ((7, 7),)), @@ -112,12 +217,31 @@ TEST_DATA = [ ( [ ("moveTo", ((0, 0),)), - ("qCurveTo", ((1, 1), (2, 2))), - ("qCurveTo", ((3, 3), (0, 0))), + ("lineTo", ((1, 1),)), # this line... + ("curveTo", ((2, 2), (3, 3), (4, 4))), + ("curveTo", ((5, 5), (6, 6), (7, 7))), ("closePath", ()), ], + True, [ ("moveTo", ((0, 0),)), + ("lineTo", ((7, 7),)), + ("curveTo", ((6, 6), (5, 5), (4, 4))), + ("curveTo", ((3, 3), (2, 2), (1, 1))), + ("lineTo", ((0, 0),)), # ... does NOT become implied + ("closePath", ()), + ], + ), + ( + [ + ("moveTo", ((0, 0),)), + ("qCurveTo", ((1, 1), (2, 2))), + ("qCurveTo", ((3, 3), (0, 0))), # closed qCurve overlaps move + ("closePath", ()), + ], + False, + [ + ("moveTo", ((0, 0),)), # no extra lineTo added here ("qCurveTo", ((3, 3), (2, 2))), ("qCurveTo", ((1, 1), (0, 0))), ("closePath", ()), @@ -127,12 +251,44 @@ TEST_DATA = [ [ ("moveTo", ((0, 0),)), ("qCurveTo", ((1, 1), (2, 2))), - ("qCurveTo", ((3, 3), (4, 4))), + ("qCurveTo", ((3, 3), (0, 0))), # closed qCurve overlaps move ("closePath", ()), ], + True, # <-- + [ + ("moveTo", ((0, 0),)), # no extra lineTo added here, same as above + ("qCurveTo", ((3, 3), (2, 2))), + ("qCurveTo", ((1, 1), (0, 0))), + ("closePath", ()), + ], + ), + ( [ ("moveTo", ((0, 0),)), - ("lineTo", ((4, 4),)), + ("qCurveTo", ((1, 1), (2, 2))), + ("qCurveTo", ((3, 3), (4, 4))), # closed qCurve not overlapping move + ("closePath", ()), + ], + False, + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((4, 4),)), # the previously implied line + ("qCurveTo", ((3, 3), (2, 2))), + ("qCurveTo", ((1, 1), (0, 0))), + ("closePath", ()), + ], + ), + ( + [ + ("moveTo", ((0, 0),)), + ("qCurveTo", ((1, 1), (2, 2))), + ("qCurveTo", ((3, 3), (4, 4))), # closed qCurve not overlapping move + ("closePath", ()), + ], + True, + [ + ("moveTo", ((0, 0),)), + ("lineTo", ((4, 4),)), # the previously implied line (same as above) ("qCurveTo", ((3, 3), (2, 2))), ("qCurveTo", ((1, 1), (0, 0))), ("closePath", ()), @@ -145,6 +301,7 @@ TEST_DATA = [ ("qCurveTo", ((2, 2), (3, 3))), ("closePath", ()), ], + False, [ ("moveTo", ((0, 0),)), ("lineTo", ((3, 3),)), @@ -154,14 +311,16 @@ TEST_DATA = [ ), ( [("addComponent", ("a", (1, 0, 0, 1, 0, 0)))], + False, [("addComponent", ("a", (1, 0, 0, 1, 0, 0)))], ), - ([], []), + ([], False, []), ( [ ("moveTo", ((0, 0),)), ("endPath", ()), ], + False, [ ("moveTo", ((0, 0),)), ("endPath", ()), @@ -172,6 +331,7 @@ TEST_DATA = [ ("moveTo", ((0, 0),)), ("closePath", ()), ], + False, [ ("moveTo", ((0, 0),)), ("endPath", ()), # single-point paths is always open @@ -179,10 +339,12 @@ TEST_DATA = [ ), ( [("moveTo", ((0, 0),)), ("lineTo", ((1, 1),)), ("endPath", ())], + False, [("moveTo", ((1, 1),)), ("lineTo", ((0, 0),)), ("endPath", ())], ), ( [("moveTo", ((0, 0),)), ("curveTo", ((1, 1), (2, 2), (3, 3))), ("endPath", ())], + False, [("moveTo", ((3, 3),)), ("curveTo", ((2, 2), (1, 1), (0, 0))), ("endPath", ())], ), ( @@ -192,6 +354,7 @@ TEST_DATA = [ ("lineTo", ((4, 4),)), ("endPath", ()), ], + False, [ ("moveTo", ((4, 4),)), ("lineTo", ((3, 3),)), @@ -206,6 +369,7 @@ TEST_DATA = [ ("curveTo", ((2, 2), (3, 3), (4, 4))), ("endPath", ()), ], + False, [ ("moveTo", ((4, 4),)), ("curveTo", ((3, 3), (2, 2), (1, 1))), @@ -215,10 +379,12 @@ TEST_DATA = [ ), ( [("qCurveTo", ((0, 0), (1, 1), (2, 2), None)), ("closePath", ())], + False, [("qCurveTo", ((0, 0), (2, 2), (1, 1), None)), ("closePath", ())], ), ( [("qCurveTo", ((0, 0), (1, 1), (2, 2), None)), ("endPath", ())], + False, [ ("qCurveTo", ((0, 0), (2, 2), (1, 1), None)), ("closePath", ()), # this is always "closed" @@ -237,6 +403,7 @@ TEST_DATA = [ ("qCurveTo", ((449, -3), (649, -3), (848, 171), (848, 348))), ("closePath", ()), ], + False, [ ("moveTo", ((848, 348),)), ("qCurveTo", ((848, 171), (649, -3), (449, -3), (449, -3))), @@ -260,6 +427,7 @@ TEST_DATA = [ ("lineTo", ((0, 651),)), ("closePath", ()), ], + False, [ ("moveTo", ((0, 651),)), ("lineTo", ((0, 651),)), @@ -268,13 +436,32 @@ TEST_DATA = [ ("closePath", ()), ], ), + ( + [ + ("moveTo", ((0, 651),)), + ("lineTo", ((0, 101),)), + ("lineTo", ((0, 101),)), + ("lineTo", ((0, 651),)), + ("lineTo", ((0, 651),)), + ("closePath", ()), + ], + True, + [ + ("moveTo", ((0, 651),)), + ("lineTo", ((0, 651),)), + ("lineTo", ((0, 101),)), + ("lineTo", ((0, 101),)), + ("lineTo", ((0, 651),)), # closing line not implied + ("closePath", ()), + ], + ), ] -@pytest.mark.parametrize("contour, expected", TEST_DATA) -def test_reverse_pen(contour, expected): +@pytest.mark.parametrize("contour, outputImpliedClosingLine, expected", TEST_DATA) +def test_reverse_pen(contour, outputImpliedClosingLine, expected): recpen = RecordingPen() - revpen = ReverseContourPen(recpen) + revpen = ReverseContourPen(recpen, outputImpliedClosingLine) for operator, operands in contour: getattr(revpen, operator)(*operands) assert recpen.value == expected @@ -288,7 +475,13 @@ def test_reverse_pen_outputImpliedClosingLine(): revpen.lineTo((0, 10)) revpen.lineTo((0, 0)) revpen.closePath() - assert len(recpen.value) == 4 + assert recpen.value == [ + ("moveTo", ((0, 0),)), + ("lineTo", ((0, 10),)), + ("lineTo", ((10, 0),)), + # ("lineTo", ((0, 0),)), # implied + ("closePath", ()), + ] recpen = RecordingPen() revpen = ReverseContourPen(recpen, outputImpliedClosingLine=True) @@ -297,11 +490,17 @@ def test_reverse_pen_outputImpliedClosingLine(): revpen.lineTo((0, 10)) revpen.lineTo((0, 0)) revpen.closePath() - assert len(recpen.value) == 6 + assert recpen.value == [ + ("moveTo", ((0, 0),)), + ("lineTo", ((0, 10),)), + ("lineTo", ((10, 0),)), + ("lineTo", ((0, 0),)), # not implied + ("closePath", ()), + ] -@pytest.mark.parametrize("contour, expected", TEST_DATA) -def test_reverse_point_pen(contour, expected): +@pytest.mark.parametrize("contour, outputImpliedClosingLine, expected", TEST_DATA) +def test_reverse_point_pen(contour, outputImpliedClosingLine, expected): from fontTools.ufoLib.pointPen import ( ReverseContourPointPen, PointToSegmentPen, @@ -309,24 +508,10 @@ def test_reverse_point_pen(contour, expected): ) recpen = RecordingPen() - pt2seg = PointToSegmentPen(recpen, outputImpliedClosingLine=True) + pt2seg = PointToSegmentPen(recpen, outputImpliedClosingLine) revpen = ReverseContourPointPen(pt2seg) seg2pt = SegmentToPointPen(revpen) for operator, operands in contour: getattr(seg2pt, operator)(*operands) - # for closed contours that have a lineTo following the moveTo, - # and whose points don't overlap, our current implementation diverges - # from the ReverseContourPointPen as wrapped by ufoLib's pen converters. - # In the latter case, an extra lineTo is added because of - # outputImpliedClosingLine=True. This is redundant but not incorrect, - # as the number of points is the same in both. - if ( - contour - and contour[-1][0] == "closePath" - and contour[1][0] == "lineTo" - and contour[1][1] != contour[0][1] - ): - expected = expected[:-1] + [("lineTo", contour[0][1])] + expected[-1:] - assert recpen.value == expected