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.

Sign up for GitHub

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

Jump to bottom

[bevy_ui/layout] Make fields private in UiSurface #13359

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[bevy_ui/layout] Make fields private in UiSurface #13359

wants to merge 4 commits into from
+1,000 −202

Conversation

StrikeForceZero
Copy link
Contributor

Blocked on:

Objective

  • Restrict read/write access to UiSurface fields to encourage using or writing functions that maintain the internal state exclusively inside UiSurface

Solution

Since the fields were originally pub(super) and not used outside of testing, replacing direct access with pub(super) get accessors for #[cfg(test)] seemed like the path of least resistance.

Testing

  • Did you test these changes? If so, how?
    • Tests passing
  • Are there any parts that need more testing?
    • Unlikely due to the scope
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
    • Evaluating the tests in layout/mod and how they've been updated to use the accessors.
    • Determining a better name or method for the ui_layout_tree_debug_string(). Or if the function is an appropriate level of responsibility for the UiSurface struct in the first place. A trait that you have to import might be interesting.
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?
    • Linux

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

  • Made UiSurface fields private and replaced direct pub(super) access with pub(super) get accessors for layout/mod.
  • Added associated function to get the UI layout tree debug string (originally being directly printed in layout/debug)

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use labels May 13, 2024
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change and removed C-Usability A targeted quality-of-life change that makes Bevy easier to use labels May 13, 2024
StrikeForceZero
StrikeForceZero commented May 13, 2024
View reviewed changes
crates/bevy_ui/src/layout/ui_surface.rs
@@ -270,4 +424,553 @@ with UI components as a child of an entity without UI components, results may be
Err(LayoutError::InvalidHierarchy)
}
}

/// Returns a debug representation of the computed layout of the UI layout tree for each camera.
pub fn ui_layout_tree_debug_string(&self) -> Result<String, fmt::Error> {
Copy link
Contributor Author

@StrikeForceZero StrikeForceZero May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This feels weird, as noted in the PR description. Open to alternatives. Maybe the pub(super) get accessors to become available outside of #[cfg(test)], and then layout/debug defines a trait that the user can manually import?

StrikeForceZero
StrikeForceZero commented May 13, 2024
View reviewed changes
crates/bevy_ui/src/layout/debug.rs
#[deprecated(
since = "0.13.3",
note = "use `ui_surface.ui_layout_tree_debug_string()` instead"
)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see: #13359 (comment)

If we leave ui_layout_tree_debug_string in UiSurface then this function should retain the deprecation notice so it can be removed later.

  • Either Remove the notice or update the version to match whatever version this is slated to land in.

@StrikeForceZero StrikeForceZero mentioned this pull request May 13, 2024
Draft
3 tasks
@StrikeForceZero
bevyengine#12803 Changes (squashed)
b699e81 
Apply rustfmt
Move test_initialization next to other tests
Add doc comment to is_root_node_pair_valid
Move helper methods into the single test case where they are used
Add tests for helper methods
Remove trait helpers to reduce complexity of tests
Mark specific test functions as unreachable
Move ui_surface test only methods into mod tests as trait
Fix tests after rebase
Add tests for bevy_ui/layout/ui_surface
Widen type for parameter children in UiSurface::update_children
Add missing asserts and Debug fields in UiSurface from bevyengine#12268 and bevyengine#12698
@StrikeForceZero
bevyengine#12804 changed Update internal mappings (squashed)
90afc38 
Move assertion above panic triggering line
Remove demotion logic to simplify scope
Remove potentially duplicated context update loop
Remove duplicated ContentSize removal handler
Restore assertion in ui tracking tests
Remove promotion test
Satisfy linter
Update test
Remove promotion helper
Apply rustfmt
Fix documentation
Add missing back ticks in doc comment
Underscore unused variables
Rebase fixes
Add regression test for recursive despawn on ui nodes
Apply rustfmt in bevy_ui/layout/debug
Remove message from unreachable!
Add/update documentation for UiSurface
Make missing root_node_data in compute_camera_layout an error
Fix promote_ui_node, add a test, and set to only be in cfg(test)
Update documentation
Use replace_camera_association to reduce redundant code
Use mark_root_node_as_orphaned
Add explicit taffy node counts to tests
Add test for promoting normal ui nodes into root nodes
Add support demoting root nodes into normal ui nodes
Expand tests to cover different methods of despawn
Remove user_root_node field in favor of using entity_to_taffy
Replace is_root_node_data_valid helper with has_valid_root_node_data
Implement updated map structure in UiSurface
Deprecate user_root_node in RootNodeData
Extract default viewport style into inline fn
Rename camera_id to camera_entity
@StrikeForceZero
Make UiSurface fields private
73459f5
@StrikeForceZero
Move debug print into UiSurface to access private fields
8b3f47c
@StrikeForceZero StrikeForceZero force-pushed the dev/bevy_ui/ui_surface_make_fields_private branch from 368e1bd to 8b3f47c Compare May 19, 2024 17:58
@janhohenheim janhohenheim added the S-Blocked This cannot move forward until something else changes label Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicoburns nicoburns Awaiting requested review from nicoburns

Assignees
No one assigned
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change S-Blocked This cannot move forward until something else changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@StrikeForceZero @alice-i-cecile @janhohenheim