makeutype is_Ligatures.c #2762

JoesCat · 2016-08-02T01:22:27Z

Allow makeutype to build an up-to-date is_Ligature.c file, and add several native script functions to allow read access to internal ligature and fraction tables.

The "python" program to build these files contributed to a headache worth of problems earlier between 2012 and 2014 when Fontforge built python as an "optional" choice and not a "forced" dependency. Removing the python program allowed us to continue onto 20140101 and onwards... Example problems/issues fontforge#384, fontforge#387, fontforge#229, fontforge#565, fontforge#382, fontforge#241, fontforge#249, fontforge#233 makeutype.c already uses the Unicode list, so it can do this here too.

frank-trampe · 2016-08-03T15:57:19Z

Unicode/makeutype.c

-    if ( index>0x11ffff )
-return;
+    if ( index<0 ) return( -1 );
+    if ( index>0x11ffff ) return( 0 );
    ++pt;							/* move past semicolon */


Where does pt originate? Are we sure that it is of non-zero length?

hehehe ;-) your turn to go off-topic ;-P
see: #2735
"Could we just fix the header and not mess with the whitespace and minor verbiage? I don't think it's worth the changelog/blame clutter."

I've already had to rebase patches a few times for throwing in extras, so, I'll just leave that pt alone for now. ;-P

A lot of code assumes things work fine and doesn't deal with "what if?" scenarios, so...as you note here...it's too easy to go fix one-more thing and throw it into a patch while you're there now.

This entire pull request is sort-of a subset of adding authors and copyrights to satisfy #2735, but this patch focuses on creating is_Ligatures.c as a substitute for the python code that had to be removed in the past. Would prefer to throw-in python script commands to keep this all together as a mini-howto of what needs attention, but this single patch is pretty big already without it.

This is not whitespace, and you're already adjusting this file. Surely adding if (pt[0] == '\0') return -1 would not ruin your day? I'm happy to do it if necessary, but it's much more efficient if you do it.

Looking at the structure of unialt.c shows that fontforge is unable to cope with codepoint==0, or has difficulty and needing workaround exceptions. Older bitmaps have references to char 0, and as you point-out, it won't kill us to make it 1 instead of zero, but why limit ourselves? These functions here will work fine if the value is zero. Youll note that there is another routine in ustring that also handles zero too. unialt could probably use a rewrite and I believe we can add zero capability into it too. Where 0 is a problem is with higher level fonts, so it makes sense to test for zero upthere...but down here should be okay to look for zero without having to limit ourselves from the get-go.

That's not what I meant. What I meant was that there is a potential segfault here if argument pt is a zero-length string because it gets incremented past the first value before it gets used. All we need is if (pt[0] == '\0') return -1, and I'm hoping that you can just put it into this patch.

ok - will add test for zero on weekend.

frank-trampe · 2016-08-24T17:48:39Z

Unicode/makeutype.c

+    if ( s<m )
+    fprintf( data, "    else\n\treturn( (int32)(%s32[n-%d]) );\n", t, s );
+    fprintf( data, "}\n\n" );
+}


So m is table size. What are t, n, and s in this context?

'm' is the entire maximum length of the table *dt which will got from reading unicode list and then be named 't' when building the tables and functions in is_Ligatures.c. Looking at a byte count, it made less waste to have an array of 500+ ligatures, 20+ vulgars, and about 50 odd fraction definitions (approx 600 x 4bytes = 2400bytes) instead of assigning an individual 1bit for each in the lookup table (approx 1bit x MAXC = 1/8byte x 65536 = 8k). doing this for ligs, vulgs and frac would make it 8k x 3 = 24k used in flag space. Since I was already building a program, it seemed worth going the extra step and compressing everything in {0..65535} into uint16 (so we split the table 0...s), and anything left afterwards goes into {65536....->) which is uint32 (and is the remainder of the table from s+1...m. so instead of using 2400bytes, now is closer to 1200bytes to define about 600 ligs/vuls/fracs.
'n' is used to name the various functions using the tables.
These routines are reusable 3x to build ligs, vuls, fracs., so testing against one verifies the other two will build similarly the same.

Great. Can we get this (or a shorter version of it) in a comment?

We probably ought to rename parameter n to something like nam so as to avoid confusion with the other n.

Are these comments right?

// m is the maximum size of the table. // s is the size of the lower partition of the table. // t is the first part of the data type name. // nam is the function name base.

added notes

frank-trampe · 2016-08-24T18:11:55Z

And thanks for making tests!

JoesCat · 2016-08-25T04:37:03Z

I was thinking of adding more stuff, but the only thing I got working ATM is vulgar alt expansions. The unicode list appears inconsistent for getting ligatures and other fractions ATM (needs more thought, and ver9.0 is a larger table, maybe improved too), plus you announced a planned 2week window on the next update, so no point on making it more complex at the moment.
There might be some interest for this PR from #2494, or this #2441, but it's only a partial solution ATM

jtanx · 2016-09-06T08:02:47Z

tests/test1009.py

+
+import sys, fontforge
+
+print "Get Table Array Totals."


Pretty sure all tests should conform to Python 3 standards.

Hmm why was the test skipped on Travis? Anyway adding brackets to the print calls should be enough for python 3 compatibility. test1009.py also needs to be added to Makefile.am EXTRA_DIST in the test folder.

Ok yep. The reason why it wasn't failing on Travis when it should have (which compiles FontForge with Python3 scripting) is because test1009.py wasn't in Makefile.am.

JoesCat · 2016-09-06T14:28:42Z

Good catch with the python3 @jtanx
I was tempted to add test926.py as well to the Makefile.am
We'll need to add one PR, update, add the other PR, or put the 926 on another line so we won't have a merge conflict....or add the 926 here since I'm already here...(even though it's not related).

jtanx · 2016-09-06T14:34:13Z

I had a brief look at 926, but it looks like it's harder to get that working on Python 3 due to bytes not being implicitly converted to strings. I'd definitely leave that one for another PR.

jtanx · 2016-09-08T23:59:28Z

ping @JoesCat to rebase because #2811 was merged

JoesCat · 2016-09-11T23:00:24Z

had to pull-out test1009.py to avoid merging conflict in tests/Makefile.am due to #2811 - will add it later after rebasing (later).

Added copyright Authors to satisfy Debian Lint copyright issue fontforge#2643. Added several unicode chart lookup functions, native scripting access, python scripting, test129.pe and test1009.py.

frank-trampe · 2016-09-14T19:46:17Z

I'm done here. @jtanx?

jtanx · 2016-09-15T00:23:39Z

@JoesCat are you going to rebase to add back test1009 to the Makefile? Otherwise lgtm

JoesCat · 2016-09-15T04:54:44Z

Gets sort of messy trying to do a PR and rebase at the same time, so test1009.py would sit somewhere after the rebase. I'll add it afterwards.with another isLigature.c PR.

JoesCat · 2016-09-15T05:04:00Z

Weird - seems like we both hit merge at the same time - hehehe
....last comment 7min ago, merged 6min ago, branch del 5min ago. Sort of explains github's weird reaction when I clicked buttons.

finally done - time to move-on.

Follow-up to #2762

makeutype is_Ligatures.c

Follow-up to fontforge#2762

frank-trampe reviewed Aug 3, 2016
View reviewed changes

JoesCat force-pushed the Ligatures branch from 6607dff to b5499bc Compare August 9, 2016 05:13

frank-trampe reviewed Aug 24, 2016
View reviewed changes

JoesCat force-pushed the Ligatures branch 2 times, most recently from acf8ee8 to ca6acd9 Compare September 1, 2016 14:24

jtanx reviewed Sep 6, 2016
View reviewed changes

jtanx mentioned this pull request Sep 6, 2016

WOFF: Compress & write the font tables in the same order as the origi… #2524

Merged

JoesCat force-pushed the Ligatures branch from ca6acd9 to 186d762 Compare September 6, 2016 14:20

jtanx mentioned this pull request Sep 7, 2016

Make test926.py Python 3 compatible and add it to Makefile.am #2811

Merged

1 task

JoesCat force-pushed the Ligatures branch 3 times, most recently from 21940f9 to 6e6fe3b Compare September 11, 2016 17:30

JoesCat force-pushed the Ligatures branch 2 times, most recently from d0a8852 to 1d5f48b Compare September 13, 2016 06:21

makeutype.c generates is_Ligature.c based on current Unicode data

62578c1

Added copyright Authors to satisfy Debian Lint copyright issue fontforge#2643. Added several unicode chart lookup functions, native scripting access, python scripting, test129.pe and test1009.py.

JoesCat force-pushed the Ligatures branch from 1d5f48b to 62578c1 Compare September 13, 2016 06:34

jtanx merged commit f868b21 into fontforge:master Sep 15, 2016

JoesCat deleted the Ligatures branch September 15, 2016 04:56

jtanx added a commit that referenced this pull request Sep 15, 2016

Add test1009.py to Makefile.am

e965d98

Follow-up to #2762

jtanx added a commit that referenced this pull request Sep 15, 2016

Add test1009.py to Makefile.am

fc3f646

Follow-up to #2762

jtanx mentioned this pull request Sep 15, 2016

Add test1009.py to Makefile.am #2837

Merged

JoesCat mentioned this pull request Nov 29, 2016

Simpler code with extra fixes #2964

Merged

JoesCat mentioned this pull request Dec 9, 2016

Simpler code #2970

Closed

Omnikron13 pushed a commit to Omnikron13/fontforge that referenced this pull request May 31, 2022

Merge pull request fontforge#2762 from JoesCat/Ligatures

024b15d

makeutype is_Ligatures.c

Omnikron13 pushed a commit to Omnikron13/fontforge that referenced this pull request May 31, 2022

Add test1009.py to Makefile.am

c78e5d6

Follow-up to fontforge#2762

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

makeutype is_Ligatures.c #2762

makeutype is_Ligatures.c #2762

JoesCat commented Aug 2, 2016

frank-trampe Aug 3, 2016

JoesCat Aug 4, 2016

frank-trampe Aug 24, 2016

JoesCat Aug 25, 2016

frank-trampe Aug 25, 2016

JoesCat Aug 26, 2016 via email

frank-trampe Aug 24, 2016

JoesCat Aug 25, 2016

frank-trampe Sep 6, 2016

frank-trampe Sep 6, 2016

JoesCat Sep 11, 2016

frank-trampe commented Aug 24, 2016

JoesCat commented Aug 25, 2016 •

edited

jtanx Sep 6, 2016

jtanx Sep 6, 2016

jtanx Sep 6, 2016

JoesCat commented Sep 6, 2016

jtanx commented Sep 6, 2016

jtanx commented Sep 8, 2016

JoesCat commented Sep 11, 2016

frank-trampe commented Sep 14, 2016

jtanx commented Sep 15, 2016

JoesCat commented Sep 15, 2016

JoesCat commented Sep 15, 2016

makeutype is_Ligatures.c #2762

makeutype is_Ligatures.c #2762

Conversation

JoesCat commented Aug 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoesCat Aug 26, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frank-trampe commented Aug 24, 2016

JoesCat commented Aug 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoesCat commented Sep 6, 2016

jtanx commented Sep 6, 2016

jtanx commented Sep 8, 2016

JoesCat commented Sep 11, 2016

frank-trampe commented Sep 14, 2016

jtanx commented Sep 15, 2016

JoesCat commented Sep 15, 2016

JoesCat commented Sep 15, 2016

JoesCat commented Aug 25, 2016 •

edited