Fix lvfontio full width glyphs#10865
Conversation
…econd slot of full width glyph overriding
tannewt
left a comment
tannewt
left a comment
There was a problem hiding this comment.
Thanks for working on this! Just a couple of things.
| // Don't evict one half of a full-width pair while the other half is still in use. | ||
| if (slot > 0 && self->codepoints[slot - 1] == codepoint && self->reference_counts[slot - 1] > 0) { | ||
| return true; | ||
| } | ||
| if (slot + 1 < self->max_glyphs && self->codepoints[slot + 1] == codepoint && self->reference_counts[slot + 1] > 0) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I think there is an edge case here where the slot if at the start or end. Instead of just testing the bounds, you'll want to do the + or - and then mod to wrap around.
| // Ignore zero-advance glyphs when inferring the terminal cell width. | ||
| // Some fonts include placeholders/control glyphs with zero advance, | ||
| // which would otherwise skew default_advance_width too small. | ||
| if (glyph_advance > 0) { |
There was a problem hiding this comment.
I think this would be slightly easier to read as an if (glyph_advance == 0) { first condition that falls through to the rest.
|
|
||
| // Check if this is a full-width character by looking for a second slot | ||
| // with the same codepoint right after this one | ||
| bool cached_is_full_width = existing_slot + 1 < self->max_glyphs && |
There was a problem hiding this comment.
This will likely have the same wrapping issue. I think my intent was to have one reference count in the left side of the glyph.
|
Those changes are made in the latest commit. Updated both spots with modulo logic for wrapping, and changed the structure of the if statement for clarity. I retested the latest version on Fruit Jam and confirmed full width glyph is still showing as expected. |
2 participants
This PR has 3 fixes for lvfontio.
header.default_advance_widthfrom getting set to the wrong value. With Unifont 8x16/16x16 font the value of this was ending up as4which caused the right half of full-width glyphs not to render properly. Resolves: lvfontio only renders the left part of full width glyphs #10864. Fixed by ignoring zero advance glyphs when inferring the cell width.NULLlast parameter. The LLM suggested this fix without explicit prompt while working on this file. I am unsure about it, but I did find that all of the other instances of f_read() in this file use the same kind ofbytes_readvar for this argument, so the fix makes this instance match the rest.find_free_slot(). This one I even less sure about, but was also suggested by the LLM as a potential fix for only the left half of the glyphs showing, albeit not actually fixing that issue in practice. Resolved by addingslot_has_active_full_width_partner()and checking it before returning the slot.Tested fix on Fruit Jam and confirmed it now shows the full width glyph:
Debugging and the proposed fix for this issue were done with help from LLM