Glyphs3 #923
Glyphs3 #923
Conversation
There was a problem hiding this comment.
@schriftgestalt I had a scroll through, overall you're renaming some methods (I suppose to match Glyphs.app API?), you're adding the post_read mechanism to normalize input data, and you've changed the approach in a few places (axes and custom parameters) I suppose to fix bugs and match parameter names with Glyphs.app, right?
I have a question about the class hierarchy in classes.py: does their API now match Glyphs.app 3? And post_read normalizes the data to be the same as Glyphs.app 3? Or do they match both Glyphs.app 2 and 3? What would you do when you release Glyphs.app 4? Will you normalize in post_read to the data model of version 4?
| def __CustomParametersProxy_get_custom_values__(self, key): | ||
| parameters = [] | ||
| for parameter in self: | ||
| if not parameter.active: | ||
| continue | ||
| if parameter.name == key: | ||
| parameters.append(parameter) | ||
| return parameters |
There was a problem hiding this comment.
I don't. Seem to be some intermediate state that is left over.
Lib/glyphsLib/classes.py
Outdated
| @@ -2440,7 +2737,7 @@ def applyTransform(self, transformationMatrix): | |||
| def draw(self, pen: AbstractPen) -> None: | |||
| """Draws contour with the given pen.""" | |||
| pointPen = PointToSegmentPen(pen) | |||
| self.drawPoints(pointPen) | |||
| frawPoints(pointPen) | |||
| @@ -2654,7 +2951,7 @@ def __init__(self, glyph="", offset=(0, 0), scale=(1, 1), transform=None): | |||
| else: | |||
| self.transform = transform | |||
|
|
|||
| def clone(self): | |||
| def copy(self): | |||
There was a problem hiding this comment.
Why rename every clone to copy? To match Glyphs.app API?
There was a problem hiding this comment.
Yes. In Glyphs/objectiveC, it is copy()
There was a problem hiding this comment.
I'm OK with this. I only called it clone because I had been doing too much Rust at the time. 🦀
| @property | ||
| def weightValue(self): | ||
| return self._get_axis_value(0) | ||
| return self.internalAxesValues[0] if len(self.font.axes) > 0 else None |
There was a problem hiding this comment.
why test the length of one variable and index into another?
There was a problem hiding this comment.
It should only produce a value if there are axis to back it up. Otherwise you get an IndexError in the getter.
Lib/glyphsLib/classes.py
Outdated
| @@ -3619,26 +4122,49 @@ class LayerComponentsProxy(LayerShapesProxy): | |||
|
|
|||
|
|
|||
| class GSLayer(GSBase): | |||
| def _serialize_attributes_to_plist(self): | |||
There was a problem hiding this comment.
I wouldn't call this serialize, maybe just get_plist_attributes? Because this method doesn't write stuff to the writer
| @property | ||
| def formatVersion(self): | ||
| raise "not implemented" | ||
|
|
||
| @formatVersion.setter | ||
| def formatVersion(self, value): | ||
| raise "not implemented" |
There was a problem hiding this comment.
Deleted, same as the next 2?
There was a problem hiding this comment.
Right. I used those for debugging.
Lib/glyphsLib/builder/builders.py
Outdated
| and glyph.export | ||
| and ".background" not in layer.name | ||
| ): | ||
| self.bracket_layers.append(layer) | ||
| elif ( | ||
| self.minimal | ||
| and layer.layerId not in master_layer_ids | ||
| and not layer._is_brace_layer() | ||
| and not layer.isBraceLayer() |
There was a problem hiding this comment.
Is this renamed to match a Glyphs.app API?
Lib/glyphsLib/builder/builders.py
Outdated
| @@ -720,7 +734,7 @@ def _fake_designspace(self, ufos): | |||
| from .masters import to_glyphs_master_attributes | |||
| from .names import to_glyphs_family_names, to_glyphs_master_names | |||
| from .paths import to_glyphs_paths | |||
| from .sources import to_glyphs_sources | |||
| from .sources import _to_glyphs_source | |||
There was a problem hiding this comment.
rename function to not be private if it's the one you want to use from the sources module
| @@ -18,10 +18,13 @@ | |||
| import logging | |||
|
|
|||
| from glyphsLib.util import bin_to_int_list, int_list_to_bin | |||
| from Foundation import NSArray | |||
There was a problem hiding this comment.
Don't depend on mac-only stuff?
There was a problem hiding this comment.
I added this to get it to work from within Glyphs. This needs a better separation.
There was a problem hiding this comment.
@schriftgestalt I just tried now to compile a project with this branch and this immediately crashed, so outside of a mac it's currently not possible to test this branch
Lib/glyphsLib/builder/masters.py
Outdated
| if note is not None: | ||
| ufo.info.note = note | ||
|
|
||
| delf(userData[UFO_NOTE_KEY]) |
| try: | ||
| return value.propertyListValueFormat_(3) # TODO: this is the plain Glyphs API. pythonize this | ||
| except: | ||
| from objc._pythonify import OC_PythonLong, OC_PythonFloat |
There was a problem hiding this comment.
This import also crashes on Windows/Linux
There was a problem hiding this comment.
Seems like we're not running CI tests on PRs, either.
There was a problem hiding this comment.
hid that in a try:except:, too.
store in designspace and ufo lib rename property to match Glyphs API
- Convert data to G3 format on load. convert back when writing G2 - Some adjustments to be able to run the builder from within Glyphs 3 - compare the result with what Glyphs would write and adjust (e.g. com.schriftgestalt keys in the libs) TODO: restore support for the old lib keys
glyphs_owner_class handles this
schriftgestalt
force-pushed
the
Glyphs3
branch
from
e8c73ed
to
72069e2
Compare
and remove version 2 code
they are stored in a feature.labels now
|
@schriftgestalt do you intend to replace this PR with the new branch Glyphs3-merge? |
|
Yes. |
No description provided.