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

Make hypertext[] respect font size settings #13858

Merged
merged 1 commit into from Oct 16, 2023

Conversation

grorp
Copy link
Member

@grorp grorp commented Sep 29, 2023

While testing #13850 on Android, I noticed that the text rendered by hypertext[] was larger than other text. hypertext[] has a hardcoded default font size of 16 and doesn't respect font_size and mono_font_size, which are set to 14 on Android by default.

Since modders can specify absolute font size values for hypertext[], just using the mentioned settings as default values instead isn't enough. What this PR does is this:

font_size = myround(font_size / 16.0f * g_fontengine->getFontSize(font_mode)); 

To do

This PR is a Ready for Review.

How to test

Set font_size to 10, set mono_font_size to 30. Execute /test_formspec in a Devtest world, go to the "Hypertext" tab and see that your font size settings are applied.

@grorp grorp added Bugfix 🐛 PRs that fix a bug Formspec labels Sep 29, 2023
@SmallJoker
Copy link
Member

SmallJoker commented Oct 1, 2023

A proper solution would be to use multipliers (e.g. 0 ... 5) to scale the font based on the client's settings. The API is marked as unstable in the docs, hence this behaviour could in theory be changed. However, I am sure several mods already rely on the fontsize property. If compatibility is desired, the formspec version could be used to differentiate between the two notations (absolute and new relative).

Opinions?

@grorp
Copy link
Member Author

grorp commented Oct 1, 2023

A proper solution would be to use fractions (e.g. 0 ... 5) to scale the font based on the client's settings. The API is marked as unstable in the docs, hence this behaviour could in theory be changed. However, I am sure several mods already rely on the fontsize property. If compatibility is desired, the formspec version could be used to differentiate between the two notations (absolute and new relative).

Opinions?

If I understand your proposal correctly, doesn't that just mean replacing the current 0...16 range with a 0...5 range?

@SmallJoker
Copy link
Member

@grorp No, the proposed change is to have a multiplier (edited the comment accordingly) in place of the current absolute font size.

@grorp grorp added this to the 5.8.0 milestone Oct 9, 2023
@grorp
Copy link
Member Author

grorp commented Oct 9, 2023

Added to milestone as written on IRC: https://irc.minetest.net/minetest-dev/2023-10-08#i_6120697

I think the approach of this PR is good enough, but I'd also be fine if someone implemented a different solution.

Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

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

This implementation looks fine to me.

tests

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

srifqi appears to use a noticeably bigger mono font size (setting value) to test this change. The hypertext element does look "more correct" when two similar or identical scales are chosen.
Works.

@grorp grorp merged commit 6fdc7e0 into minetest:master Oct 16, 2023
13 checks passed
@srifqi
Copy link
Member

srifqi commented Oct 17, 2023

srifqi appears to use a noticeably bigger mono font size (setting value) to test this change.

Correct. I used the values in the first post to test it.

How to test

Set font_size to 10, set mono_font_size to 30. Execute /test_formspec in a Devtest world, go to the "Hypertext" tab and see that your font size settings are applied.

@grorp grorp deleted the hypertext-font-size-fix branch December 18, 2023 16:39
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants