schriftgestalt / GlyphsSDK

Scripting SDK for Glyphs
Apache License 2.0
89 stars 37 forks source link

We need a named constant for 9223372036854775808 #79

Open justanotherfoundry opened 1 year ago

justanotherfoundry commented 1 year ago

It seems Glyph internally uses 9223372036854775808, or LLONG_MAX as ObjC calls it, in certain scenarios to indicate “no value” or similar. I believe it would be much cleaner to have a named constant for this, and proper documentation.

For example, in my ObjC code, I am checking the return value of kerningForFontMasterID:LeftKey:RightKey:direction: against LLONG_MAX to determine whether a kerning pair exists. This has always felt like a dirty hack as it is undocumented and doesn’t use a clearly named constant. The Python wrapper simply uses if value > 1000000:. That’s even worse IMHO, as if the value came from an unreliable external source, and needs a sanity check.

The unit tests check some values against 9223372036854775807 (that’s a 7 at the end, i.e. LLONG_MAX-1). Why is this? What is the meaning?

Can I kindly ask to solve this in a cleaner, less hacky way by simply providing a named constant (in ObjC as well as Python). Thanks! – Tim

florianpircher commented 1 year ago

9223372036854775807 is NSNotFound, a constant that Apple’s APIs are frequently using to indicate that there is no normal value (since in Objective-C ints, points, etc. cannot be nil).

justanotherfoundry commented 1 year ago

Thanks, that’s interesting.

This means:

That’s not satisfying, is it?

florianpircher commented 1 year ago

In some cases, Glyphs internally uses NSNotFound (which is different from LLONG_MAX)

Where are you getting 9223372036854775808? LLONG_MAX is also 9223372036854775807.

justanotherfoundry commented 1 year ago

Now, that’s confusing. It took me a while to solve that puzzle:

kerningForFontMasterID:LeftKey:RightKey:direction: returns a CGFloat a.k.a. double • in my code, checking that returned double against LLONG_MAX a.k.a. NSNotFound returns true • printing that value shows 9223372036854775808, i.e. NSNotFound + 1

Can’t be? Yes, it can. The reason is – as far as I figured out – that NSNotFound can’t be stored in a double. Apparently the method tries to return NSNotFound but it actually returns NSNotFound + 1. During my test for equality, NSNotFound is lossily cast to a double, this is why the values appear to be identical even though they aren’t. So, I was unconsciously testing whether the method returns a lossy cast of NSNotFound by comparing it to another lossy cast of NSNotFound.

To me, this shows how hacky and fragile the current solution is. I feel uncomfortable working like this.

florianpircher commented 1 year ago

The workaround is to check whether a value is equal to or less than NSNotFound. Still hacky, but there is no good solution to this in Objective-C. NaN values have their own set of problems, especially when working with Apple APIs. CGFLOAT_MAX works in some cases, but is unusual as a ‘nothing’ value on Apples platforms.

schriftgestalt commented 1 year ago

Comparing floats is always a bit tricky. That's why I picked a (random) big (but smaller than NSNotFound) number as a threshold in the python code. As far as I understand, kerning values are stored as Int16, so the max value is 0x7FFF. So any value between that and NSNotFound is fine as threshold.

justanotherfoundry commented 1 year ago

Thanks for the explanations. I wouldn’t say comparing floats is always tricky, though. It would have been easy to define a large integer value (one that can be represented by a 32-bit float) as a named constant, let’s say NO_KERNING_VALUE or UNSPECIFIED_VALUE, then all code could safely check for equality instead of using a random threshold. This would make the code robust and readable.

schriftgestalt commented 1 year ago

It might be better wrap the relevant methods so that they return None instead of NSNotFound.

But we can add a constant like MAX_KERN that you need to compare with a < instead of ==.