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

Remove some method names that assume y pointing down #363

Closed
wants to merge 2 commits into from

Conversation

@nical
Copy link
Collaborator

nical commented Jul 11, 2019

With this PR, the only things in euclid that dictate the direction of the y axis are:

  • Rect::inner_rect / outer_rect
  • Box2D::inner_box / outer_box

This change is Reviewable

@nical
Copy link
Collaborator Author

nical commented Jul 11, 2019

This is an incomplete alternative to #361 and #353.

I don't have good ideas for SideOffsets2D member names that are agnostic to the y axis orientation without being awful, but since I'm unlikely to get consensus on either of #361 and #353, I think that I might propose this one and call it a day.

r? @kvark and/or @Manishearth

@kvark
kvark approved these changes Jul 11, 2019
Copy link
Member

kvark left a comment

Can we have SideOffsets just storing things like x0, x1, y0, y1?

@Manishearth
Copy link
Member

Manishearth commented Jul 11, 2019

r? @pcwalton @nox

I don't use these in my code, but I know servo layout does

@nical
Copy link
Collaborator Author

nical commented Jul 11, 2019

The second commit renames SideOffsets2D's top/right/bottom/left into y0/x1/y1x0.

Caveat

One would think that it would make more sense for the structure and parameters to be laid out in the order "x0,y0, x1, y1" or "x0, x1, y0 y1", except that doing this means the order of the parameters SideOffsets2D::new:

  • doesn't match the CSS syntax anymore,
  • becomes wrong in all current uses of the method (and I think that there lies madness).

So far, @kvark, @Manishearth and @SimonSapin have expressed various levels of agreement about doing something like this in principle (ranging from, "OK" to "meh, OK". Personally I am not a huge fan but it's good enough for me and makes euclid agnostic to the orientation of the y-axis which is good.

I think that this can be a controversial change so don't hesitate to cc people who use euclid and side offsets a lot.

@SimonSapin
Copy link
Member

SimonSapin commented Jul 12, 2019

How about removing SideOffsets2D::new? Four parameters with the same type are easy to get in the wrong order.

@nical
Copy link
Collaborator Author

nical commented Jul 12, 2019

How about removing SideOffsets2D::new?

This would be nice except for the extra PhantomData field which would have to be passed every time.

bors-servo added a commit that referenced this pull request Jul 15, 2019
Remove some Rect method names that assume y pointing down

The first (uncontroversial) part of #363. This removes `Rect::to_right` and similar methods but leaves SideOffsets untouched.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/364)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 4, 2019

The latest upstream changes (presumably #368) made this pull request unmergeable. Please resolve the merge conflicts.

@nical nical closed this Dec 6, 2019
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

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