The C code looks fine to me. I'm concerned a bit about duplication of attributes. Could it be better to allow setting device tables like the following?

font.math.MathLeading = 10 # Set leading
font.math.MathLeading = (10, {9: -1, 10: 2}) # Set leading with device table values

This looks more concise to me,but could break assumption about attribute value type in existing scripts.
The reading of the attribute could look like

leading = font.math.MathLeading
if isinstance(leading, int):
    # Treat like single value
else:
    # Treat like a tuple
    default_lead, lead_devtable = leading
    lead_10px = lead_devtable[10]

Thanks for your suggestion, I think using a dict is much better than using tuples.

I don't like setting the constant and the device table together since this would basically break every piece of code that sets the math constants and you would have to implement this type check everywhere if you're not sure about FontForge's version.

I also don't like having to re-set the constant whenever changing the adjustment values. There is definitely the case where you want to add an adjustment to an existing constant and you would have to (most likely get it first and then) set the constant again together with the device table.

In my opinion, a separate property is the lesser of two evils.

Inspired by your dict suggestion, I'm thinking of an API like this:

font.math.MathLeadingDeviceTable = {9: -1, 10: 2}
font.math.MathLeadingDeviceTable[12] = -2
devtab = font.math.MathLeadingDeviceTable # {9: -1, 10: 2, 12: -2}
adj = font.math.MathLeadingDeviceTable[9] # -1

What do you think? I don't know if this is possible with Python's C API, but I will check.

Yep, maybe squashing things together is not such a good idea after all, and I also like the dictionary API much more. Let's ask for @skef's input before you update it.

I think the dict value assignment already works for layers:

# Copy foreground layer to background
l = glyph.layers[1]
glyph.layers[0] = l

I don't like setting the constant and the device table together since this would basically break every piece of code that sets the math constants and you would have to implement this type check everywhere if you're not sure about FontForge's version.

I also don't like having to re-set the constant whenever changing the adjustment values. There is definitely the case where you want to add an adjustment to an existing constant and you would have to (most likely get it first and then) set the constant again together with the device table.

Just to say it -- the setting side isn't actually the problem with the unified design. It would be easy enough to differentiate by the input type, setting the constant when the argument is (or can be converted to) an int, setting the device table when it's a dict, and setting both when its a tuple. The problem comes when you want to read the data, and right now reading one of these properties yields an int, and having it yield something else is what would break compatibility.

As for adding the new names and having those be dicts, it seems like a reasonable solution. You'll want to sanity check the keys and values and reject those that don't make sense with a sensible exception.

Thanks for your feedback. I've looked into how to implement this and I'm still not sure if it is possible.
I tested by changing the getter and setter of math's attributes to deal with dicts, so this is easily possible:

font.math.MathLeadingDeviceTable = {9: -1, 10: 2}
devtab = font.math.MathLeadingDeviceTable # {9: -1, 10: 2}

font.math.MathLeadingDeviceTable[9] also works because it applies the subscript ([key]) syntax to the returned dict. The problem is setting a value with the subscript syntax since the returned dict is modified but not sent back to MathLeadingDeviceTable.

I thought about implementing a new PyTypeObject for device tables with a tp_as_mapping slot which should allow setting with the subscript syntax, just like many other objects in FontForge's API. But I don't know how the getter of the MathLeadingDeviceTable attribute should know whether to return a dict (in case of devtab = font.math.MathLeadingDeviceTable) or to return an instance of the new PyTypeObject (in case of font.math.MathLeadingDeviceTable[12] = -2).

I'm pretty new to C and the Python/C API, so maybe I'm missing something obvious. Any ideas are appreciated.

I'll think about the C-API engineering questions and try to give some guidance in the next few days.

I thought about implementing a new PyTypeObject for device tables with a tp_as_mapping slot which should allow setting with the subscript syntax, just like many other objects in FontForge's API. But I don't know how the getter of the MathLeadingDeviceTable attribute should know whether to return a dict (in case of devtab = font.math.MathLeadingDeviceTable) or to return an instance of the new PyTypeObject (in case of font.math.MathLeadingDeviceTable[12] = -2).

I think your idea about tp_as_mapping slot is correct. The getter of MathLeadingDeviceTable would always return a new object type (see PyFF_LayerArrayType as an example). This would be a dictionary-like object with a tp_as_mapping slot providing length, subscripting and subscript_assignment. The user may keep it as a variable,and it would behave just like a dict.

You'll need some trick to make this type aware of the specific math constant it originated from, so that you don't end up creating a separate type for each constant. Maybe store the closure inside it.

You will also need to define both getter and setter in the relevant PyGetSetDef record.
The getter would work for both cases you mentioned (devtab = font.math.MathLeadingDeviceTable and font.math.MathLeadingDeviceTable[12] = -2)

The setter would need to accommodate both plain dict:

font.math.MathLeadingDeviceTable = {10: -1, 11: 2, 12: 1}

and the new object you define:

dt = fontforge.math.AccentBaseHeightDeviceTable
font.math.FlattenedAccentBaseHeightDeviceTable = dt

I haven't had time to think about this in any depth. It poses a problem we've run into in other places that I haven't determined the best way to solve.

Each of these device tables is, in design, just a dictionary. So you want to be able to get and set them as dictionaries. However, internally the data isn't always going to take the form of a Python structure, so then there's the question of what to do on the C/Python line. And it seems like the solution to that is to do something like FontForge does with Layers, where a layer is a separate object implemented in C with the needed data (although it's annoying to do this with a dictionary).

But then layers have an additional problem, which is that there are really two "kinds" of layer, the layer that you access to the API to get and set the content of a glyph, and then this C-API object. This is confusing -- you get the layer and make changes to it and think those changes will have an effect on the glyph but they don't; you have to assign the layer back to get the changes.

You could have the same problem here depending on the implementation. If it's a separate object and you , for example, assign like font.math.MathLeadingDeviceTable[12] = -2, that won't work if font.math.MathLeadingDeviceTable is just a copied object, because you'll just be modifying the copy. So what you want is a true dict-like interface all the way back to the FontForge structure, but that's a big pain to get right.

The indexable object was quite tricky after all, but I think I nailed it now.
@mf2vec-dev, see my PR into your branch mf2vec-dev#1, please take a look.

Copying here from mf2vec-dev#1, as it probably got lost in the side PR. @skef, could you please check and comment on the proposed API?

Copy on assignment is inherently non-Pythonic, so the following would be confusing:

# User unknowingly creating a detached copy of device table
a = font.math.MathLeadingDeviceTable

# Assigning element inside a
a[10] = 1

# Expecting font.math.MathLeadingDeviceTable to change
print(font.math.MathLeadingDeviceTable[10]) # Not "1" here

For my copy()-based proposal the differentiation would technically be as follows:

# a is a FontForge-specific object PyFF_MathDeviceTable, which has no data,
# but only reference to font and specific device table inside it.
a = font.math.MathLeadingDeviceTable

# Assignment of PyFF_MathDeviceTable object to other device table fails
font.math.SpaceAfterScriptDeviceTable = a # Error: direct assignment to another device table not allowed

# a.copy() copies data into a plain dictionary like {10: 1, 12: -1},
# disconnected from the specific font and the specific device table
#
# Assignment of plain dictionary to other device table succeeds.
font.math.SpaceAfterScriptDeviceTable = a.copy() # OK: assignment of plain data to device table allowed

@skef, could you please check and comment on the proposed API?

I don't have an inherently negative view of this, but there are some other parts of the Python API (which I'm afraid currently escape me) that cause problems when a fontforge.font object is deleted. It sounds like this has the potential to cause similar problems. Right now we basically say: "Be careful not to use this after the font object goes away", which obviously isn't great but is maybe acceptable. It's not awesome to introduce new things with this problem, but it's not a deal breaker either.

To do the hero's version of this, I suppose one would have a list of reverse references to each such outstanding object in the font, and the object could have a boolean to turn it off so that the font can deactivate its orphan objects.

After further deliberation I chose to abandon the .copy() thing and add an iterator to the device table, so that the following would be possible:

for px in font.math.MathLeadingDeviceTable:
    print('Pixel size: {}, value: {}'.format(px, font.math.MathLeadingDeviceTable[px]))

@mf2vec-dev's concern about counterintuitive assignment (mf2vec-dev#1 (review)) is definitely valid, but the similar counterintuitive behavior is already present in FontForge, for example in font.cvt object, so I'm afraid that introducing yet another approach would do more harm than benefit.

The mac job failed with:

This request was automatically failed because there were no enabled runners online to process the request for more than 1 days.

Should I push a trivial change to trigger a re-run or is there a better way?

The mac job failed with:

This request was automatically failed because there were no enabled runners online to process the request for more than 1 days.

Should I push a trivial change to trigger a re-run or is there a better way?

Please ignore this failure. The mac runner is dead, and #5423 is dedicated to it.

I think we are good to go here. @skef , could you please review?

This is "a lot", but it's on my list to review.

@mf2vec-dev, could you please merge from the latest master, to verify CI checks?

@skef, could you please merge this, so that I can adjust the upcoming #5483 for the new PyFF_MathDeviceTable type?