Skip to content

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Mar 29, 2025

In 89a7e19, an API for converting "dvi glyph indices" (as stored in a dvi file) to FreeType-compatible keys (either "indices into the native charmap" or "glyph names") was introduced. It was intended that end users (i.e., backends) would check the type of text.glyph_name_or_index ((A) int or (B) str) and load the glyph accordingly ((A) FT_Set_Charmap(native_cmap); FT_Load_Char(index); or (B) FT_Load_Glyph(FT_Get_Name_Index(name));); however, with the future introduction of {xe,lua}tex support, this kind of type checking becomes inconvenient, because {xe,lua}tex's "dvi glyph indices", which are directly equal to FreeType glyph indices (i.e. they would be loaded with FT_Load_Glyph(index);), would normally also be converted to ints.

This PR introduces a new API (Text.index) that performs this mapping (via the new DviFont._index_dvi_to_freetype), always mapping to FreeType glyph indices (i.e. one can always just call FT_Load_Glyph on the result). To do so, in case (A) it loads itself the native charmap (something the end user needed to do by themselves previously) and performs the cmap-to-index conversion (FT_Get_Char_Index) previously implicit in FT_Load_Char; in case (B) it performs itself the name-to-index conversion (FT_Get_Name_Index). When {xe,lua}tex support is introduced in the future, _index_dvi_to_freetype will just return the index as is.

The old APIs are not deprecated yet, as other changes will occur for {xe,lua}tex support (e.g. font_effects will go away and be replaced by .font.effects, as {xe,lua}tex don't store that info in the pdftexmap entry), and grouping all API changes together seems nicer (to only require a single adaptation step by the API consumer).

PR summary

PR checklist

In 89a7e19, an API for converting "dvi glyph indices" (as stored
in a dvi file) to FreeType-compatible keys (either "indices into
the native charmap" or "glyph names") was introduced.  It was
intended that end users (i.e., backends) would check the type of
`text.glyph_name_or_index` ((A) int or (B) str) and load the glyph
accordingly ((A) `FT_Set_Charmap(native_cmap); FT_Load_Char(index);` or
(B) `FT_Load_Glyph(FT_Get_Name_Index(name));`); however, with the future
introduction of {xe,lua}tex support, this kind of type checking becomes
inconvenient, because {xe,lua}tex's "dvi glyph indices", which are
directly equal to FreeType glyph indices (i.e. they would be loaded with
`FT_Load_Glyph(index);`), would normally also be converted to ints.

This PR introduces a new API (`Text.index`) that performs this mapping
(via the new `DviFont._index_dvi_to_freetype`), always mapping to
FreeType glyph indices (i.e. one can always just call `FT_Load_Glyph`
on the result).  To do so, in case (A) it loads itself the native
charmap (something the end user needed to do by themselves previously)
and performs the cmap-to-index conversion (`FT_Get_Char_Index`)
previously implicit in `FT_Load_Char`; in case (B) it performs itself
the name-to-index conversion (`FT_Get_Name_Index`).  When {xe,lua}tex
support is introduced in the future, `_index_dvi_to_freetype` will
just return the index as is.

The old APIs are not deprecated yet, as other changes will occur for
{xe,lua}tex support (e.g. font_effects will go away and be replaced by
`.font.effects`, as {xe,lua}tex don't store that info in the pdftexmap
entry), and grouping all API changes together seems nicer (to only
require a single adaptation step by the API consumer).
@story645 story645 added this to the v3.11.0 milestone May 1, 2025
Comment on lines +649 to +651
# - if no ".enc" file is specified, then the font must be a Type 1
# font, and dvi indices directly index into the font's CharStrings
# vector.
Copy link
Member

Choose a reason for hiding this comment

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

just checking but is this why this step is no longer needed: index = t1_encodings[font][glyph_name_or_index]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This still happens at the bottom: self._encoding = face._get_t1_encoding_vector() (likewise caching the value of the encoding vector, but this now occurs on the DviFont object) then return self._encoding[idx].

if self._encoding is None:
psfont = PsfontsMap(find_tex_file("pdftex.map"))[self.texname]
if psfont.filename is None:
raise ValueError("No usable font file found for {} ({}); "
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a test for coverage?

Copy link
Contributor Author

@anntzer anntzer May 7, 2025

Choose a reason for hiding this comment

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

This gets further refactored in the followup PR #29939 into DviFont.path, which does have test coverage (because it is also used elsewhere -- see https://app.codecov.io/gh/matplotlib/matplotlib/pull/29939#4c703021fcf83cd901550e2c525d78ae-R733). I can move code around so that the intermediate stage also has full coverage if you prefer, but this split seems more logical to me.

@story645 story645 merged commit 3da6ea9 into matplotlib:main May 8, 2025
40 checks passed
@anntzer anntzer deleted the dvi2ft branch May 8, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants