- Notifications
You must be signed in to change notification settings - Fork 56
Remove kurbo and peniko dependencies #414
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
Conversation
34c41f7 to 6be402c Compare 4ef0637 to dfa357f Compare
DJMcNab left a comment
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.
This is a good way to achieve the goal, and I agree with the goal. I've got some thoughts about making sure that we make clear to our users and tooling what the purpose of this new type is.
parley/src/util.rs Outdated
| | ||
| /// A rectangle. | ||
| #[derive(Clone, Copy, Default, PartialEq)] | ||
| pub struct Rect { |
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.
What do you think about making this non_exhaustive (and removing pub fn new/Default), to gently discourage more general use. Obviously if people can get a single Rect, since it's Copy, they can easily make new ones, but it should be sufficient roadblocks to prevent people doing that.
I'd also consider renaming to something like OutputRect or similar. In particular, I never want Rust Analyzer to suggest this type when I am looking to import Rect.
Basically strongly saying to applications that they should standardise on a rectangle type, and it shouldn't be this one.
That would make this PR much easier for me to approve, as it makes this API much clearer.
But I'd also suggest that we can talk about this in office hours.
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 guess so. I don't really care if anybody uses it, but I do agree that it would be good to remove it from autocomplete for Rect. How about a rename to BoundingBox (with an eye on using BoundingBox from the font-types crate once font-types hits 1.0.
Or we could even be really specific with CursorArea.
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 rename is the more important of the two, I agree; BoundingBox is a pretty good name here.
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.
Renamed to BoundingBox
| This turns out to have a nice compile time benefit for
|
f76b2cf to 0285ce9 Compare Signed-off-by: Nico Burns <nico@nicoburns.com>
0285ce9 to 47b6486 Compare
DJMcNab left a comment
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.
A few nits. Approval is conditional on them being applied
I would probably like to see a slightly expanded doc on bounding box (e.g. mentioning y-down space, talking about whether the values are logical pixels, etc.). But I'm not going to block on that.
I also think that this change does deserve a changelog entry, but again I'm not going to block on that.
Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
We should probably wait until we've backported
linebender_resource_handletopeniko0.4 to land this. Then the font/blob conversion code can disappear.We may want to name
parley::Rectsomething more specific likeparley::SelectionAreaDepends on:
clippy::obfuscated_if_elselints #415