Skip to content
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

Add methods to get points with axes #337

Merged
merged 2 commits into from
May 7, 2020
Merged

Add methods to get points with axes #337

merged 2 commits into from
May 7, 2020

Conversation

jamwaffles
Copy link
Member

@jamwaffles jamwaffles commented May 6, 2020

Hi! Thank you for helping out with Embedded Graphics development! Please:

  • Check that you've added passing tests and documentation
  • Add a simulator example(s) where applicable
  • Add a CHANGELOG.md entry in the Unreleased section under the appropriate heading (Added, Fixed, etc) and appropriate crate (embedded-graphics, simulator, tinytga, tinybmp) if your changes affect the public API
  • Run rustfmt on the project
  • Run ./build.sh (Linux/macOS only) and make sure it passes. If you use Windows, check that CI passes once you've opened the PR.

PR description

Adds x_axis and y_axis methods to get a Point or Size with one dimension set to zero. Some places can already use these, but they would be particularly useful in the RoundedRectangle PR. Also adds Point::fill and Size::fill which will be less useful but there are a couple of places they can be used. If these are really useless I don't mind removing them.

@rfuest
Copy link
Member

rfuest commented May 6, 2020

Adds x_axis and y_axis methods to get a Point or Size with one dimension set to zero. Some places can already use these, but they would be particularly useful in the RoundedRectangle PR.

Good idea!

Also adds Point::fill and Size::fill which will be less useful but there are a couple of places they can be used. If these are really useless I don't mind removing them.

I did temporarily add similar methods before, but removed them because they weren't totally necessary for that particular PR and I couldn't come up with a good name. I think we should keep them, but maybe we can come up with a better name, because I wouldn't associate the name fill with a constructor. Normally additional constructors should be prefixed by with_, but that doesn't work well in this case IMO. Maybe new_square?

@jamwaffles jamwaffles force-pushed the axes branch 2 times, most recently from 58d0d22 to dcf494b Compare May 6, 2020 15:38
@jamwaffles
Copy link
Member Author

The name was inspired by nalgebra's Matrix::fill, so might be familiar to some users. An alternative could be new_equal or new_filled perhaps? I think this would then need a shim adding for the features = [ "nalgebra" ] which is easy enough.

I deliberately didn't do a find/replace with the ::fill() methods as it makes most of the examples and tests where it's used a bit less clear IMO. There are a couple of usages in internal code, but we should probably change those manually going forward.

@jamwaffles jamwaffles marked this pull request as ready for review May 6, 2020 15:47
@jamwaffles jamwaffles requested a review from rfuest May 6, 2020 15:47
jamwaffles added a commit that referenced this pull request May 6, 2020
@rfuest
Copy link
Member

rfuest commented May 6, 2020

The name was inspired by nalgebra's Matrix::fill, so might be familiar to some users. An alternative could be new_equal or new_filled perhaps?

Am I right that Matrix::fill isn't a constructor? If that's the case I would go with new_filled, which still remains similar to the nalgebra method.

I think this would then need a shim adding for the features = [ "nalgebra" ] which is easy enough.

Why would this be necessary?

I deliberately didn't do a find/replace with the ::fill() methods as it makes most of the examples and tests where it's used a bit less clear IMO. There are a couple of usages in internal code, but we should probably change those manually going forward.

SGTM

@jamwaffles
Copy link
Member Author

I've changed fill changed to new_filled. It's been a while since I used nalgebra - looks like I got some things confused. Ready to review now :)

Copy link
Member

@rfuest rfuest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits...

embedded-graphics/src/geometry/point.rs Outdated Show resolved Hide resolved
embedded-graphics/src/geometry/point.rs Outdated Show resolved Hide resolved
embedded-graphics/src/geometry/point.rs Outdated Show resolved Hide resolved
embedded-graphics/src/geometry/point.rs Outdated Show resolved Hide resolved
embedded-graphics/src/geometry/size.rs Outdated Show resolved Hide resolved
embedded-graphics/src/geometry/size.rs Outdated Show resolved Hide resolved
@jamwaffles
Copy link
Member Author

Thanks for the review. Comments addressed

@rfuest
Copy link
Member

rfuest commented May 7, 2020

I did give the name of the constructor another thought and I think we should change it.

IMO the word filled could easily be mistaken for something related to styling, if someone isn't familiar with e-g:

let rect = Rectangle::new(Point::new(10, 20), Size::new_filled(10));
// does this create a "filled rectangle"?

For this case the new_square would work:

let rect = Rectangle::new(Point::new(10, 20), Size::new_square(10));

but Point::new_square might not be the greatest name.

Where can I put my 🚲? 😉

@jamwaffles
Copy link
Member Author

Yes that's true, in context new_filled isn't very clear. That said, new_square doesn't look quite right when used with Ellipse as it makes a circle:

Ellipse::new(Point::new(10, 20), Size::new_square(10));

The best I can come up with is Size::new_equal(u32) or maybe even just Size::equal(u32). If you're not keen on anything equal based, I'll change this PR to new_square, let the docs explain what it does for user who might be confused and leave it there.

Can I share your 🔒 for my 🚲? 😁

@rfuest
Copy link
Member

rfuest commented May 7, 2020

The best I can come up with is Size::new_equal(u32)

OK, let's just go with that.

@jamwaffles
Copy link
Member Author

Changes pushed 👍

embedded-graphics/src/geometry/point.rs Outdated Show resolved Hide resolved
embedded-graphics/src/geometry/size.rs Outdated Show resolved Hide resolved
@jamwaffles
Copy link
Member Author

jamwaffles commented May 7, 2020

Those examples names do look better, thanks

@jamwaffles jamwaffles merged commit a5364a4 into master May 7, 2020
@jamwaffles jamwaffles deleted the axes branch May 7, 2020 15:00
@jamwaffles
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants