Tuesday, January 5, 2016

Possible 15 year old regression

A few days ago, I filed bug 96826 ("Typewriter attribute not given enough weight when finding font based on attributes") against LibreOffice. Basically, I've been refactoring VCL font code and I was very interested to see what my predecessors had been doing before me. This meant that I actually read pretty much all of the commits I could find all the way back to the year 2000 for that file, which held much of the font code for VCL. 

One of the functions was originally in a class called ImplFontCache, which had a get() function that worked as a font mapper. It basically does a running total of a number of weighted values depending on the "strength" or "quality" of the font matching attribute. One of those attributes was to check if the font is a fixed-width font, then check to see if the font is a typewriter style font, giving a deliberately higher weighting to typewriter style fonts. 

Anyway, on the 29th June 2001 someone with the initials th (no idea who this was, nor is it really important) did some tweaks to the function in an attempt to improve font mapping in commit 925806de6. To ensure that the typewriter style was given a weighted value, they have multiplied the value by 2. However, they appear to have accidentally removed a zero from the weighting, thus reducing the weighting of the typewriter style fonts, not increased it!

As I was reading the code, I thought "whoops, that was unfortunate" and assumed I'd see the error picked up somewhere down the track and fixed. Now either the weighting has little effect on these fonts, or not many people have problems with typewriter fonts not being cleanly mapped by the underlying platform, but this has never been fixed

The code has since migrated its way into PhysicalFontCollection::FindFontFamilyByAttributes(), and it's still there!

    610         // test MONOSPACE+TYPEWRITER attributes
    611         if( nSearchType & ImplFontAttrs::Fixed )
    612         {
    613             if( nMatchType & ImplFontAttrs::Fixed )
    614                 nTestMatch += 1000000*2;
    615             // a typewriter attribute is even better
    616             if( ImplFontAttrs::None == ((nSearchType ^ nMatchType) & ImplFontAttrs::Typewriter) )
    617                 nTestMatch += 10000*2;
    618         }

As with any codebase with a 30 year pedigree, I've not committed a fix because I just don't know if this is something I want to touch. Hence I've logged the bug to see if anyone can shed any light on it. I'll probably follow up on the mailing list in a few weeks time to ping any more experienced developers, but until then this potential bug, which is in the middle of going through puberty, will remain. 

No comments:

Post a Comment