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

Feature-gate all references to bevy_text in bevy_ui #11391

Merged
merged 11 commits into from
Jan 28, 2024

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Jan 17, 2024

Objective

Solution

  • Add #[cfg(feature = "bevy_text")] to all items that require it.

I think this change is honestly a bit ugly, but I can't see any other way around it. I considered making bevy_text required, but we agreed on Discord that there were some use cases for bevy_ui without bevy_text. If you have any ideas that decreases the amount of #[cfg(...)]s and #[allow(...)]s, that would be greatly appreciated.

This was tested by running the following commands:

$ cargo clippy -p bevy_ui
$ cargo clippy -p bevy_ui -F bevy_text
$ cargo run -p ci

Changelog

  • Fixed bevy_ui not compiling without bevy_text.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets A-Text Rendering and layout for characters labels Jan 17, 2024
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

system extract_uinodes should not be behind the bevy_text feature, it can work without changes without bevy_text

@BD103
Copy link
Member Author

BD103 commented Jan 18, 2024

system extract_uinodes should not be behind the bevy_text feature, it can work without changes without bevy_text

I believe I fixed this in b236ab5.

@BD103 BD103 requested a review from mockersf January 18, 2024 14:16
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This is... less bad. Not the prettiest but it'll do.

@BD103
Copy link
Member Author

BD103 commented Jan 18, 2024

I'm considering moving all of the #[cfg(feature = "bevy_text")] statements to a separate function, that way we only have to use the cfg attribute once.

#[cfg(feature = "bevy_text")]
fn build_text_interop(app: &mut App) {
    app.add_systems(...);
    // etc.
}

crates/bevy_ui/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jdm jdm left a comment

Choose a reason for hiding this comment

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

This looks like a reasonable way to contain the current dependencies.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 28, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 28, 2024
Merged via the queue into bevyengine:main with commit 069a877 Jan 28, 2024
23 checks passed
@BD103 BD103 deleted the ui-text-flag branch January 28, 2024 17:50
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
…1391)

# Objective

- `bevy_ui` fails to compile without `bevy_text` being enabled.
- Fixes bevyengine#11363.

## Solution

- Add `#[cfg(feature = "bevy_text")]` to all items that require it.

I think this change is honestly a bit ugly, but I can't see any other
way around it. I considered making `bevy_text` required, but we agreed
[on
Discord](https://discord.com/channels/691052431525675048/743663673393938453/1196868117486379148)
that there were some use cases for `bevy_ui` without `bevy_text`. If you
have any ideas that decreases the amount of `#[cfg(...)]`s and
`#[allow(...)]`s, that would be greatly appreciated.

This was tested by running the following commands:

```shell
$ cargo clippy -p bevy_ui
$ cargo clippy -p bevy_ui -F bevy_text
$ cargo run -p ci
```

---

## Changelog

- Fixed `bevy_ui` not compiling without `bevy_text`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Text Rendering and layout for characters A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bevy_ui does not compile without bevy_text feature
4 participants