8000 [varLib] make `build` also accept DesignSpaceDocument objects by anthrotype · Pull Request #1425 · fonttools/fonttools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[varLib] make build also accept DesignSpaceDocument objects #1425

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 22 commits into from
Jan 3, 2019
Merged

[varLib] make build also accept DesignSpaceDocument objects #1425

merged 22 commits into from
Jan 3, 2019

Conversation

anthrotype
Copy link
Member

with source descriptors containing pre-loaded TTFont instances

Continuation of #1416

madig and others added 15 commits December 19, 2018 13:40
@madig
Make build and load_designspace accept a DS object
e464e45
@madig
Be clever in what we accept 8000
67d1f35 
See #1416 (comment)
@madig
Minor reorganization
31c15b7
@madig
Remove unused variable
7983517
@madig
Extract Designspace font loader into own function
596b2ce
@anthrotype
rename _ensure_sources to load_masters, misc refactor
04d5060
@anthrotype
ttFont: add copy() method that return new instance with same data
6f317aa 
and use it in varLib to copy master font
@anthrotype
interpolate_layout: allow to pass DesignSpaceDocument as input, like …
9b45094 
…varLib.build
@anthrotype
override SFNTReader __deepcopy__ for 'file' isn't copyable; use it in…
4001ded 
… varLib
@anthrotype
SFNTReader: try deepcopy'ing first, if fails do manual copy of file c…
560c755 
…ontents
@anthrotype
varLib_test: no need to reload master_ttfs; reuse the reload_font hel…
ba197cb 
…per func in the other tests
@anthrotype
varLib_test: test loading master with layerName but without required …
5b250db 
…font object
@anthrotype
designspaceLib: add getSourcePath method
1470ef7 
and remove BaseDocReader.getSourcePaths as it was broken anyway (it was assuming 'sources'
attribute of DesignSpaceDocument object was a dict, whereas it is now a list of
SourceDescriptor objects
@anthrotype
designspace_test: test getSourcePath method prefers 'path' attribute …
88eacd3 
…when present
@anthrotype
Merge remote-tracking branch 'origin/master' into varlib-build-accept…
3558d4e 
…-ds-object
@anthrotype anthrotype requested a review from madig January 2, 2019 14:16
@madig
Copy link
Collaborator
madig commented Jan 2, 2019 •
edited
Loading

Can we use Python 3.x features yet? The whole path handling stuff makes my head hurt. I'd like to have all paths be pathlib.Path objects, no POSIX gymnastics, that are written to disk with as_posix(). Since filenames (not .path) must be relative according to the spec, we don't have to deal with drive roots on Windows.

I'm not super familiar with all the use-cases for designspace objects, but what about making filename a write-only attribute (not accessible from Python) and always have path point to the absolute location of sources and instances? On writing the DS, the filename property is calulated as filename = SourceOrInstanceDescriptor.path.relative_to(Path(designspace_file)).as_posix(). Then there is also no need for a getSourcePath() or updatePaths() method.

Let me think about what that would mean for client code.

  1. A piece of code with an in-memory designspace object with in-memory UFOs or TTFs attached and no path for them set yet will write all font objects to disk somewhere, write the paths to the DS object and write it last.
  2. Loading a designspace file would resolve the filename attributes on loading and instantiate absolute Path objects.
  3. A piece of code moving font objects somewhere else on disk would overwrite the path attributes and write the DS object again.
  4. Moving DS plus font files around on disk manually works (or not) as before.

Maybe I'm forgetting something, but path modification would mean simply overwriting the path attribute with the known new absolute path and writing the DS object to disk would take care of the rest.

Edit: this might be something for another PR really...

@madig
Copy link
Collaborator
madig commented Jan 3, 2019 •
edited
Loading

Aside from this, the changes look mostly fine to me. One thing that has me scratching my head is the reload_font thing in varLib_test.py. Are we testing the __deepcopy__ changes properly? The TTX dumps we load from the test data are already "properly" serialized data structures with no temporary data, no? Shouldn't we test freshly compiled fonts that still have some working data in them that gets removed on save() to check that it doesn't interfere with merging?

@anthrotype
varLib_test: read TTFonts from compiled binary files
7d688ff 
instead of importing from TTX, and set lazy=True so we exercise the special SFNTReader.__deepcopy__
@anthrotype
[sfnt] allways copy file data to BytesIO, not just if deepcopy failed
ce86397 
on py27, calling deepcopy on a file object doesn't raise TypeError, but leaves
the duplicated file object in an inconsistent state:

https://travis-ci.org/fonttools/fonttools/jobs/474811063#L714

any operations on it will fail with 'ValueError: I/O operation on uninitialized object'.
Thus, for semplicity for now we always do a manual copy the file contents into an
in-memory BytesIO stream, rather than attempting to call deepcopy first.
@anthrotype
Copy link
Member Author
anthrotype commented Jan 3, 2019 •
edited
Loading

what you suggest make sense, and i've also always been confused by this duality of path/filename attributes in designspace document. However, I don't want to break existing code for the sake of it. For now, let's keep both path and filename attributes as posix path strings, mutable, and nullable (with all the problems that this implies), and deal with them later in separate PRs.

About the reload_font thing in the test, that's only done because we are comparing TTX dumps for string equality, and there are a few redundant things which are only computed at compile time: e.g. count values (written out inside xml comments in the ttx dump), or in this particular case, the name records in the name table are sorted upon compile. No big deal, the data doesn't change.

I changed the test to load compiled binary test fonts instead of importing them as ttx to address your concern, and it actually caught a bug in that custom deepcopy implementation for SFNTReader.

@anthrotype
varLib/designspaceLib: require font-less source to have a path; remov…
448b946 
…e getSourcePath

simpler is better. Also, refactored updatePaths and added more tests to it.
@anthrotype
minor: docstring [skip ci]
b0a00a8
@anthrotype anthrotype merged commit f88a1f6 into master Jan 3, 2019
@anthrotype anthrotype deleted the varlib-build-accept-ds-object branch January 3, 2019 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madig madig Awaiting requested review from madig

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anthrotype @madig
0