-
Notifications
You must be signed in to change notification settings - Fork 26
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 a sample.py script #2
Conversation
the python callback for the buffer message func only takes a str (message) for now... Not sure how or whether I should pass buffer and font (or even user_data) on to it. uuzz/_harfbuzz.pyx
src/uharfbuzz/_harfbuzz.pyx
Outdated
@classmethod | ||
def create(self, blob: Blob, index: int): | ||
def create(cls, fontdata: bytes, index: int): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it's a python function, here we could probably accept not only just bytes, but a union of bytes/bytearray/array.array or anything that exposes the buffer interface really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant not only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you're looking for memoryview? also can you keep the name blob (and below hb_blob), thus far I keep the same signature names as harfbuzz. (and bytes is a binary "blob" so.. we can revisit later if needed)
yo @adrientetar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I thought of putting an example such as this. Good to have!
examples/sample.py
Outdated
@@ -0,0 +1,45 @@ | |||
#!/usr/bin/python3 | |||
|
|||
from __future__ import print_function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need this?
examples/sample.py
Outdated
import sys | ||
|
||
|
||
with open(sys.argv[1], 'rb') as fp: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fp → fontfile?
examples/sample.py
Outdated
face = hb.Face.create(fontdata, 0) | ||
font = hb.Font.create(face) | ||
upem = face.upem | ||
del face |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: font keeps a reference to face, so "del face" isn't doing much here. Is it here to somehow highlight that face isn't used after that point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, was just a blind copy from harfbuzz own example script
examples/sample.py
Outdated
x_advance = pos.x_advance | ||
x_offset = pos.x_offset | ||
y_offset = pos.y_offset | ||
print("gid%d=%d@%d,%d+%d" % (gid, cluster, x_advance, x_offset, y_offset)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could use an f-string ;)
src/uharfbuzz/_harfbuzz.pyx
Outdated
# XXX I'm passing the python callback as user_data and recovering it | ||
# from the C-API func below. Is there a better way? | ||
# Also, the callback is only taking a message argument, no buffer, | ||
# nor font or its own user_data. Not sure whether/how to pass them on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably look into this, but do we at all need implementing "message func" in the first place?
The nice thing about the other stuff I put thus far is I use it so I know what things need to be passed to python or not..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we need to implement it. I was just trying to port the simple debugger that is exemplified in the harfbuzz's original sample.py, but using uharfbuzz.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can leave it out for now, I've never needed to use it myself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrientetar I removed it from the example. Shall I remove the set_message_func method altogether from the pyx source file as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, and pxd too
src/uharfbuzz/_harfbuzz.pyx
Outdated
@classmethod | ||
def create(self, blob: Blob, index: int): | ||
def create(cls, fontdata: bytes, index: int): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you're looking for memoryview? also can you keep the name blob (and below hb_blob), thus far I keep the same signature names as harfbuzz. (and bytes is a binary "blob" so.. we can revisit later if needed)
src/uharfbuzz/_harfbuzz.pyx
Outdated
@@ -451,3 +468,7 @@ def ot_layout_table_get_script_tags(face: Face, tag: str) -> List[str]: | |||
packed = cstr | |||
tags.append(packed.decode()) | |||
return tags | |||
|
|||
|
|||
cpdef void ot_font_set_funcs(Font font): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why cpdef here? I used def for all module level functions.
I don't mind cpdef (is it more efficient?) but we should use it consistently in the file then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's only more efficient in so far as you also need to call a cpdef function from within cython (in which case it uses its faster cdef version). But if we only call this from python, then def is ok.
src/uharfbuzz/charfbuzz.pxd
Outdated
@@ -197,3 +203,6 @@ cdef extern from "hb-ot.h": | |||
unsigned int start_offset, | |||
unsigned int* script_count, # in/out | |||
hb_tag_t* script_tags) # out | |||
|
|||
# hb-ot-font.h | |||
void hb_ot_font_set_funcs(hb_font_t *font) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: so far I put the stars before the space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copy-pasted it from the harfbuzz headers, which put the stars after the space, but I don't mind.
examples/sample.py
Outdated
@@ -0,0 +1,45 @@ | |||
#!/usr/bin/python3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could simply put this example in the readme, it's the first file ppl look at so more convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
PyPI now supports Github Flavoured Markdown, enough with reStructuredText.
Adrien likes it that way
setup.py
Outdated
@@ -1,12 +1,22 @@ | |||
#!/usr/bin/env python3 | |||
# -*- coding: utf-8 -*- | |||
from skbuild import setup | |||
import os | |||
from io import open |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you sort the imports
src/uharfbuzz/_harfbuzz.pyx
Outdated
# XXX I'm passing the python callback as user_data and recovering it | ||
# from the C-API func below. Is there a better way? | ||
# Also, the callback is only taking a message argument, no buffer, | ||
# nor font or its own user_data. Not sure whether/how to pass them on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, and pxd too
src/uharfbuzz/_harfbuzz.pyx
Outdated
cdef Face inst = cls() | ||
inst._hb_face = hb_face_create(blob, index) | ||
cdef hb_blob_t* cblob = hb_blob_create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: name it hb_blob
either we do it properly or we don't, says Adrien
I was just playing with the api and tried to rewrite the sample script from harfbuzz upstream repo using uharfbuzz:
https://github.com/harfbuzz/harfbuzz/blob/master/src/sample.py
just scrap it if you think we don't need it here