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

Rename text alignment settings #561

Merged
merged 2 commits into from
Mar 5, 2021

Conversation

rfuest
Copy link
Member

@rfuest rfuest commented Mar 4, 2021

This PR changes the names of the HorizontalAlignment and VerticalAlignment enums to Alignment and Baseline. The reason for this change is that VerticalAlignment might be misleading, because the vertical alignment does only apply to the first line of multiline text:
Bildschirmfoto von 2021-03-04 18-19-46
The text on the left is the actual result for Baseline::Middle and the right is an example of what users might expect for VerticalAlignment::Center. This also matches the names used in the HTML5 canvas API: https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/textBaseline, https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/textAlign

The PR also included some tweaks to the TextRenderer trait. I've removed the vertical_offset method, which was intended as a performance optimization. The potential grain of saving a few additions and subtractions per text object isn't worth having an more error prone API.

@@ -423,6 +424,7 @@ mod tests {
}

#[test]
#[ignore]
Copy link
Member Author

Choose a reason for hiding this comment

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

Ignored because of #562

Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

LGTM, and the reasoning makes sense.

I haven't been following the text changes too closely, but does this need a breaking changelog entry for any behavioural changes since the last alpha?

@rfuest
Copy link
Member Author

rfuest commented Mar 4, 2021

I haven't been following the text changes too closely, but does this need a breaking changelog entry for any behavioural changes since the last alpha?

The behavior shouldn't have changes, but I've added changelog entries for the naming changes and the changed text renderer API.

Copy link
Member

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

Just a few nits. Otherwise, looks good.

Baseline,
/// Middle.
Middle,
/// Alphabetic baseline.
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 these could/should be explained better than just echoing the variant names.

For example I'm not entirely sure what Alphabetic refers to, although I have a guess. Also, does Top refer to the topmost pixel of the character, or something else? Perhaps we could adapt the text from the linked canvas docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

The docs need some work in general, I'll add a note to the tracking issue to revisit this later.

For example I'm not entirely sure what Alphabetic refers to, although I have a guess.

The main reason I changed it from Baseline to Alphabetic is that Baseline::Baseline would look weird. But it also matches the canvas API. I don't think we will ever support ideographic or hanging baselines in the internal text renderer.

Also, does Top refer to the topmost pixel of the character, or something else?

It refers to the top of the glyph bounding box. This can include additional blank pixels above the caps if the font includes them.

Copy link
Member

@bugadani bugadani Mar 5, 2021

Choose a reason for hiding this comment

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

I mean, I know what these are (although I had to think about alphabetic for a moment), but it's not obvious from the docs :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I know what these are, ... ,but it's not obvious from the docs :)

The docs for the font/text stuff really need some work. Contributions to the docs are always welcome 😉

src/text/text.rs Show resolved Hide resolved
@rfuest rfuest merged commit 8a8a22b into embedded-graphics:master Mar 5, 2021
@rfuest rfuest deleted the rename-alignment branch March 5, 2021 15:34
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

3 participants