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

Bug/Feature(?): make a version of find_glyph for ttf::Font that supports characters beyond the Basic Multilingual Plane #1383

Open
CaspianA1 opened this issue Apr 6, 2024 · 4 comments · May be fixed by #1414
Labels

Comments

@CaspianA1
Copy link

The current implementation of find_glyph for ttf::Font looks like this:

/// Returns the index of the given character in this font face.
pub fn find_glyph(&self, ch: char) -> Option<u16> {
    unsafe {
        let ret = ttf::TTF_GlyphIsProvided(self.raw, ch as u16);
        if ret == 0 {
            None
        } else {
            Some(ret as u16)
        }
    }
}

There is a major problem with this: if the input char is something like the 🤣 emoji (which has a numerical value of 129315), it'll get cast to u16 and become 63779. This can then lead to false positives, where fonts say that they support a UTF-8 codepoint, when they do in fact not; and vice versa for fonts that aim to only support emoji-like characters (like plane 1 in GNU Unifont).

The solution to this would be simple: either patch this function to use TTF_GlyphIsProvided32 instead (and without the u16 cast), or make a separate function called find_glyph_32.

Please fix this! It would be enormously helpful.

@CaspianA1 CaspianA1 changed the title Make a version of find_glyph for ttf::Font that supports characters beyond the Basic Multilingual Plane Bug: make a version of find_glyph for ttf::Font that supports characters beyond the Basic Multilingual Plane Apr 6, 2024
@CaspianA1 CaspianA1 changed the title Bug: make a version of find_glyph for ttf::Font that supports characters beyond the Basic Multilingual Plane Bug/Feature(?): make a version of find_glyph for ttf::Font that supports characters beyond the Basic Multilingual Plane Apr 6, 2024
@Cobrand Cobrand added the bug label Apr 12, 2024
@Cobrand
Copy link
Member

Cobrand commented Apr 12, 2024

Looks like a bug, but it requires a breaking change to fix (fortunately rather easy to fix on the client's side)

@CaspianA1
Copy link
Author

@Cobrand how could that be fixed client-side??

Also - a non-breaking change would be to introduce a new function 'find_glyph_32' that fixes this.

@Cobrand
Copy link
Member

Cobrand commented Apr 15, 2024

Here is how I understand it:

  • We change the call from TTF_GlyphIsProvided to TTF_GlyphIsProvided32, and remove the cast.
  • We change the output from a Option<u16> to Option<u32>, which is a breaking change which will be required to be fixed on the client side, this is what I was talking about.

But it's true we can also add a find_glyph_32, and put the other function as to be used only when absolutely necessary, but I don't really like this idea because the first function already takes a char as input, so it implies that it handles correctly every char we can have.

@CaspianA1
Copy link
Author

@Cobrand I agree, adding a find_glyph_32 would work, but it would be an ugly fix, given that find_glyph is supposed to work for characters outside the BMP.

Wouldn't it be worth a breaking change in this case though? The current version of find_glyph just won't work at all for chars above 65535, which means that it is impossible to use this function for any emojis, or really, any character in the Supplementary Multilingual Plane. That seems major enough to warrant a breaking change, I think..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants