Skip to content

Moved UI widgets to a bevy_widget crate#7197

Closed
Pietrek14 wants to merge 52 commits intobevyengine:mainfrom
Pietrek14:widget-plugin
Closed

Moved UI widgets to a bevy_widget crate#7197
Pietrek14 wants to merge 52 commits intobevyengine:mainfrom
Pietrek14:widget-plugin

Conversation

@Pietrek14
Copy link
Contributor

Objective

Fixes #7190.

Solution

  • Added a new bevy_widget crate to Bevy
  • Moved the widgets to the new crate
  • Created a seperate category for widget-related examples

Changelog

  • Added
    • A bevy_widget crate
    • WidgetPlugin
  • Moved
    • Widgets to bevy_widget crate
    • Widget-related examples to a dedicated directory

Migration Guide

  • Widget imports now have to reference the widget module instead of ui
  • If you don't use the default features and want to have widgets available, you will have to enable the bevy_widget feature flag
  • If you don't use the default plugins and want to have widgets available, you will have to add a WidgetPlugin to your app

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets X-Needs-SME This type of work requires an SME to approve it. C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 14, 2023
@@ -0,0 +1,12 @@
mod widget;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add forbid(unsafe_code) and warn(missing_docs) crate level rules please?

Once that's in, we should add basic doc strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing with missing docs is QueuedText. It's only used as a Local for text_system, so should we even keep it public?

Copy link
Member

Choose a reason for hiding this comment

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

We should probably keep text_system public, so we need to keep the Local type public or Rust will complain.

@alice-i-cecile
Copy link
Member

Marked as Controversial since this adds a new crate, but it seems like there's pretty clear consensus on splitting apart the code like this.

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.

Feedback on structure:

  1. I think that the widget bundles belong in the same file as the logic for that widget.
  2. I don't think that the widget module is useful since we're in a dedicated crate for widgets. We can just make each widget a top level module.

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.

LGTM now :) I trust that you'll get basic docs for QueuedText working.

@Pietrek14 Pietrek14 mentioned this pull request Jan 14, 2023
@github-actions
Copy link
Contributor

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

1 similar comment
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

1 similar comment
@github-actions
Copy link
Contributor

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

@Pietrek14
Copy link
Contributor Author

I don't mean to rush anything, but how long may it take for this PR to reach Cart's attention? There are merge conflicts to resolve every time any change is made to the bevy_ui crate, which is pretty often. I'm going to try to keep this up-to-date as much as I can, but I'm travelling to Portugal in the second half of April and I won't be able to take care of it. I'm not planning on abandoning this PR, but I just want to know if it will be reviewed by then.

@mockersf
Copy link
Member

Disclaimer: I'm not Cart

I think it's too early to do this, and I haven't seen either in this PR or in the issue it fixes a good enough justification for the change. This is just a code organisation changes that doesn't improve anything or unlock anything, as such it's very low priority

@github-actions
Copy link
Contributor

You added a new example but didn't update the readme. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@github-actions
Copy link
Contributor

You added a new example but didn't update the readme. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@alice-i-cecile alice-i-cecile added S-Waiting-on-SME This is currently waiting for an SME to resolve something controversial and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 22, 2024
@bas-ie
Copy link
Contributor

bas-ie commented Nov 23, 2024

Backlog cleanup: closing due to inactivity. We've already split out a number of crates, so I guess there's nothing to stop us doing this later but probably easier to begin again after 0.15.

@bas-ie bas-ie closed this Nov 23, 2024
@alice-i-cecile
Copy link
Member

I still think we should do something like this, but yeah, this requires consensus and broad maintainer buy-in :(

@bas-ie
Copy link
Contributor

bas-ie commented Nov 24, 2024

I hovered over the button, but figured someone would yell if they really wanted it kept!

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

Labels

A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Waiting-on-SME This is currently waiting for an SME to resolve something controversial X-Needs-SME This type of work requires an SME to approve it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move ButtonBundle, ImageBundle, TextBundle and their behaviour into a WidgetPlugin

7 participants