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

Render small-caps font variant #1611

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

Conversation

samizdatco
Copy link
Contributor

@samizdatco samizdatco commented Jun 24, 2020

  • pango has a PANGO_VARIANT_SMALL_CAPS variant that can be added to the font description, but setting it doesn't cause text to be rendered in small-caps, even if the font supports it
  • adding the appropriate opentype features to the layout's attributes list will use the font's small-caps glyphs if it provides them

Thanks for contributing!

  • Have you updated CHANGELOG.md?

- pango has a PANGO_VARIANT_SMALL_CAPS variant that can be added to the font description, but setting it doesn't cause text to be rendered in small-caps, even if the font supports it
- adding the appropriate opentype features to the layout's attributes list will use the font's small-caps glyphs if it provides them
@samizdatco samizdatco changed the title render small-caps font variant Render small-caps font variant Jun 24, 2020
Copy link
Collaborator

@chearon chearon left a comment

Choose a reason for hiding this comment

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

Thanks for this. Looks good in general. pango_attr_font_features_new is not available in the (old) version of Pango for Windows that we link to in the wiki and use in CI, so we might have to #ifdef PANGO_VERSION(... the new code.

We should also probably change CI testing to use MSYS like the prebuilds do.

src/CanvasRenderingContext2d.cc Outdated Show resolved Hide resolved
- the font-variant state will get out of sync with the fontDescription across context-saves & restores unless we keep a copy of the PangoAttrList around
@samizdatco
Copy link
Contributor Author

I realized that my initial version didn't account for state and would get out of sync with the displayed font after a save() and a restore(). So I've added a field to the state struct called textAttributes that holds a pointer to the layout's current PangoAttrList (though maybe fontAttributes or layoutAttributes would be a better name?).

- the pango NEWS file suggests 1.37.1 was where the `pango_attr_font_features_new` call was added...
- it can be enabled if/when the pango version is updated
test/canvas.test.js Outdated Show resolved Hide resolved
@samizdatco samizdatco mentioned this pull request Jul 2, 2020
1 task
Copy link
Collaborator

@chearon chearon left a comment

Choose a reason for hiding this comment

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

Looking good, I have one comment about state management.

@@ -258,6 +262,8 @@ Context2d::save() {
states[++stateno] = (canvas_state_t *) malloc(sizeof(canvas_state_t));
memcpy(states[stateno], state, sizeof(canvas_state_t));
states[stateno]->fontDescription = pango_font_description_copy(states[stateno-1]->fontDescription);
states[stateno]->textAttributes = pango_attr_list_copy(states[stateno-1]->textAttributes);
pango_layout_set_attributes(_layout, states[stateno]->textAttributes);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't quite rhyme with how we handle the font description on the state (continued below)

} else {
features = pango_attr_font_features_new("");
}
pango_attr_list_change(context->state->textAttributes, features);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a little more code, but you could instead free the attr list on the stack first, then set the a fresh attr list on the layout and stack here. That way it's a little more like the code above and we're not mutating objects on the state.

@jeffhaack
Copy link

It looks like no progress on this in awhile. Is it a complicated fix to add small-caps support in?

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

4 participants