search

unified-font-object / ufoNormalizer

A tool that will normalize the XML and other data inside of a UFO.
Other
51 stars 19 forks source link

Fixed bug that caused ufonormalizer to delete glyph files, due to a c… #16

Closed readroberts closed 8 years ago

readroberts commented 8 years ago

…onflict in upper/lower case file names.

moyogo commented 8 years ago

If adding Xy (X_y) and then x_y clashes then it should be fixed in userNameToFileName() instead of every time userNameToFileName() is called. Maybe add existing = [name.lower() for name in existing] under line 3185

    # test for clash
    fullName = prefix + userName + suffix
    existing = [name.lower() for name in existing]
    if fullName.lower() in existing:
        fullName = handleClash1(userName, existing, prefix, suffix)
readroberts commented 8 years ago

The problem with this is performance. The larger Western fonts contain up to 6K glyphs. With your solution, for every time userNameToFileName() is called to check a glyph name, a lowercase version of the existing array is created. I agree with you that my solution is not as good as making a lowercase version of the existing array, but I think it is better to respect the comment at the beginning of userNameToFileName(), and create the lowercase version of the existing list in the functions that call userNameToFileName().

typesupply commented 8 years ago

The names stored in existing are supposed to be case-insensitive. See this note at the top of the function. This function is the example implementation of the user name to file name algorithm defined in the UFO spec.

The two sets that Read highlighted are only used for this file name creation/testing so they are the existing arrays for their respective loops. That the created file names were being stored in these in a case sensitive way was a bug.

moyogo commented 8 years ago

I see. You’re right, my solution is not efficient.

davelab6 commented 8 years ago

Even if not efficient, deleting glyphs seems bad and an inefficient way of not deleting them (and filing an issue to make a more efficient fix) seems better than deleting them.

typesupply commented 8 years ago

Sorry for the delay. I wasn't sure which commit would be merged. I've got it now. Read's solution is being applied.