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

Implementing Display formatter for Rect type. #73

Merged
merged 3 commits into from Mar 16, 2015

Conversation

@Adenilson
Copy link
Contributor

Adenilson commented Mar 16, 2015

We need this for dumping DisplayList geometric information on servo.

We need this for dumping DisplayList geometric information on servo.
src/rect.rs Outdated
fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
// Why this won't compile? rustc will say that neither Point2D or
// Size2D has Debug implemented!
//write!(f, "Rect(Size {:?} at Origin {:?})", self.size, self.origin)

This comment has been minimized.

@pcwalton

pcwalton Mar 16, 2015

Contributor

Can you try Size {} at Origin {}? (or at least that with self.size.to_string() and likewise with origin)

This will allow to have cleaner code in Rect.
@Adenilson
Copy link
Contributor Author

Adenilson commented Mar 16, 2015

Implemented the Display operator (previously String op) in both Point and Size to make it work on Rect.

@brendanzab
Copy link
Member

brendanzab commented Mar 16, 2015

Why do you need it? Wouldn't you be able to use Debug?

@Adenilson
Copy link
Contributor Author

Adenilson commented Mar 16, 2015

bjz check servo/servo#5209

src/rect.rs Outdated
@@ -29,6 +29,13 @@ impl<T: fmt::Debug> fmt::Debug for Rect<T> {
}
}

impl<T: fmt::Display> fmt::Display for Rect<T> {
fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
write!(formatter, "Rect(Size {} at Origin {})", self.size.to_string(), self.origin.to_string());

This comment has been minimized.

@pcwalton

pcwalton Mar 16, 2015

Contributor

Is there a reason why this follows a different format from Debug? Rect({} at {}) seems terser.

src/size.rs Outdated
@@ -26,6 +26,13 @@ impl<T: fmt::Debug> fmt::Debug for Size2D<T> {
}
}

impl<T: fmt::Display> fmt::Display for Size2D<T> {
fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
write!(formatter, "({},{})", self.width, self.height);

This comment has been minimized.

@pcwalton

pcwalton Mar 16, 2015

Contributor

Ditto; using the multiplication sign seems clearer.

@Adenilson
Copy link
Contributor Author

Adenilson commented Mar 16, 2015

Should be ok now.

pcwalton added a commit that referenced this pull request Mar 16, 2015
Implementing Display formatter for Rect type.
@pcwalton pcwalton merged commit 1385a0c into servo:master Mar 16, 2015
@brendanzab
Copy link
Member

brendanzab commented Mar 17, 2015

I'm not very invested in this project, but I might suggest reverting this, because it is not needed by servo/servo#5209

@Adenilson
Copy link
Contributor Author

Adenilson commented Mar 17, 2015

@bjz would elaborate more on it?

We have to print the Rect's in BaseDisplayItem (i.e. bounds). The first patch version would directly access Rect fields but that turned out to be quite verbose.

That lead to implementing the operators for Rect and its types.

@brendanzab
Copy link
Member

brendanzab commented Mar 17, 2015

{} uses fmt::Display, {:?} uses fmt::Debug which is already implemented.

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.