Remove OriginDimensions trait #737
Conversation
rfuest
force-pushed
the
remove-origin-dimensions
branch
from
f3bf502 to
ef48aa0
Compare
|
CI fails at the moment, because this change brakes |
|
It is most likely outside the scope of this PR, but it may be worth investigating adding something akin to embedded-hal-compat to ease interoperability between crates while also enabling e-g to introduce breaking API changes |
|
I hope that in the future we don't break e-g-core with every major e-g release, but until now we haven't been successful in maintaining compatibility. Providing a crate similar to e-h-compat would be useful, but it's outside the scope of this PR and we should discuss this in a new issue. |
core/src/draw_target/mod.rs
Outdated
| /// Size::new(64, 64) | ||
| /// impl Dimensions for ExampleDisplay { | ||
| /// fn bounding_box(&self) -> Rectangle { | ||
| /// Rectangle::new(Point::zero(), Size::new(64, 64)) |
There was a problem hiding this comment.
It may be worth adding a new constructor method to Rectangle enabling individuals to write something along the lines of:
Rectange::at_origin(Size::new(64, 64));There was a problem hiding this comment.
I agree, but I wanted to keep this PR focused on one issue and open another PR for that. This has been discussed before in #706 (comment) and #448.
Do you prefer one of the methods that has been suggested before, either a new constructor in Rectangle or a Size::to_rectangle method? to_rectangle looks a bit nicer in this use case: #448 (comment), but there might be other cases where a Rectangle constructor has benefits.
There was a problem hiding this comment.
I personally prefer Rectangle::at_origin() over Size::to_rectangle() as I feel at_origin() is slightly more explanatory from the perspective of someone that may be reading code in a downstream crate.
I think implementing From/Into, however, may be the right answer here.
I agree, but I wanted to keep this PR focused on one issue and open another PR for that.
I do agree; this feels like something we could rat hole on for awhile so it may be best to move this to a dedicated issue. I could see arguments for adding various From/Into implementations to improve ergonomics and it may be best to contain all that conversation in a more specific issue :)
There was a problem hiding this comment.
I've created #738 to add the constructor to Rectangle.
I think implementing
From/Into, however, may be the right answer here.
We decided against that in the past for the same reason you mentioned for to_rectangle. let rect: Rectangle = size.into(); wouldn't be as easy to understand as let rect = Rectangle::new_at_origin(size);
At the moment I would suggest we stick to the constructor and if it turns out that there is a need to allow something like fn some_fn<T: Into<Rectangle>>(rect_or_size: T) in the future we can still add the From/Into impl.
rfuest
force-pushed
the
remove-origin-dimensions
branch
from
ef48aa0 to
00fdb90
Compare
2 participants
This PR removes the
OriginDimensionstrait as previously discussed in #736 and #706.