Use ArcStr in LocalizedString and LabelText #1245
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmyr
force-pushed
the
arcstr-in-labeltext
branch
from
7530005 to
e101e2b
Compare
cmyr
force-pushed
the
arcstr-in-labeltext
branch
2 times, most recently
from
5291c12 to
b6e7216
Compare
luleyleo
approved these changes
Sep 25, 2020
druid/src/widget/label.rs
Outdated
Comment on lines
40
to
49
| Static { | ||
| /// The text. | ||
| string: ArcStr, | ||
| /// Whether or not the `resolved` method has been called yet. | ||
| /// | ||
| /// We want to return `true` from that method when it is first called, | ||
| /// so that callers will know to retrieve the text. This matches | ||
| /// the behaviour of the other variants. | ||
| resolved: bool, | ||
| }, |
There was a problem hiding this comment.
Maybe move this into its own Static struct the way we did with Dynamic? Then we could hide the resolved field.
druid/src/widget/label.rs
Outdated
Comment on lines
56
to
47
| #[doc(hidden)] | ||
| pub struct Dynamic<T> { |
There was a problem hiding this comment.
Not really related to this PR, but I think it is a bit irritating that Dynamic is doc(hidden). It makes the LabelText documentation slightly weird, because Dynamic looks like a broken link.
druid/src/widget/label.rs
Outdated
| @@ -310,8 +316,8 @@ impl<T: Data> Label<T> { | |||
| impl<T> Dynamic<T> { | |||
| fn resolve(&mut self, data: &T, env: &Env) -> bool { | |||
| let new = (self.f)(data, env); | |||
| let changed = new != self.resolved; | |||
| self.resolved = new; | |||
| let changed = new.as_str() != &*self.resolved; | |||
There was a problem hiding this comment.
I usually prefer as_ref over &*
Suggested change
| let changed = new.as_str() != &*self.resolved; | |
| let changed = new.as_str() != self.resolved.as_ref(); |
There was a problem hiding this comment.
ah totally if AsRef is available, I thought this was only available via deref. :)
CHANGELOG.md
Outdated
Comment on lines
60
to
63
| - Contexts' `text()` methods return `&mut PietText` instead of cloning ([#1205] by [@cmyr]) | ||
| - `LocalizedString` and `LabelText` use `ArcStr` instead of String ([#1245] by [@cmyr]) |
There was a problem hiding this comment.
This diff looks weird… Did you delete the #1205 entry intentionally?
This lets us share our text between druid and the piet layout object, and starts the process of moving us away from using String where possible. This was motivated by explorations around making Label optionally work as a `Widget<ArcStr>`, and using lens-like adapters to generate that text from some other data types as needed; this would be a small improvement of the label API.
cmyr
force-pushed
the
arcstr-in-labeltext
branch
from
b6e7216 to
067323c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This lets us share our text between druid and the piet layout
object, and starts the process of moving us away from using String
where possible.
This was motivated by explorations around making Label optionally work
as a
Widget<ArcStr>, and using lens-like adapters to generatethat text from some other data types as needed; this would be a small
improvement of the label API.