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

Run morx if run is horizontal or GSUB doesn't exist #2130

Merged
merged 1 commit into from
Jan 24, 2020

Conversation

ebraminio
Copy link
Collaborator

@ebraminio ebraminio commented Jan 24, 2020

Related to #2129 and #2124

Dominik, can you check if this still works on those fonts?

@ebraminio ebraminio requested a review from drott January 24, 2020 18:50
{
/* https://github.com/harfbuzz/harfbuzz/issues/2124 */
return hb_aat_layout_has_substitution (face) &&
(HB_DIRECTION_IS_HORIZONTAL (props->direction) || !hb_ot_layout_has_substitution (face));
Copy link
Collaborator Author

@ebraminio ebraminio Jan 24, 2020

Choose a reason for hiding this comment

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

I think semantically hb_ot_layout_has_substitution should itself check hb_aat_layout_has_substitution also but it isn't the case for now. This shows the important of tests, Dominik, can you provide hb-shape input to trigger this? We have macOS bots that have access to such fonts so no issue there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Test input for which part? I only have an example for a GSUB + Morx font in vertical mode, for example:
util/hb-view ~/dev/Mac_Fonts/10_10/System/Library/Fonts/ヒラギノ明朝\ ProN\ W3.otf --direction=ttb "[Hello4]"

I currently do not have an example for a font that has vertical substitution in AAT, but not GSUB. Does that help?

Change LGTM, Chromium bot run completed successfully here:
https://chromium-review.googlesource.com/c/chromium/src/+/2020963

Copy link
Collaborator Author

@ebraminio ebraminio Jan 27, 2020

Choose a reason for hiding this comment

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

Done d809aca Thanks :)

@ebraminio
Copy link
Collaborator Author

Am merging but please have a look. Thanks

@ebraminio ebraminio merged commit f9070cf into harfbuzz:master Jan 24, 2020
@ebraminio ebraminio deleted the morx-policy branch January 24, 2020 21:06
Copy link
Collaborator

@drott drott left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

{
/* https://github.com/harfbuzz/harfbuzz/issues/2124 */
return hb_aat_layout_has_substitution (face) &&
(HB_DIRECTION_IS_HORIZONTAL (props->direction) || !hb_ot_layout_has_substitution (face));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test input for which part? I only have an example for a GSUB + Morx font in vertical mode, for example:
util/hb-view ~/dev/Mac_Fonts/10_10/System/Library/Fonts/ヒラギノ明朝\ ProN\ W3.otf --direction=ttb "[Hello4]"

I currently do not have an example for a font that has vertical substitution in AAT, but not GSUB. Does that help?

Change LGTM, Chromium bot run completed successfully here:
https://chromium-review.googlesource.com/c/chromium/src/+/2020963

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants