-
Notifications
You must be signed in to change notification settings - Fork 119
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
Stop storing text in TextLayout #666
Conversation
Remove type parameter from TextLayout. Tweak error message in text widgets. This makes the TextLayout code slightly simpler (though with more invariants) in anticipation of splitting it into its own crate.
This is another chunk of work split off from #625. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't go into detail yet ...
@@ -37,8 +37,7 @@ use crate::text::render_text; | |||
/// | |||
/// TODO: Update docs to mentionParley | |||
#[derive(Clone)] | |||
pub struct TextLayout<T> { | |||
text: T, | |||
pub struct TextLayout { | |||
// TODO: Find a way to let this use borrowed data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment still accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean the "use borrowed data" comment, yes, it's still accurate.
It's from this commit: 732cfa8#diff-da6cde61a550125d612ef93c9aecd16c1210e2cfbfb664d3ba0a9ab00ddc2caf
Not sure what it means either. @DJMcNab ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea. It seems you understand this comment better than me - because I assume that it applied to the text
field and was misplaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll remove it then.
I'm curious if this has any bearing on changing from just displaying text of a single run of size + color to being able to do richer text via a builder or something. |
I don't think it does. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The percent label in progress_bar is not updating in the widgets example.
Other than that regression, I don't see any other issues.
Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested. It works now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the motivation for this change a little bit confusing, as the same bookkeeping is still happening, just in more places.
But removing the generic is probably good.
@@ -37,8 +37,7 @@ use crate::text::render_text; | |||
/// | |||
/// TODO: Update docs to mentionParley | |||
#[derive(Clone)] | |||
pub struct TextLayout<T> { | |||
text: T, | |||
pub struct TextLayout { | |||
// TODO: Find a way to let this use borrowed data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea. It seems you understand this comment better than me - because I assume that it applied to the text
field and was misplaced.
See discussion in #666
Remove type parameter from TextLayout.
Tweak error message in text widgets.
This makes the TextLayout code slightly simpler (though with more invariants) in anticipation of splitting it into its own crate.