Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add libuninameslist Names2 support #3290

Merged
merged 6 commits into from May 24, 2018
Merged

Add libuninameslist Names2 support #3290

merged 6 commits into from May 24, 2018

Conversation

JoesCat
Copy link
Contributor

@JoesCat JoesCat commented May 16, 2018

Add native scripting and python access to libuninameslist >= ver10.9 Names2 functions

@JoesCat
Copy link
Contributor Author

JoesCat commented May 18, 2018

Hi @jtanx, @frank-trampe,
This is getting pretty close to a squash-n-merge once the python part is checked-out.
At the moment I don't have python dev loaded, but I noted that enabling it in configure failed to stop configure, so, in summary, the python part of configure needs checking first before I complete this.
If you have python dev package installed and want to give this a whirl, you can try at the commandline:
fontforge -lang=py script tests/test1090.py
it should perform similarly to
fontforge script tests/test131.pe

.travis.yml Outdated
@@ -83,15 +83,17 @@ install:
wget 'https://github.com/zeromq/zeromq4-1/releases/download/v4.1.5/zeromq-4.1.5.tar.gz' -O - |tar -zxf -
wget 'https://github.com/zeromq/czmq/releases/download/v3.0.2/czmq-3.0.2.tar.gz' -O - |tar -zxf -
wget 'https://github.com/fontforge/libspiro/releases/download/0.5.20150702/libspiro-0.5.20150702.tar.gz' -O - | tar -zxf -
wget 'https://github.com/fontforge/libuninameslist/releases/download/20160701/libuninameslist-20160701.tar.gz' -O - | tar -zxf -
# wget 'https://github.com/fontforge/libuninameslist/releases/download/20160701/libuninameslist-20160701.tar.gz' -O - | tar -zxf -
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove this please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Will upgrade.

.travis.yml Outdated
wget 'https://bitbucket.org/sortsmill/libunicodenames/downloads/libunicodenames-1.1.0_beta1.tar.xz' -O - | tar -Jxf -
wget --tries 1 "http://download.savannah.gnu.org/releases/freetype/freetype-$FTVER.tar.gz" || \
wget "https://sourceforge.net/projects/freetype/files/freetype2/$SFFTVER/freetype-$FTVER.tar.gz"

pushd zeromq-4.1.5 && ./configure --prefix=$DEPSPREFIX && make && make install && popd
pushd czmq-3.0.2 && CFLAGS="-Wno-format-truncation" ./configure --prefix=$DEPSPREFIX && make && make install && popd
pushd libspiro-0.5.20150702 && ./configure --prefix=$DEPSPREFIX && make && make install && popd
pushd libuninameslist-20160701 && ./configure --prefix=$DEPSPREFIX && make && make install && popd
# pushd libuninameslist-20160701 && ./configure --prefix=$DEPSPREFIX && make && make install && popd
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto - removed on squash-n-merge

if ( !PyArg_ParseTuple(args,"|i",&val) )
return( NULL );
if ( (temp=unicode_name2FrmTab(val))==NULL ) {
temp=malloc(1*sizeof(char)); *temp='\0';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big fan of this. Surely you could just

return Py_BuildValue("s", "");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice replacement. There's a lot of places to replace stuff like this in python.c.

@@ -133,6 +140,7 @@ char *unicode_name(int32 unienc) {
* revisit later.
*/
if( ( unienc >= 0xAC00 && unienc <= 0xD7A3 ) && ( name_data == NULL ) ) {
/* replaced code below to reduce library dependencies
Copy link
Contributor

@jtanx jtanx May 18, 2018

Choose a reason for hiding this comment

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

Can we please not do this, we already are using xasprintf so just use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ok with it in the executables, since you need it for the GUI anyways, but if we have an opportunity to reduce dependencies in the libraries, they can be a little more portable.
I'd like to reduce dependencies on the nox if it can be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

This code depends on xasprintf, which comes from gnulib, and is required whether or not you build with the gui.

Even if you wanted to avoid that dependency, the way you have rewritten it is not good. Instead of adding yet more magic arrays, with memcpys with magic indexing (which is hard for me and others reading this to verify that it's correct), you could have used some form of malloc+sprintf, or asprintf, instead of making it this unreadable and relying on yet more magic arrays

Anyway, just revert it back to how it was before, it compiled fine with or without the gui.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if you wanted to avoid that dependency, the way you have rewritten it is not good.

...would disagree. lengths pre-computed at compile. 1 alloc. 1 branch. more portable.

import sys, fontforge

cnt = UnicodeBlockCountFromLib()
if ( cnt < 1 ):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is python, not C, remove the brackets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't want it to be confused as a variable :-)
...but it appears to be common:
https://stackoverflow.com/questions/19747371/python-exit-commands-why-so-many-and-when-should-each-be-used
...even here, there is an example where you need to say quit() exit(), not quit or exit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Functions have nothing to do with if statements. Having brackets where they are unnecessary in such statements is un-pythonic. Just remove them.

u1 = fontforge.UnicodeNames2NxtUniFromLib(1)
u2 = fontforge.UnicodeNames2NxtUniFromLib(2)
print("Sample of internal table has ",u0,", ",u1,", ",u2," as the first 3 unicode values.")
if ( u0 < 0 || u1 < 1 || u2 < 2 ):
Copy link
Contributor

Choose a reason for hiding this comment

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

or, not ||. Did you run this locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need to get python dev loaded. This can get removed in a squash-merge.

@jtanx
Copy link
Contributor

jtanx commented May 18, 2018

If you're going to add this for python, then please get a python build working and test it locally before continuing.

@JoesCat
Copy link
Contributor Author

JoesCat commented May 22, 2018

ready to squash-n-merge.

@frank-trampe
Copy link
Contributor

Okay. Do it.

@JoesCat
Copy link
Contributor Author

JoesCat commented May 24, 2018

Hi @frank-trampe - I was about to head-out-the-door.
I see @khaledhosny patch getting merged now - so it's going to still be checking before I'm out.
If this and the others look ok to you, please go ahead and merge - these will take time to get the check (vs the X).

@JoesCat JoesCat merged commit 1d9fb33 into fontforge:master May 24, 2018
Omnikron13 pushed a commit to Omnikron13/fontforge that referenced this pull request May 31, 2022
* Add libuninameslist Names2 support

* Add libuninameslist Names2 support

* Add libuninameslist Names2 support

* Add libuninameslist Names2 support

* Add libuninameslist Names2 support

* Add libuninameslist Names2 support
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.

None yet

3 participants