Skip to content

Support font.math.*DeviceTable as indexable object#1

Merged
mf2vec-dev merged 9 commits intomf2vec-dev:py-math-devtabfrom
iorsh:iorsh-devtab-dict
Jul 27, 2024
Merged

Support font.math.*DeviceTable as indexable object#1
mf2vec-dev merged 9 commits intomf2vec-dev:py-math-devtabfrom
iorsh:iorsh-devtab-dict

Conversation

@iorsh
Copy link
Copy Markdown

@iorsh iorsh commented Mar 29, 2024

No description provided.

Copy link
Copy Markdown
Owner

@mf2vec-dev mf2vec-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for looking into this. I did some testing and it seems to work.

Besides the comments, please also update fontforge.rst.

The only thing that is a bit strange is the behavior of something like the following. I understand why this happens, but this may not be the most intuitive behavior for some users.

font.math.MathLeadingDeviceTable[10] = 1
a = font.math.MathLeadingDeviceTable
print(font.math.MathLeadingDeviceTable[10]) # 1
print(a[10]) # 1
print('')

a[10] = 2
print(font.math.MathLeadingDeviceTable[10]) # 2
print(a[10]) # 2
print('')

font.math.SpaceAfterScriptDeviceTable = a
a[10] = 3
print(font.math.MathLeadingDeviceTable[10]) # 3
print(a[10]) # 3
print(font.math.SpaceAfterScriptDeviceTable[10]) # 2

(a = font.math.*DeviceTable by reference and font.math.*DeviceTable = a by value)

I'm not sure if this can or should be changed. However, it's fine for me to merge it with this behavior if there is no easy alternative.

Comment thread fontforge/python.c Outdated
Comment thread fontforge/python.c
Comment thread fontforge/python.c Outdated
Comment thread fontforge/python.c Outdated
Comment thread fontforge/python.c
Comment thread fontforge/python.c
Comment thread fontforge/python.c
Comment thread tests/test1019.py
Comment thread tests/test1019.py
@iorsh
Copy link
Copy Markdown
Author

iorsh commented Apr 4, 2024

The only thing that is a bit strange is the behavior of something like the following. I understand why this happens, but this may not be the most intuitive behavior for some users.

font.math.MathLeadingDeviceTable[10] = 1
a = font.math.MathLeadingDeviceTable
print(font.math.MathLeadingDeviceTable[10]) # 1
print(a[10]) # 1
print('')

a[10] = 2
print(font.math.MathLeadingDeviceTable[10]) # 2
print(a[10]) # 2
print('')

font.math.SpaceAfterScriptDeviceTable = a
a[10] = 3
print(font.math.MathLeadingDeviceTable[10]) # 3
print(a[10]) # 3
print(font.math.SpaceAfterScriptDeviceTable[10]) # 2

(a = font.math.*DeviceTable by reference and font.math.*DeviceTable = a by value)

I'm not sure if this can or should be changed. However, it's fine for me to merge it with this behavior if there is no easy alternative.

I think we can resolve this ambiguity by forcing user into explicit copy:

a = font.math.MathLeadingDeviceTable
a[10] = 3
font.math.SpaceAfterScriptDeviceTable = a # Error: direct assignment to another device table not allowed
font.math.SpaceAfterScriptDeviceTable = a.copy() # OK: data explicitly copied from a into another device table

Alternatively, we can just forbid copying into another device table altogether.

@skef, what do you think about the desired API?

@iorsh iorsh requested a review from mf2vec-dev April 4, 2024 21:36
Comment thread fontforge/python.c
Comment thread tests/test1019.py
Comment thread fontforge/python.c
@iorsh iorsh requested a review from mf2vec-dev April 6, 2024 18:44
@skef
Copy link
Copy Markdown

skef commented Apr 17, 2024

I think we can resolve this ambiguity by forcing user into explicit copy:

What would be the disadvantage of always copying on assignment ourselves in this case? Would that lead to too much confusion about what was by value and what was by reference?

font.math.SpaceAfterScriptDeviceTable = a # Error: direct assignment to another device table not allowed
font.math.SpaceAfterScriptDeviceTable = a.copy() # OK: data explicitly copied from a into another device table

Is this even possible to differentiate at the C-API level (genuine question)? I would have thought both would just look the same internally.

@iorsh
Copy link
Copy Markdown
Author

iorsh commented Apr 20, 2024

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

@iorsh
Copy link
Copy Markdown
Author

iorsh commented Jul 12, 2024

@mf2vec-dev, please review

Copy link
Copy Markdown
Owner

@mf2vec-dev mf2vec-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing the iterator.

In light of the discussion in this PR and in fontforge#5348, I'm fine with the current approach.

@mf2vec-dev mf2vec-dev merged commit 116d527 into mf2vec-dev:py-math-devtab Jul 27, 2024
@iorsh iorsh deleted the iorsh-devtab-dict branch March 28, 2025 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants