Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

font-patcher: Cover alternate unicode encodings #1058

Merged
merged 3 commits into from
Jan 23, 2023
Merged

font-patcher: Cover alternate unicode encodings #1058

merged 3 commits into from
Jan 23, 2023

Conversation

nathanielevan
Copy link
Contributor

@nathanielevan nathanielevan commented Jan 19, 2023

Description

[why]
As described in this comment, issue #400 recently reoccurred with the latest build of Input font, and it turns out the dotless-j part of the small j now points to U+0237, which in turn has an alternate unicode encoding to U+F6BE; overwriting U+F6BE effectively overwrites U+0237, and in turn, alters the small j. This patch aims to fix that.

[how]
In addition to references, the patcher also checks for alternate unicode encodings which are returned by the glyph.altuni attribute, adds those to the essential set of glyphs, and in turn recursively searches for their references/alternate unicode encodings, making sure to handle circular references if they appear (for example: U+2010 and U+2011 in Input Mono).

Fixes: #400

Requirements / Checklist

What does this Pull Request (PR) do?

Make font-patcher also check for alternate unicode encodings in addition to references so they won't be overwritten.

How should this be manually tested?

Input Mono so far is the only font I've had this issue with, and this patch successfully prevents font-patcher from overwriting or altering those essential glyphs, but should work with most if not all fonts.

What are the relevant tickets (if any)?

#400

@nathanielevan nathanielevan marked this pull request as draft January 19, 2023 23:43
@nathanielevan nathanielevan marked this pull request as ready for review January 20, 2023 00:14
@Finii
Copy link
Collaborator

Finii commented Jan 20, 2023

Thanks for reporting and of course for the nice PR!
Really appreciate the commit style 🎉 etc.

I will have a closer look at the weekend.

@Finii Finii added this to the v2.3.2 milestone Jan 20, 2023
@Finii
Copy link
Collaborator

Finii commented Jan 20, 2023

I wonder if the altuni part needs to be handled at all in self.essential, of if the check while patching should be "check if this glyph's codepoint or any of its alternative codepoints is in essential".

But then, that would probably be more work because we need to check more often. And the altuni bindings will not go away or something, so yes, probably this is best.

Copy link
Collaborator

@Finii Finii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thank you.

Feel free to add some more comment in the code and/or use list comprehension if you like 😬

Tell me if I shall merge or if you want to add something before.
Again your commit style and the thought you put into this 👍 💚

font-patcher Outdated Show resolved Hide resolved
font-patcher Outdated Show resolved Hide resolved
@Finii
Copy link
Collaborator

Finii commented Jan 20, 2023

Ah I forgot, would be good to increase the script version.

@nathanielevan
Copy link
Contributor Author

Done! Let me know if you have any more recommendations, or whether I introduced bugs etc

@Finii
Copy link
Collaborator

Finii commented Jan 22, 2023

Did you test it with Input? I do not have that avail right away 😬

Looks very good ❤️

@nathanielevan
Copy link
Contributor Author

nathanielevan commented Jan 22, 2023

I did, and the glyph that was previously broken seems to have been fixed! But if you do have the font in hand you might want to test it just to make sure I didn't mess anything up. If you do decide to merge, you'd want to change the version number as it's now outdated -- that's where the branch conflict is from

[why]
Issue #400 recently reoccurred with the latest build of Input font, and
it turns out the dotless-j part of the small `j` now points to U+0237,
which in turn has an alternate unicode encoding to U+F6BE; overwriting
U+F6BE effectively overwrites U+0237, and in turn, alters the small `j`.
This patch aims to fix that.

[how]
In addition to references, the patcher also checks for alternate unicode
encodings which are returned by the glyph.altuni attribute, adds those
to the essential set of glyphs, and in turn recursively searches for
their references/alternate unicode encodings, making sure to handle
circular references (for example: U+2010 and U+2011 in Input Mono)
[why]
Using `continue` feels inelegant when there's a way to write the if
conditions in add_glyphrefs_to_essential() without necessitating the use
of `continue` while ensuring that the function still works as intended.

[how]
Change the `if` conditions and remove any usage of the `continue`
keyword in add_glyphrefs_to_essential().
[why]
List comprehension helps with readability. Also add comments that
describe expected data structures of altuni and references. Also bump up
the patcher version number.

[how]
Use list comprehension. Add comments. Change the version number.
@Finii
Copy link
Collaborator

Finii commented Jan 23, 2023

Rebase on master, force push

@Finii
Copy link
Collaborator

Finii commented Jan 23, 2023

Tried to switch this to some kind of tail-recursion, but the code will end up very ugly.

Also noticed that Input Mono indeed contains refs and alt-codes in the extended latin range.

Very nice code and PR, thanks again.

@Finii Finii merged commit bdf9c24 into ryanoasis:master Jan 23, 2023
@nathanielevan
Copy link
Contributor Author

Sweet! I basically rely on Nerd Fonts for my day-to-day computer usage, so a little code is the least I can give back. Should I invoke the contributors bot as described here?

@Finii
Copy link
Collaborator

Finii commented Jan 23, 2023

*lauch* yes please, I just searched for that syntax also, to add you :-)

@Finii
Copy link
Collaborator

Finii commented Jan 23, 2023

Hey @all-contributors please add @nathanielevan for code

@allcontributors
Copy link
Contributor

@Finii

I've put up a pull request to add @nathanielevan! 🎉

@Finii
Copy link
Collaborator

Finii commented Jan 23, 2023

I'm not very knowledgeable with Python

To be honest .. I really struggle to take Python seriously, and Nerd Fonts is the only project where I actually use it.

🤔 No, meson uses it also I and contributed there. But not much.

@Finii
Copy link
Collaborator

Finii commented Jan 23, 2023

Thinking... Hope my release workflow will not break, because we added your contributor commit.
There was a problem when the repo is touched after the release has been started 🤔 😒
Wanted to save time by not always pulling again before pushing.
Lets see...
https://github.com/ryanoasis/nerd-fonts/actions/runs/3984511030

@Finii
Copy link
Collaborator

Finii commented Jan 23, 2023

😭

image

LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
font-patcher: Cover alternate unicode encodings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Patching InputMono overwrites the letter j
2 participants