-
Notifications
You must be signed in to change notification settings - Fork 567
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
Standardize Text type in Contexts #996
Conversation
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 don't get why the lifetime is no longer needed but it looks like a nice change.
CHANGELOG.md
Outdated
@@ -73,6 +73,8 @@ This means that druid no longer requires cairo on macOS and uses Core Graphics i | |||
- Replaced `Command::one_shot` and `::take_object` with a `SingleUse` payload wrapper type. ([#959] by [@finnerale]) | |||
- Renamed `WidgetPod` methods: `paint` to `paint_raw`, `paint_with_offset` to `paint`, `paint_with_offset_always` to `paint_always`. ([#980] by [@totsteps]) | |||
- `Command` and `Selector` have been reworked and are now statically typed, similarly to `Env` and `Key`. ([#993] by [@finnerale]) | |||
- Standardize the type returned by the contexts' `text()` methods. (#[996] by |
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 PR link is missing.
A long standing pet-peeve of mine: Previously there were a number of different lifetypes associated with the text factory depending on where it came from. This standardizes that, so that all contexts' text() method returns an owned PietText handle. This has the added advantage of getting rid of the weird lifetime in LayoutContext, which will also unblock my macro work.
as far as I understand the reason we needed the lifetime previously is because we held onto a reference to the type, and so we had to be explicit about its inner lifetime; but now that we just hand one out (not a reference) we can elide it? Still feels weird though |
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.
It's odd but I like it a lot.
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.
Looks good!
A long standing pet-peeve of mine: Previously there were a
number of different lifetypes associated with the text factory
depending on where it came from.
This standardizes that, so that all contexts' text() method returns
an owned PietText handle.
This has the added advantage of getting rid of the weird lifetime
in LayoutContext, which will also unblock my macro work.
This is a breaking change in a fairly minor way;
LayoutCtx
returnsText
now instead of&mut Text
.