#oval(left, top, radius) should be #oval(left, top, diameter) #118

Closed
wasnotrice opened this Issue Sep 3, 2012 · 6 comments

4 participants

@wasnotrice
Shoes member

Shoes 3 lets you make a circle or a square like this:

Shoes.app width: 350, height: 150 do
  rect 10, 10, 50
  oval 60, 10, 50
end

The shapes are the same size. Let's make that happen in Shoes 4, too, by changing the third arg to #oval so that it is the diameter of the circle, not the radius.

@ashbb
Shoes member

+1 #oval(left, top, diameter)

@wasnotrice
Shoes member

Ok, I made this change in c4b681b.

I also changed the named argument in the style hash from :radius to :diameter. This is an API change from Shoes 3. To create a circle using the styles hash,

in Shoes 3:

Shoes.app height: 300, width: 300 do
  oval left: 10, top: 10, radius: 100
end

in Shoes 4

Shoes.app height: 300, width: 300 do
  oval left: 10, top:10, diameter: 200
end

Shoes 3 ignores :diameter; Shoes 4 ignores :radius. Is this API change from Shoes 3 a good one?

@davorb
Shoes member

I'm not sure that the code that interprets the arguments belongs in element_methods.rb (where it is now). If we move it somewhere else, then there's no reason why we can't support both.

What do you guys think?

@ashbb
Shoes member

@davorb

Yes, we can support both :radius and :diameter in the style hash.
But we have to make a choice as the third arg.

We decided that the third arg value means diameter not radius.
So, IMHO, it's reasonable to change from :radius to :diameter in the style hash, too.

@wasnotrice
Shoes member

@davorb I moved the code that interprets the arguments into ElementMethods because the ambiguity (overloaded method signatures) is for user convenience, but unnecessary to the internal code. This way, we deal with the ambiguity (and document it) in one place—at the edge of the system. I did have this code in Oval#initialize but then we had two methods with these complicated signatures, and two places to document them. I think this way is cleaner.

We can definitely support :radius and :diameter in the style hash. I guess there is a benefit of backward compatibility (although it's only in the style hash, not in the other method signatures). And it could be convenient. But on the other hand, what is the point of offering both, when the user just has to multiply the radius by 2 to get the diameter? I do not have a strong opinion on this question.

@wasnotrice
Shoes member

Closing as fixed. If there's more desire to add :radius to the style hash, please open a new issue.

@wasnotrice wasnotrice closed this Sep 7, 2012
@PragTob PragTob added this to the 4.0.0.pre2 milestone Jun 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment