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

Fix function description for Font.get_char_size() #88444

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

DaltonSW
Copy link
Contributor

@DaltonSW DaltonSW commented Feb 17, 2024

Bugsquad edit: closes #88348

Removed description implying you can pass a second char in order to account for kerning

Removed description implying you can pass a second char in order to account for kerning
@DaltonSW DaltonSW requested a review from a team as a code owner February 17, 2024 16:00
@AThousandShips AThousandShips added bug documentation cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 17, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Feb 17, 2024
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Current description is literally wrong so yes

@@ -136,7 +136,7 @@
<param index="0" name="char" type="int" />
<param index="1" name="font_size" type="int" />
<description>
Returns the size of a character, optionally taking kerning into account if the next character is provided.
Returns the size of a character. Does not take kerning into account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if mentioning kerning is even necessary anymore. I don't know how any reader would assume kerning is accounted for.
Does it hurt though? No...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, I don't mind either way

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to be explicit.

@akien-mga akien-mga changed the title Fix function description for Font.get_char_size() Fix function description for Font.get_char_size() Feb 17, 2024
@akien-mga akien-mga merged commit 5b968af into godotengine:master Feb 23, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.
Cherry-picked for 4.1.4.

@akien-mga akien-mga removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 11, 2024
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.

Font.get_char_size() documentation is either incorrect or very confusing
4 participants