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

Reject font values that are invalid for pango #7092

Merged
merged 3 commits into from
Jul 1, 2022

Conversation

WhyNotHugo
Copy link
Contributor

Use pango to parse font configuration early, and reject the command as
invalid if the value is invalid for pango. Since we're already parsing
the font into a PangoFontDescription, keep that instance around and
avoid re-parsing the font each time we render text.

Fixes: #6805

Hugo Osvaldo Barrera added 2 commits June 29, 2022 21:42
Use pango to parse font configuration early, and reject the command as
invalid if the value is invalid for pango. Since we're already parsing
the font into a `PangoFontDescription`, keep that instance around and
avoid re-parsing the font each time we render text.

Fixes: swaywm#6805
Comment on lines 245 to +246
if (!(config->font = strdup("monospace 10"))) goto cleanup;
config->font_description = pango_font_description_from_string(config->font);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we call the font command here with arg "monospace 10"?

We'd be able to de-duplicate some font-configuration bits, and also drop the
extra call to config_update_font_height below (which is kind of a hack). In
fact, we can move the logic from config_update_font_height into the font
command itself, since the font height only changes when the font changes, and
drop the separate config_update_font_height entirely.

Does this seem reasonable?

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not execute commands when initializing data structures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll re-think how to optimise that further and address it separately then. It's a non-issue in this scope.

@WhyNotHugo
Copy link
Contributor Author

Results with different values:

4

2

3

1

@emersion
Copy link
Member

emersion commented Jul 1, 2022

This LGTM!

Follow-up issue: also pass a parsed PangoFontDescription to other text-related functions like render_text, but that's a more involved change.

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

This LGTM!

Follow-up issue: also pass a parsed PangoFontDescription to other text-related functions like render_text, but that's a more involved change.

Avoids parsing the configured font each time text is rendered.
@WhyNotHugo
Copy link
Contributor Author

I'd missed that one! That was quicker than expected (took less than my lunch in the oven 😂 ).

I wonder if swaynag and swaybar should fall back to "monospace 10" if the given input is invalid too. That's out of scope tho; the current versions will also misrender if the input font is invalid.

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Nice, the new commit LGTM as well, thanks!

@emersion emersion merged commit 80e386f into swaywm:master Jul 1, 2022
@WhyNotHugo WhyNotHugo deleted the validate-font-with-pango branch July 1, 2022 11:29
@bl4ckb0ne bl4ckb0ne mentioned this pull request Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Text in title bars is not rendered when custom font used
2 participants