mdumic/cufon — comment on commit 82c82828af2527b62f0fc803b96b8791dbbed5c9
File:  line 0
By sorccu:

I like it, but there are a few things I would change:

Would like to have:

1. Please no deleting / changing empty lines, I'm not saying the original is perfect either, but those changes unnecessarily complicate the patch and are irrelevant anyway.
2. Please no whitespace between "function" and "(". Again, I'm not saying that either style is wrong, this is just to keep the coding style consistent.
3. Remove the semicolon after the if () {} inside the for. To keep the coding style consistent I would perhaps add {} to the for because it's not a one-liner.
4. Add a semicolon after the last function in the return, this time it's an expression so a semicolon would be nice to have.
5. I see 4-wide and 2-wide indents... could be just GitHub formatting, but make sure that all indents are tabs. Again, for consistency.

Must have:

1. Move latinLigatures inside Font (or possibly even inside applyLigatures to keep almost all the changes in one place). Yes, it will be evaluated once for each Font instance, but it won't make any real difference because the instances are so few.
2. Text nodes, on the other hand, can be quite numerous and function calls are relatively expensive. Ligatures are also quite rare (well, mainly because the generator filters them out) so this is an unnecessary penalty. Property access is quite a bit faster, so I would prefer something like:

Cufon.CSS.textTransform(font.applyLigatures ? font.applyLigatures(text) : text, style).split('');

which I presume to be somewhat faster (although benchmarks would be nice as well).

3. There must be a JS option to disable ligatures altogether. Some of the font ranges in the generator (namely Latin-1 Supplement) include ligatures. They are filtered out for now, but maybe not in the future. I am not sure if the option should be enabled or disabled by default, but I have a feeling that having it enabled by default could lead to some users getting surprised. Unfortunately, not many people even know what ligatures are and would not know what to look for to disable the feature.

Can't be helped:

1. Having to change both renderers sucks, but it is indeed the way it has to be for now.

I will pull this if you fix these issues :)