Cache is_justifiable
#2399
Cache is_justifiable
#2399
Conversation
|
@Enter-tainer I am going to leave until late this afternoon (Brussels time), I have just pushed all of the changes I have done, I think this should fix the cache by updating it in the right places, but apparently I must have missed one, do you think you could have a look at it so that I can push it when I get back? Please 😄 |
|
I will take a closer look when I leave work. (aka ~2 or 3h later) 👍 |
|
i'm thinking of changing fields in shapeglyph to private and use setters to access x_advance and stretchability 🤔 this can avoid problem like this |
seems that cjk punctuation adjustment is broken, let me see how to fix it |
I had a look at it, and I did see that some of the spacing was wrong, but since I know nothing about CJK (sorry about that, might one day try and learn Japanese or Chinese) I didn't know where to look for. Thanks a lot @Enter-tainer :D |
|
Overall this PR is great but there is something I concern. As all field of ShapedGlyph is public, there is a chance people change x_advance or something but forget to update is_justifiable. I don't know if there is some good rust pattern to handle this. From another view, after adding is_justifiable, fields in this struct no longer vary independently but has an invariant. Therefore I'd prefer change fields of this struct to private and use getters to get access to them, and only allow mutate these fields through certain setters and accessors -- is_justifiable is maintained in them. But this sounds like a lot of code change 🤔. cc @laurmaedje what do you think? |
|
I think that's a valid concern and a needed change |
|
A lot of the shaping code is in the same module, so as long as we don't move |
Oh i've written too much C++ these days and forgot about that. You are right. So make it private might not work. How do rust people solve this problem? |
|
Sometimes by putting it into a module, sometimes by just not using it wrongly. Depends. For public API, I would also enforce correct usage, but for internal use I think it's okay to enforce some variants manually. There's tons of different ways to break the shaping code that the compiler won't catch. |
Co-authored-by: Pg Biel <9021226+PgBiel@users.noreply.github.com>
|
Thanks! |
| } | ||
|
|
||
| /// Updates the justifiability of the glyph. | ||
| fn update_justifiable(&mut self) { |
There was a problem hiding this comment.
Sorry for the late comment after the PR merged. But the update_justifiable function is not needed. Though the width of a glyph may change due to some CJK adjustments, whether the glyph is justifiable will never change.
There was a problem hiding this comment.
You're absolutely right, the reason why I added the update_justifiable was because I assumed it changes (since it relies on the width) but it does not, I'll open up a follow up PR, Thanks a lot!
| fn is_justifiable( | ||
| c: char, | ||
| script: Script, | ||
| x_advance: Em, | ||
| stretchability: (Em, Em), |
There was a problem hiding this comment.
The x_advance and stretchability arguments are used as heuristics to distinguish between English and Chinese quotation marks. It can be combined to one argument width, which should be x_advance + stretchability.
More information that can put into code comments: The quotations in two language is unfortunately share the same code point in Unicode (U+2018, U+2019, U+201C, U+201D), but quotation marks in Chinese is fullwidth (1em) while in English is usually not. This heuristic will have false positive for monospace English fonts, but it will not hurt much because monospace texts usually need not justifying.
See #2393