Skip to content

CosmicBuffer is a public type but not not used or accessible in any public API#17748

Merged
alice-i-cecile merged 6 commits intobevyengine:mainfrom
fschlee:main
Feb 10, 2025
Merged

CosmicBuffer is a public type but not not used or accessible in any public API#17748
alice-i-cecile merged 6 commits intobevyengine:mainfrom
fschlee:main

Conversation

@fschlee
Copy link
Copy Markdown
Contributor

@fschlee fschlee commented Feb 9, 2025

Objective

Currently CosmicBuffer is a public type with a public field that is not used or accessible in any public API. Since it is prominently shown in the docs it is the obvious place to start when trying to access cosmic_string features such as for mapping between screen coordinates and positions in the displayed text.
The only place CosmicBuffer is currently used is as a field of ComputedTextBlock, where a comment explains why the field is private:

/// Buffer for managing text layout and creating [`TextLayoutInfo`].
///
/// This is private because buffer contents are always refreshed from ECS state when writing glyphs to
/// `TextLayoutInfo`. If you want to control the buffer contents manually or use the `cosmic-text`
/// editor, then you need to not use `TextLayout` and instead manually implement the conversion to
/// `TextLayoutInfo`.
#[reflect(ignore)]
pub(crate) buffer: CosmicBuffer,

Unfortunately this comment does not appear in the docs, so a user looking for a way to access CosmicBuffer will not find it unless they check the source code.
Also there does not seem to be any alternative way to map between screen coordinates and positions in the displayed text, which would be highly useful for things like text edit widgets or tool tips. The reasons given for making the field private only apply for mutable access, so non-mutable access would presumably be fine.

Solution

I added a getter to ComputedTextBlock, and added the explanation for why there is no mutable access in the comment:

/// Accesses the underling buffer which can be used for `cosmic-text` APIs such as accessing layout information
/// or calculating a cursor position.
///
/// Mutable access not offered because changes would be overwritten during the automated layout calculation.
/// If you want to control the buffer contents manually or use the `cosmic-text`
/// editor, then you need to not use `TextLayout` and instead manually implement the conversion to
/// `TextLayoutInfo`.
pub fn get_buffer(&self) -> &CosmicBuffer {
	&self.buffer
}

Testing

I tested that the getter could be used to map from screen coordinates to string positions by creating a rudimentary text edit widget and trying it out.

Alternatives

An alternative to making CosmicBuffer accessible would be to make the type private so that no one wastes time looking for a way of accessing it, and adding additional methods to ComputedTextBlock that make use of the buffer as implementation detail and offer access to currently inaccessible functionality.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 9, 2025

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@UkoeHB
Copy link
Copy Markdown
Contributor

UkoeHB commented Feb 9, 2025

Thanks, the publicity of CosmicBuffer is leftover from a previous design iteration. It is fine to expose immutable access as a solution.

fschlee and others added 2 commits February 9, 2025 16:31
Typo fix

Co-authored-by: Rob Parrett <robparrett@gmail.com>
Grammar fix

Co-authored-by: Rob Parrett <robparrett@gmail.com>
Co-authored-by: Rob Parrett <robparrett@gmail.com>
Copy link
Copy Markdown
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

This was useful to me when I tested a previous iteration of the cosmic integration where this was available.

@rparrett rparrett added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it A-Text Rendering and layout for characters D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Feb 9, 2025
@rparrett rparrett mentioned this pull request Feb 9, 2025
@bytemunch bytemunch mentioned this pull request Feb 10, 2025
@alice-i-cecile alice-i-cecile added the C-Feature A new feature, making something new possible label Feb 10, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 10, 2025
Merged via the queue into bevyengine:main with commit db03565 Feb 10, 2025
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 C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

4 participants