Optional cross-platform outline rendering mode

[stenson]

In order to use FontGoggles as a code-only and cross-platform dependency in coldtype, I’ve refactored some code to use a new wrapper for all the mac-only operations in Lib/fontgoggles/compile and Lib/fontgoggles/font. The main outcome is that there’s now an additional RecordingPen-based code path for all of the outline-building (which I use in coldtype). Most of the new code here is in Lib/fontgoggles/misc/platform.py.

One odd thing / TODO here is that the code is now cross-platform, but the test I wrote assumes the test only runs on macOS (since it’s assuming Platform.UseCocoa will be True in the default setup). I’m not sure if this should be spelled out explicitly, since the tests only run on macOS anyway.

I’ve also wrapped the Turbo build step in a try/except since the darwin test isn't fully accurate, i.e. if you install this fork of fontgoggles into DrawBot, the build_lib.sh step fails even though it's building on mac. Obviously that's not ideal since the build step is necessary for the GUI version to work, though does ensure a more constrained environment like DrawBot or Blender python can install this without issue.

Thanks for taking a look!

[justvanrossum]

I'm not sure what to do about the "Turbo" build. Maybe it is fine for now. Ideally you'd distribute a wheel for macOS that includes the Turbo binary, but that is a pain to set up (and TBH I wouldn't know where to start at this point).

Let me know what you think about my other comments.

[justvanrossum]

I've fixed the ufo2ft problem, so if you rebase/merge main you should be able to unpin it.

[justvanrossum]

(I messed up some other dependencies in the meantime, working on it)

[justvanrossum]

Should be all good after #454

[justvanrossum]

Applogies for my slow progress on this.

I have one more request regarding the platform module: that all functions/classes that need alternate implementations aren't wrapped, but left as is, and that compatible implementations are provided in the not USE_COCOA case. This avoids some function call overhead.

For example, for the pathFromGlyph() case, I would prefer this:

if USE_COCOA:
    from ..mac.makePathFromOutline import makePathFromGlyph
else:
    def makePathFromGlyph(font, gid):
        rp = RecordingPen()
        font.draw_glyph_with_pen(gid, rp)
        return rp

There are two cases where you don't provide an alternative implementation, yet don't raise NotImplementedError either. Is that intentional? I would prefer to raise the error:

if USE_COCOA:
    from ..mac.drawing import nsColorFromRGBA
else:
    def nsColorFromRGBA(c):
        raise NotImplementedError()

makePathFromArrays() is special: your implementation uses different arguments. I would suggest to solve that like this:

if USE_COCOA:
    from ..mac.makePathFromOutline import makePathFromArrays
else:
    makePathFromArrays = None  # caller should use pen protocol instead

And then in VarGlyph.getOutline():

    def getOutline(self):
        if makePathFromArrays is not None:
            return makePathFromArrays(self.getPoints(), self.tags, self.contours)
        else:
            rp = RecordingPen()
            self.draw(rp)
            return rp

For the PlatformPen I would suggest this:

if USE_COCOA:
    PlatformPen = CocoaPen
else:
    class PlatformPen(RecordingPen):
        @property
        def path(self):
            return self

It could be one big if USE_COCOA: ... else: ... statement.

[stenson]

Hi @justvanrossum — no worries at all on timing, there's no urgency on my end as I've already published a version of the coldtype-fontgoggles idea on pypi. I’m happy to modify this PR, although I'm not sure your suggestion works for my use case, as this isn’t a classic platform-detection situation.

I use the RecordingPen implementation in Coldtype even if Coldtype is running on macOS (i.e. I set USE_COCOA at runtime to turn off the NS-based codepath on macOS, even if CAN_COCOA has returned True during setup). That way I can avoid having pyobjc be a Coldtype dependency. But as far as I can tell your suggestion would mean I wouldn’t be able to turn off the USE_COCOA codepath at all if my code is running on macOS. (I’m seeing now that that “platform” might not be the best word for the new abstraction, although I’m not sure what a better word would be.)

I think I could get your idea to work if I could override CAN_COCOA or USE_COCOA by reading an environment variable at initialization time. Although in that case I wouldn’t be able to run the current test I've added, as I wouldn't be able to change between cocoa and non-cocoa mode as the test runs, without doing some kind of importlib.reload type thing on the fontgoggles code.

Would an environment variable read at initialization + the one big if/else statement be preferable to the current runtime-modifiable code?

Sorry for the long note!

[justvanrossum]

Ahhhh yes, I understand, you're "poking" the USE_COCOA var, so this isn't a static thing at all. Let me think about this a bit more.

[stenson]

I've been thinking about this a bit myself and I realiazed the coldtype-fontgoggles fork on pypi could just hardcode USE_COCOA to be False as a patch, so it could be a fully static thing as you've outlined, i.e. in the fork USE_COCOA is always False but in the real fontgoggles repo it'd be an actual platform-detect based on CocoaPen. I have no real need for the feature to be dynamic; I think the only issue would be that it'd be impossible to test USE_COCOA=False on a mac with pytest.

[justvanrossum]

Hm, I still prefer something dynamic, also so we can easily test both.

I'm leaning to something closer to your original proposal: to have an object in platform.py that provides the functionality.

Something in this direction:

class PlatformCocoa:
    @staticmethod
    def makePathFromArrays(...):
        ...


class PlatformGeneric:
    @staticmethod
    def makePathFromArrays(...):
        ...


platform = PlatformCocoa if CAN_COCOA else PlatformGeneric


def setUseCocoa(onOff):
    global platform
    if onOff:
        assert CAN_COCOA
    platform = PlatformCocoa if onOff else PlatformGeneric


def getUseCocoa():
    return platform is PlatformCocoa

[stenson]

Works for me! Just pushed up code in that style. I think the only "odd" thing is that you can’t do from ..misc.platform import platform anymore, as the setUseCocoa won’t be able to modify that platform variable after it’s imported. Instead (as far as I can tell), you have to do something like:

import fontgoggles.misc.platform as platform
print(platform.platform) # <class 'fontgoggles.misc.platform.PlatformCocoa'>
platform.setUseCocoa(False)
print(platform.platform) # <class 'fontgoggles.misc.platform.PlatformGeneric'>

and then everything works as it should. If it’s a relative import in the style you’re using, then it doesn't seem to work:

from ..misc.platform import platform, setUseCocoa
print(platform) # <class 'fontgoggles.misc.platform.PlatformCocoa'>
setUseCocoa(False)
print(platform) # # <class 'fontgoggles.misc.platform.PlatformCocoa'>

Basically that just means there’s lots of platform.platform.pathFromArrays kind of calls, not sure if that's a dealbreaker.

[justvanrossum]

You're right, the platform.platform.xxx looks a little odd. I think I know a solution, let me try something.

[justvanrossum]

the platform variable could be a SimpleNamespace object, that we populate/update with the methods from either class. Then we never reassign that object, so from xxx.platform import platform should then work with platform.makePathEtc(), and we can still dynamically switch. A bit of a hack, but perhaps worth it.

[stenson]

Makes sense to me, I've just pushed that up and got rid of all the platform.platform stuff. This is how I ended up updating the SimpleNamespace in a single call; didn't see any other suggestions for how to do it w/o accessing the __dict__ directly.

_platform = PlatformCocoa if CAN_COCOA else PlatformGeneric
platform.__dict__.update(**_platform.__dict__)

[justvanrossum]

Yeah, that __dict__ stuff is a little hackish, but it's good enough.

One ore request: instead of the PenWrapper attribute I'd like you to take the suggestion from this comment, but then as platform.PenClass. So, no wrapping at all in the use-cocoa case.

[stenson]

Ah, sorry for missing that! Just pushed up platform.Pen — no wrapping for CocoaPen and just needed an additional init override for RecordingPen to match the CocoaPen constructor. Thanks for all the help with this!

[justvanrossum]

You can leave out the path Pen init argument: we don't use it in FG, and if we did, we'd have bigger porting problems...

[stenson]

Forgot to comment here that I deleted the pen init argument, and just now I just realized the getUseCocoa function wasn’t being tested and was incorrect, so I just pushed a fix for that.

[stenson]

Just discovered the SimpleNamespace + @staticmethod decorator won’t work on Python<=3.9 (reference), but I also see that the pinned numpy 2.1.1 requires Python>=3.10 — just wanted to check, is it safe to assume 3.9 compatibility isn’t an issue? (I’m fine requiring >=3.10 for coldtype)

[justvanrossum]

Ah, you're right. The app is currently built with 3.12, the README asks for 3.10 or up, but the (root) setup.py asks for 3.7 and up. The latter is out of date. I suppose we should bump that to 3.10 as well.

[stenson]

Sounds good to me, I’ve bumped setup.py to 3.10 on this PR.