Skip to content

Font::glyph doesn't work the way its users expect

Created by: jimblandy

In my (slowly proceeding) work on #84 (closed), I noticed that Font::glyph returns Option<Glyph<'a>>. The doc comment says:

If id corresponds to a glyph identifier, the identifier must be valid (smaller than self.glyph_count()), otherwise None is returned.

Note that code points without corresponding glyphs in this font map to the "undef" glyph, glyph 0.

In other words, invalid glyph indices elicit None, and invalid char indices produce Some(glyph) where glyph.id is zero, for the .notdef character.

Looking up a char or Codepoint in a font can reasonably fail; since both the text being displayed and the choice of font are often under users' control, this can only be handled effectively as a run-time error.

But glyph ids are direct indices into the font's glyph table, and can only reasonably be produced by some prior lookup in the font's maps. By returning None when passed a GlyphId that was apparently looked up some other font, this function is inviting callers to try to handle a case that probably reflects a coding error.

I think it would be more helpful to our users for Font::glyph to panic when passed a bad GlyphId, analogous to the way arrays treat out-of-bound indices. Then it can simply return Glyph<'a>, without the Option.

Impact

Among rusttype's ten most-downloaded users, five use Font::glyph. All pass char, so None cannot possibly be returned.

  • Two (conrod, piston2d-graphics) just apply unwrap to the result. Since they passed a char, this is correct.
  • The font-atlas checks for None and propagates that to its caller as an indication that the character is not handled by the font. This is a bug: it should be checking for the .notdef glyph. Perhaps the authors saw Option in the return type and jumped to conclusions?
  • The gfx-glyph and radiant-rs check for None and drop the character from the string. This is also a bug (albeit a harmless one): since they pass char, they will never encounter a None.

Changing Font::glyph's return type to Glyph<'a> would simplify all these callers, by removing .unwrap() calls and unnecessary error-handling code. Only font-atlas requires the addition of any code at all.

(These users seem well-served by returning a .notdef glyph when given a char not covered by the font: other characters will display normally, and the user will see the notdef glyph, which seems to be more helpful than error-ing out or panicking.)