Skip to content

Text Rework cleanup#15887

Merged
alice-i-cecile merged 12 commits intobevyengine:mainfrom
MiniaczQ:text-rework-followup
Oct 15, 2024
Merged

Text Rework cleanup#15887
alice-i-cecile merged 12 commits intobevyengine:mainfrom
MiniaczQ:text-rework-followup

Conversation

@MiniaczQ
Copy link
Copy Markdown
Contributor

@MiniaczQ MiniaczQ commented Oct 13, 2024 •
edited
Loading

Objective

Cleanup naming and docs, add missing migration guide after #15591

All text root nodes now use Text (UI) / Text2d.
All text readers/writers use Text<Type>Reader/Text<Type>Writer convention.

Migration Guide

Doubles as #15591 migration guide.

Text bundles (TextBundle and Text2dBundle) were removed in favor of Text and Text2d.
Shared configuration fields were replaced with TextLayout, TextFont and TextColor components.
Just TextBundle's additional field turned into TextNodeFlags component,
while Text2dBundle's additional fields turned into TextBounds and Anchor components.

Text sections were removed in favor of hierarchy-based approach.
For root text entities with Text or Text2d components, child entities with TextSpan will act as additional text sections.
To still access text spans by index, use the new TextUiReader, Text2dReader and TextUiWriter, Text2dWriter system parameters.

@MiniaczQ MiniaczQ added D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Text Rendering and layout for characters labels Oct 13, 2024
@MiniaczQ MiniaczQ removed the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Oct 13, 2024
@MiniaczQ MiniaczQ changed the title [Text Rework](#15591) cleanup Text Rework cleanup Oct 13, 2024
@MiniaczQ MiniaczQ marked this pull request as ready for review October 13, 2024 19:27
@MiniaczQ MiniaczQ added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Oct 13, 2024
@MiniaczQ
Copy link
Copy Markdown
Contributor Author

MiniaczQ commented Oct 13, 2024

I'm also thinking whether this should reintroduce deprecated Text2dBundle and TextBundle so people get a deprecation message with migration instructions
@alice-i-cecile

@MiniaczQ MiniaczQ removed the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Oct 13, 2024
ickshonpe
ickshonpe reviewed Oct 13, 2024
View reviewed changes
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Oct 13, 2024

I really like most of these changes :) If we revert the TextUi change IMO it's uncontroversial.

@JMS55 JMS55 added this to the 0.15 milestone Oct 13, 2024
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Oct 14, 2024

I'm also thinking whether this should reintroduce deprecated Text2dBundle and TextBundle so people get a deprecation message with migration instructions @alice-i-cecile

Yes please!

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 14, 2024
@MiniaczQ MiniaczQ added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 14, 2024
@MiniaczQ MiniaczQ added the X-Uncontroversial This work is generally agreed upon label Oct 14, 2024
ickshonpe
ickshonpe suggested changes Oct 14, 2024
View reviewed changes
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 14, 2024

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@MiniaczQ MiniaczQ force-pushed the text-rework-followup branch from a58c01c to 695a9f7 Compare October 14, 2024 12:07
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 14, 2024
ickshonpe
ickshonpe approved these changes Oct 14, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@ickshonpe ickshonpe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Seems fine. I'd prefer if TextUiReader/Writer was just called TextReader/Writer but then we'd need to rename the underlying type. It's not worth fussing over for now.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 15, 2024
Merged via the queue into bevyengine:main with commit f602eda Oct 15, 2024
@alice-i-cecile alice-i-cecile mentioned this pull request Oct 15, 2024
Merged
This was referenced Nov 15, 2024
Open
Closed
@alice-i-cecile alice-i-cecile mentioned this pull request Nov 18, 2024
Merged
Aravind-blip
Aravind-blip reviewed Mar 1, 2026
View reviewed changes
Copy link
Copy Markdown

@Aravind-blip Aravind-blip left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR

@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Mar 3, 2026

@Aravind-blip, commenting on PRs that were merged two years ago is unhelpful. Moreover, undisclosed AI use is frustrating and harms trust. Please do not do this again, or you will be banned from this repo.

@Aravind-blip
Copy link
Copy Markdown

Aravind-blip commented Mar 3, 2026 •
edited
Loading

@alice-i-cecile Thanks for pointing that out. I was completing an external review exercise and didn’t realize commenting on an old merged PR would cause disruption. That wasn’t my intention. I won’t do that again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+4 more reviewers

@Aravind-blip Aravind-blip Aravind-blip left review comments

@ickshonpe ickshonpe ickshonpe approved these changes

@UkoeHB UkoeHB UkoeHB approved these changes

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Text Rendering and layout for characters D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon

Projects

None yet

Milestone

0.15

Development

Successfully merging this pull request may close these issues.

7 participants

@MiniaczQ @alice-i-cecile @Aravind-blip @ickshonpe @UkoeHB @BenjaminBrienen @JMS55