Skip to content

FontFactory refactoring

PBS requested to merge pbs3141/inkscape:fontrefactorying into master

This MR is mainly a refactoring of FontFactory. Although there was nothing wrong with it, I found it difficult to follow and so I cleaned it up. The reason I was looking at it in the first place was because fonts are one of several points to consider when making rendering multithreaded.

The changes are the following:

  • Delete the unused header font-style.h.
  • Rename the remaining headers to be more consistent: FontFactory.h to font-factory.h.
  • Rename some classes to match style guidelines: font_factory to FontFactory.
  • Get rid of NR_LIB. (As far as I can see the only reason for keeping libnrtype a separate library is historical. If there's a good reason, please tell me so I can put it back!)
  • Remove USE_PANGO_WIN32. (It has been commented out since 2004, enables deprectated Pango calls, and doesn't appear to be enabled by the build system. Again, if someone points out that this is in error, I'll put it back.)
  • C++ification - e.g. adding const, private/public, smart pointers, exception handling, etc. Using exceptions actually simplified FontInstance a lot because it previously had to check for a failure state all the time, in case construction failed. Now the constructor just throws before it can enter a failure state.
  • FontInstance refcounting traded for shared pointers. This is as things should be, because FontInstances shouldn't care how they are used, e.g. whether they are refcounted or not.
  • Split out the font caching logic into a generic template class cached_map. This is also how things should be, because caching is a generic concept that has nothing to do with fonts.
  • Ensure FontInstance pixbufs and glyph curves are immutable. This is the main motivation for doing all this. With only minimal changes, mostly the addition of const, I can now be sure that if I start using these pixbufs and glyphs in a mutlithreaded way, accidental screwups involving reads/writes from different threads cannot easily be added in the future. (The same guarantees Rust provides, only enforced by const rather than borrow checker and therefore easier to break, but good enough.)

Another separate but closely related change snuck in too. While making FontInstance's pixbufs and glyphs immutable, I also made SPImage's pixbuf immutable too. Again, this didn't really require any changes other than addition of const and a few safe const_casts.

I've been testing this for a while and the font cache definitely still works, but I'm slapping needs-testing on it all the same.

Merge request reports