Skip to content

FT2Font.get_char_index never returns None. #14870

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 23, 2019
Merged

FT2Font.get_char_index never returns None. #14870

merged 1 commit into from
Jul 23, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 23, 2019

... because its body is

    FT_UInt index;
    FT_ULong ccode;

    if (!PyArg_ParseTuple(args, "k:get_char_index", &ccode)) {
        return NULL;
    }

    index = FT_Get_Char_Index(self->x->get_face(), ccode);

    return PyLong_FromLong(index);

so kill code paths which handle a None-return.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer
FT2Font.get_char_index never returns None.
c91edb3 
... because its body is
```
    FT_UInt index;
    FT_ULong ccode;

    if (!PyArg_ParseTuple(args, "k:get_char_index", &ccode)) {
        return NULL;
    }

    index = FT_Get_Char_Index(self->x->get_face(), ccode);

    return PyLong_FromLong(index);
```

so kill code paths which handle a None-return.
@anntzer anntzer mentioned this pull request Jul 23, 2019
Merged
6 tasks
@timhoffm
Copy link
Member

Is this just a misinterpreted return type? FT_Get_Char_Index() returns 0 for undefined characters (https://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html#ft_get_char_index). So, maybe the checks should just be switched from if gind == None to if gind == 0 (or maybe change FT2Font.get_char_index to actually return None, which is what one would usually do in python).

@anntzer
Copy link
Contributor Author

anntzer commented Jul 23, 2019

Returning zero is actually fine, it results in an empty character (.notdef, i.e. a white rectangle box) being emitted. If anything returning None would be more of a pain because that would need to be converted back to zero when sent back to freetype functions (e.g. freetype will happily compute the kerning between .notdef and a defined character for you, but you can't pass None there).

timhoffm
timhoffm approved these changes Jul 23, 2019
View reviewed changes
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Maybe add a comment to the docstring of FT2Font.get_char_index about returning 0 for undefined chars.

@anntzer
Copy link
Contributor Author

anntzer commented Jul 23, 2019

It's pretty much a straight "does the same thing as the similarly named FT_ C function".

@QuLogic QuLogic merged commit 520b5f6 into matplotlib:master Jul 23, 2019
@QuLogic QuLogic added this to the v3.2.0 milestone Jul 23, 2019
@anntzer anntzer deleted the get_char_index_none branch July 23, 2019 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
topic: text
Projects
None yet
Milestone
v3.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@anntzer @timhoffm @QuLogic