Update afm docs and internal data structures #10692
Conversation
lib/matplotlib/afm.py
Outdated
| the character, values are a (*wx*, *name*, *bbox*) tuple, where | ||
| *wx* is the character width, *name* is the postscript language | ||
| name, and *bbox* is a (*llx*, *lly*, *urx*, *ury*) tuple. | ||
| Parse the given filehandle for character metrics information and returns |
There was a problem hiding this comment.
Some tense inconsistency: Parse vs returns
lib/matplotlib/afm.py
Outdated
| composites. Values are a num parts list of composite information, | ||
| with each element being a (*name*, *dx*, *dy*) tuple. Thus a | ||
| composites line reading: | ||
| Parse the given filehandle for composites information returns it as a dict. |
lib/matplotlib/afm.py
Outdated
| The fields do currently only describe a subset of character metrics | ||
| information defined in the AFM standard. | ||
| """ | ||
| CharMetrics.width.__doc__ = "The character width (WX)." |
There was a problem hiding this comment.
Why no triple-quotes (like the other one-line named tuples)?
lib/matplotlib/afm.py
Outdated
| Parse the given filehandle for character metrics information and returns | ||
| the information as dicts. | ||
|
|
||
| It is assumed that the file cursor is on the line behind |
There was a problem hiding this comment.
I'm not sure I understand this "behind" business. Should the file cursor not be pointing at the 'S'?
There was a problem hiding this comment.
_parse_header consumes lines up to and including StartCharMetrics. It then returns and the file handle is passed on to _parse_char_metrics. The latter interprets the contents of a char metrics block, but not the StartCharMetrics command itself.
This is maybe not 100% clean and intuitive, but good enough for the internal parser functions. The docstring just describes what the code is doing/expecting.
There was a problem hiding this comment.
OK, I think I understand now. When I saw 'line behind something', I thought it was a different line from 'something'. I also don't think 'behind' is the right word here.
Since the cursor is in the middle of a line anyway, I would not mention a line, just: 'the file cursor is pointing after "StartCharMetrics".'
lib/matplotlib/afm.py
Outdated
| composites line reading: | ||
| Parse the given filehandle for composites information returns it as a dict. | ||
|
|
||
| It is assumed that the file cursor is on the line behind 'StartComposites'. |
There was a problem hiding this comment.
Same answer. parse_composites interprets the contents of a composites block, but not the initial StartComposites, which is already consumed by the previous parser function.
lib/matplotlib/afm.py
Outdated
| from :func:`_parse_char_metrics`, | ||
| *dkernpairs* is a :func:`_parse_kern_pairs` dict (possibly {}) and | ||
| *dcomposite* is a :func:`_parse_composites` dict (possibly {}) | ||
| Parse the Adobe Font Metics file in file handle *fh*. |
lib/matplotlib/afm.py
Outdated
| From :func:`_parse_char_metrics`. | ||
| cmetrics_by_name : dict | ||
| From :func:`_parse_char_metrics`. | ||
| dkernpairs : dict |
There was a problem hiding this comment.
I think this 'd' in the front stood for dict, so it's probably not needed with the type now explicitly named.
| ascii_d[num] = (wx, name, bbox) | ||
| name_d[name] = (wx, bbox) | ||
| ascii_d[num] = metrics | ||
| name_d[name] = metrics |
There was a problem hiding this comment.
I guess this is technically a backcompat break? (someone could have been doing wx, bbox = name_d[name].)
It would not have been a problem if parse_afm was private (... as it probably should have been) but a note is probably useful.
There was a problem hiding this comment.
If we are at it, shouldn't we just make parse_afm private? After all, parse_afm is not something I would want to maintain as part of the public matplotlib API in the future.
There was a problem hiding this comment.
I'm fine with deprecating it.
lib/matplotlib/afm.py
Outdated
| self._metrics = dcmetrics_ascii | ||
| self._metrics_by_name = dcmetrics_name | ||
| self._composite = dcomposite | ||
| (header, |
There was a problem hiding this comment.
Now that you're here you may as well write
(self._header,
...,
self._composite) = parse_afm(fh)
lib/matplotlib/afm.py
Outdated
|
|
||
| def get_height_char(self, c, isord=False): | ||
| """ | ||
| Get the height of character *c* from the bounding box. This | ||
| is the ink height (space is 0) | ||
| is the ink height (space is 0). |
There was a problem hiding this comment.
Get the bounding box (ink) height of character c (space is 0).
lib/matplotlib/afm.py
Outdated
|
|
||
| def get_kern_dist(self, c1, c2): | ||
| """ | ||
| Return the kerning pair distance (possibly 0) for chars *c1* | ||
| and *c2* | ||
| and *c2*. |
87f2304 to
2e18e7a
Compare
|
rebased to fix conflict with master |
|
@anntzer is this approved then, or should still someone else approve? |
PR Summary
Improve afm docstrings and cleanup the internal data structures.
PR Checklist