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

Expanded TextBuffer interface to allow borrowed values. #444

Merged
merged 3 commits into from
Jun 12, 2021

Conversation

Zenithsiz
Copy link
Contributor

Previously the TextBuffer trait only allowed owned values, since it relied on Clone, Default and 'static.
This allows it to use borrowed values.

A possible use case for this is creating a buffer which has dynamic max length:

struct LimitedStringBuffer<'a> {
    text: &'a mut String,
    max_len: usize,
}

This wasn't possible in the previous api, as Default and Clone cannot be implemented for this struct, and it wasn't 'static.

All new trait methods have a default impl using the existing 2 methods, so this is backwards compatible and also suffers from no performance loss on Strings due to them implementing these new methods.

@emilk
Copy link
Owner

emilk commented Jun 12, 2021

I wanted to wait with this until #412 is in, and now it is.

This seems like a good idea, but there is a conflict with the above PR: we now make a clone of the text each frame (not great, not terrible) to easily detect and report changes for screen readers. This would have to change to instead of cloning into an S to clone into a String (e.g. call text.to_string() instead of text.clone())

@Zenithsiz
Copy link
Contributor Author

It should also be possible to reuse the buffer by storing it somewhere, which would avoid an allocation every frame, and instead just do old_text.clone_from(text.as_ref()).

I haven't implemented that however, as I'm not sure where I could store the old buffer, nor if it would be a good idea.
Have rebased on master though.

@emilk emilk merged commit 7f1123a into emilk:master Jun 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants