Skip to content

Commit

Permalink
hb_buffer_get_glyph_positions can return NULL
Browse files Browse the repository at this point in the history 
During shaping tracing before applying GPOS (at least in HarfBuzz
2.8.2).
  • Loading branch information
@khaledhosny
khaledhosny committed Jul 23, 2021
1 parent 5ed0ff2 commit 9988e4f
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/uharfbuzz/_harfbuzz.pyx
View file
Expand Up @@ -143,6 +143,8 @@ cdef class Buffer:
cdef unsigned int count
cdef hb_glyph_position_t* glyph_positions = \
hb_buffer_get_glyph_positions(self._hb_buffer, &count)
if glyph_positions is NULL:
return None
cdef list positions = []
cdef GlyphPosition position
cdef unsigned int i
Expand Down

7 comments on commit 9988e4f

@justvanrossum
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not quite happy with this, as it's a breaking change. I wonder if uharfbuzz did something wrong before? Did the behavior of hb_buffer_get_glyph_positions really change in HB 2.8.2? Sounds like it may be a breaking change for HB itself, too, no? @behdad?

@khaledhosny
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think it is a deliberate change. This happens only during tracing though, which I don't think is widely used.

@justvanrossum
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have some production code that I'll likely have to adapt, but I may be the only one :)

@khaledhosny
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can simulate the old behavior if you feel the change is too much. Here is the relevant commit, check to see if you prefer to adapt to the new behavior harfbuzz/harfbuzz@c61ce96

@justvanrossum
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, after reading the discussion before that I think I have more reworking to anyway, so let's just stick with this.

@behdad
Copy link
Member

@behdad behdad commented on 9988e4f Jul 26, 2021

Choose a reason for hiding this comment

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

I'm not quite happy with this, as it's a breaking change. I wonder if uharfbuzz did something wrong before? Did the behavior of hb_buffer_get_glyph_positions really change in HB 2.8.2? Sounds like it may be a breaking change for HB itself, too, no? @behdad?

The change only affects message funcs. Since those are not really used in production, I was comfortable releasing this.

If your code crashes now, then it was wrong before. Because calling hb_buffer_get_glyph_positions() would clear the position buffer; if buffer didn't have positions yet, you were supposed to NOT call this function. If you did, it was clearing data that was needed for proper substitution.

By making this change, we do not let you accidentally shoot yourself in the foot.

@justvanrossum
Copy link
Collaborator

Choose a reason for hiding this comment

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

By making this change, we do not let you accidentally shoot yourself in the foot.

Thanks for the further explanation! All good.

Please sign in to comment.