Pomax / lib-font

This library adds a new Font() object to the JavaScript toolbox, similar to new Image() for images
MIT License
730 stars 72 forks source link

Fix some CMAP supports() functions #67

Closed belluzj closed 4 years ago

belluzj commented 4 years ago

I think the current implementation of some of the CMAP supports() functions were basically returning true all the time. I fixed one of them and applied the same fix to the four of them that were using findIndex(). I updated a bit the unit tests but nowhere near thoroughly enough.

Pomax commented 4 years ago

Can you explain this change in a bit more detail? It seems to remove a lot of code while replacing it with something that looks... less thorough.

(Not to say it won't work as intended, but I'd like to understand this from a "does it follow the spec" level =)

belluzj commented 4 years ago

Here is the what the spec says about format 4:

You search for the first endCode that is greater than or equal to the character code you want to map. If the corresponding startCode is less than or equal to the character code, then you use the corresponding idDelta and idRangeOffset to map the character code to a glyph index

Basically what this says is:

I assumed that this library doesn't have to be super fast so I didn't implement a binary search, just used the findIndex() function as you were doing before, which is a linear search, and I also assumed from the function name that it should only return a boolean, and not actually try to map the character code to the matching glyph index (which it looks like what the previous code was trying to do but that computation looked wrong), hence the resulting line of code: just find whether there exists any segment that covers the given character code, using a simple linear search.

Now that I've written this, it means that the code 0xFFFF will be returned as supported even though it probably isn't.

Does that answer your question?