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

Warn when asked to measure a character not shaped in a font #2193

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alerque
Copy link
Member

@alerque alerque commented Dec 9, 2024

Closes #1706

@alerque alerque added this to the v0.15.8 milestone Dec 9, 2024
@alerque alerque added the enhancement Software improvement or feature request label Dec 9, 2024
shapers/base.lua Outdated Show resolved Hide resolved
shapers/base.lua Outdated Show resolved Hide resolved
shapers/base.lua Outdated
)
)
end
return { height = item.height, width = item.width, depth = item.depth, name = item.name }
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need returning the name? Could be the GID, or just a boolean "undefined = true", so the caller wouldn't have to perform a ".notdef" explicit check. The name might not be meaningful or portable any for most extended characters (ligatures, etc.), would it?

Copy link
Member

@Omikhleia Omikhleia Dec 9, 2024

Choose a reason for hiding this comment

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

N.B. I am not fond of warnings is such utilities. The caller could try to measure char X, and fallback to char Y if X is not defined, without low-level warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I think we need to return the name. That way downstream users of the API can make their own call about whether to use the results or not. Actually I just took it a step farther and passed the entire table of info we got back from the shaper. That includes all the keys currently used and also some information about offsets that could be useful for some measurement scenarios. Also it includes the GID and name in case the consumer wants to check them.

My rational for including the warning was that we have better access to the font name and such here in the function than the user does easily when using the function so it was easier to create a meaningful warning. I'm not set on it though, so lets try a different shape ... returning a boolean status flag might be pretty useful too. How does that look?

shapers/base.lua Outdated Show resolved Hide resolved
@alerque alerque requested a review from a team as a code owner December 11, 2024 12:53
@alerque alerque requested a review from Omikhleia December 11, 2024 13:05
@alerque
Copy link
Member Author

alerque commented Dec 12, 2024

The fact that this failed a test may have just revealed a big lurking bug, but I don't have time to pull it apart right now and I urgently need a tagged release so I can send a book to press with a known version. I'm going to bump this and release dot-8, but I'm open to a quick follow up patch release if we figure this or anything else out. Right now macOS being 2 versions behind because I broke Darwin builds is a big deal and that needs to get fixed ASAP.

@alerque alerque modified the milestones: v0.15.8, v0.15.9 Dec 12, 2024
return { height = items[1].height, width = items[1].width, depth = items[1].depth }
if items and items[1] then
local item = items[1]
return item, item.gid ~= 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This code (even before this PR) seems to assume that a character will be mapped to only a single glyph, but this is not true in general.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Software improvement or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shaper:measureChar can return the width of a .notdef character
3 participants