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

Extention traits for axis-specific method names #317

Closed
nical opened this issue Jan 14, 2019 · 5 comments
Closed

Extention traits for axis-specific method names #317

nical opened this issue Jan 14, 2019 · 5 comments
Labels

Comments

@nical
Copy link
Collaborator

@nical nical commented Jan 14, 2019

For a while I've received negative feedback about euclid fixing some axis directions such as "positive y points downward" through the name of a few methods like Rect::top_right.

In Gecko/Servo it's convenient to have these names as they correspond to our layout conventions, though.
As a compromise we could put these methods on "extension traits".

For example:

pub trait YDownRect {
    type Point;
    type Vector;

    fn top_right(&self) -> Point;
    // etc.
}

impl<T, U> YDownRect for TypedRect<T, U> {
    // ...
}

It would have the benefit of letting people opt into specific naming conventions on a per-file basis by simply writing something like use euclid::convention::YDownRect; at the top of the file. We could even implement several conventions if there is a demand for it.
Without any opt-in, euclid would remain agnostic to the axis direction conventions and be useful to more people.

I haven't thought about it very much so I am not even sure how much I like the idea myself (I confess I never liked the pattern of extension traits in the first place).

The main advantage of this over, say a feature flag is that each file can decide what convention they use by importing the trait rather than enforcing a convention on all the crates in the project.
It would also let us be less conservative about names (I try to minimize these names that enforce y-down convention).
The main downside in my opinion is the burden of importing the trait(s) in servo/gecko in the places where we want to use these names.

Thoughts?

@kvark
Copy link
Member

@kvark kvark commented Feb 1, 2019

I'd prefer it to be agnostic. The users can also include the direction information into the space generic:

enum YDown {}
enum YUp {}
enum DevicePixel {}
type MyRect = TypedRect<f32, (DevicePixel, YUp)>;
@kvark kvark added the question label Feb 1, 2019
@nical
Copy link
Collaborator Author

@nical nical commented Feb 2, 2019

@kvark your idea of putting this information in the unit/space made me think of an alternative which I find quite appealing:

Add a bunch of dummy traits such as YUp with no methods, and in euclid implement y-up-specific methods only for vectors/points/rects/etc which have a unit that implements YUp, for example:

impl<T, U: YUp> TypedRect<T, U> {
    pub fn top_left() -> TypedPoint2D<T, U> { .. }
    // ...
}

This way users of euclid can decide whether their vector space has y pointing upward where the aliases are declared like this:

pub struct LayoutSpace;
impl YUp for LayoutSpace {}; // opt into having the y-up-specific functions;
pub type LayoutRect = TypedRect<f32, LayoutSpace>;

I would rather avoid a scheme that generates different types for "YUp" and "don't care" LayoutRects.
My original idea was to opt into the functions per file that use the types but your idea of putting this in the vector space makes a lot of sense, and avoids the burden of importing traits which is what annoyed me most about my initial proposal.

@kvark
Copy link
Member

@kvark kvark commented Feb 3, 2019

If the semantics of top_left() and such are the only thing that depends on the Y-direction, can't we address it by simply avoiding calling a spatial direction in the names? I.e. instead of implying by the API that top is less than bottom and left is less than right, just make the references to min/max straight away. There are already min_x() method and such. We could could rename the methods as:

  • top_left() -> min_x_min_y_corner()
  • bottom_left() -> min_x_max_y_corner()
  • top_right() -> max_x_min_y_corner()
  • bottom_right() -> max_x_max_y_corner()

This would mean that any decision on what is Up and Down is fully on the user side. Also related to #306

@nical
Copy link
Collaborator Author

@nical nical commented Feb 4, 2019

I've been trying to minimize this type of names but sometimes they are just the simplest way to think about a certain thing.
Names like top_left have a lot less cognitive overhead than min_x_min_y_corner.
My worry about completely removing these names is that layout code uses them a lot and this kind of name change would IMO be a bit mean to everyone working on servo layout.
I don't know how we'd name everything in SideOffsets2D.

Also in general I'd like to avoid solutions that need major code churns (especially on code that doesn't belong to the gfx team). One of the reasons I like opting into these names through the dummy traits is that the only place where we'd need to change anything for, say, servo layout is the one place where the type aliases are declared.

Also related to #306

Are you referring to adding a TypedBox2D type with min/max representation? I think of it as orthogonal to the naming schemes. The 2d box type would also benefit from having "nice" opt-in names in some places.

@nical
Copy link
Collaborator Author

@nical nical commented Feb 6, 2019

Actually, my dummy trait proposal doesn't "just work" (not in the simple way I had in mind) because we can't implement the same method several times and select depending on trait bounds.

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.