-
Notifications
You must be signed in to change notification settings - Fork 194
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
Updating Oval to use new styles #736
Changes from 8 commits
e179525
c9bbc28
be703d2
49873d8
4986d22
ee9e8d6
7c593c9
bc88438
c9cb687
90851e2
d87e3c0
f25b480
9384835
2437857
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,7 +184,7 @@ def sound(soundfile, opts = {}, &blk) | |
# @option opts [Boolean] wedge (false) | ||
# @option opts [Boolean] center (false) is (left, top) the center of the rectangle? | ||
def arc(left, top, width, height, angle1, angle2, opts = {}) | ||
create Shoes::Arc, left, top, width, height, angle1, angle2, opts#style.merge(arc_style) | ||
create Shoes::Arc, left, top, width, height, angle1, angle2, opts | ||
end | ||
|
||
# Draws a line from point A (x1,y1) to point B (x2,y2) | ||
|
@@ -221,7 +221,7 @@ def line(x1, y1, x2, y2, opts = {}) | |
# @option styles [Integer] top (0) the y-coordinate of the top-left corner | ||
# @option styles [Boolean] center (false) is (left, top) the center of the oval | ||
def oval(*opts, &blk) | ||
oval_style = style_normalizer.normalize pop_style(opts) | ||
oval_style = pop_style(opts) | ||
case opts.length | ||
when 3 | ||
left, top, width = opts | ||
|
@@ -243,7 +243,7 @@ def oval(*opts, &blk) | |
EOS | ||
raise ArgumentError, message | ||
end | ||
create Shoes::Oval, left, top, width, height, style.merge(oval_style), &blk | ||
create Shoes::Oval, left, top, width, height, oval_style, &blk | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wasnotrice The merging all happens in style now, so in the dsl we don't do any merging. I'm glad you pointed this out since it was the problem! :) |
||
end | ||
|
||
# Creates a rectangle | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,23 @@ | ||
class Shoes | ||
class Oval | ||
include CommonMethods | ||
include Common::Fill | ||
include Common::Stroke | ||
include Common::Style | ||
include Common::Clickable | ||
include DimensionsDelegations | ||
|
||
attr_reader :app, :dimensions, :angle, :gui, :parent | ||
attr_reader :app, :dimensions, :parent, :gui | ||
style_with :art_styles, :center, :radius | ||
|
||
def initialize(app, parent, left, top, width, height, opts = {}, &blk) | ||
def initialize(app, parent, left, top, width, height, styles = {}, &blk) | ||
@app = app | ||
@dimensions = AbsoluteDimensions.new left, top, width, height, opts | ||
@style = Shoes::Common::Fill::DEFAULTS.merge( | ||
Shoes::Common::Stroke::DEFAULTS).merge(opts) | ||
@style[:strokewidth] ||= 1 | ||
@angle = opts[:angle] | ||
@dimensions = AbsoluteDimensions.new left, top, width, height, styles | ||
@parent = parent | ||
|
||
|
||
style_init(styles) | ||
@parent.add_child self | ||
|
||
@gui = Shoes.backend_for(self, &blk) | ||
|
||
clickable_options(opts) | ||
clickable_options(styles) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,21 +17,9 @@ | |
it_behaves_like "object with style" | ||
it_behaves_like "object with dimensions" | ||
it_behaves_like "left, top as center", :start_angle, :end_angle | ||
it_behaves_like 'object with parent' | ||
|
||
#unpack styles. In the future we'd like to do something clever to avoid this duplication. | ||
supported_styles = [] | ||
%w[art_styles cap center dimensions radius].map(&:to_sym).each do |style| | ||
if Shoes::Common::Style::STYLE_GROUPS[style] | ||
Shoes::Common::Style::STYLE_GROUPS[style].each{|style| supported_styles << style} | ||
else | ||
supported_styles << style | ||
end | ||
end | ||
|
||
supported_styles.each do |style| | ||
it_behaves_like "object that styles with #{style}" | ||
end | ||
it_behaves_like "object with parent" | ||
|
||
it_styles_with :art_styles, :center, :dimensions, :radius | ||
|
||
it "is a Shoes::Arc" do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realized that this runs cap twice |
||
expect(arc.class).to be(Shoes::Arc) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,11 +13,11 @@ | |
|
||
it "applies to subsequently created objects" do | ||
dsl.stroke color | ||
expect(Shoes::Oval).to receive(:new) do |*args| | ||
expect(Shoes::Rect).to receive(:new) do |*args| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed these to Rect from oval because they were breaking. If you test the things these are supposed to test, they do work as expected but the tests fail. So we should rework the dsl stroke, strokewidth, etc tests in a different PR. For now it works to just stick these tests on an element that doesn't use the new styling. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems totally reasonable to me. 👍 |
||
style = args.pop | ||
expect(style[:stroke]).to eq(color) | ||
end | ||
dsl.oval(10, 10, 100, 100) | ||
dsl.rect(10, 10, 100, 100) | ||
end | ||
|
||
context "with hex string" do | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,24 @@ | ||
require 'spec_helper' | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, so this is the only thing I changed essentially with this last commit. Something tells me this is pretty unorthodox and therefore maybe bad, but I view it more as a quick patch until the meta-styles spec issue (the one where we don't want to use lots of Whaddya think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm good with pulling these out in this fashion as long as it's an interim step, which seems to be your plan. |
||
def it_styles_with(*styles) | ||
supported_styles = unpack_styles(styles) | ||
spec_styles(supported_styles) | ||
end | ||
|
||
def unpack_styles(styles) | ||
supported_styles = [] | ||
styles.each do |style| | ||
if Shoes::Common::Style::STYLE_GROUPS[style] | ||
Shoes::Common::Style::STYLE_GROUPS[style].each{|style| supported_styles << style} | ||
else | ||
supported_styles << style | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also extract a method maybe and indent back one level (is a follow up error of the indention error before I guess) |
||
end | ||
supported_styles | ||
end | ||
|
||
def spec_styles(supported_styles) | ||
supported_styles.each do |style| | ||
it_behaves_like "object that styles with #{style}" | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1692,10 +1692,10 @@ also set, shapes drawn will not be visible. | |
Draws a line using the current line color (aka "stroke") starting at | ||
coordinates (left, top) and ending at coordinates (x2, y2). | ||
|
||
=== oval(left, top, radius) » Shoes::Shape === | ||
=== oval(left, top, diameter) » Shoes::Shape === | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on what I saw over in #118 I changed the manual too. |
||
|
||
Draws a circular form at pixel coordinates (left, top) with a width and height | ||
of `radius` pixels. The line and fill colors are used to draw the shape. By | ||
of `diameter` pixels. The line and fill colors are used to draw the shape. By | ||
default, the coordinates are for the oval's leftmost, top corner, but this can | ||
be changed by calling the [[Art.transform]] method or by using the `:center` | ||
style on the next method below. | ||
|
@@ -1718,7 +1718,7 @@ Draw circular form using a style hash. The following styles are supported: | |
|
||
* `top`: the y-coordinate for the oval pen. | ||
* `left`: the x-coordinate for the oval pen. | ||
* `radius`: the width and height of the circle. | ||
* `diameter`: the width and height of the circle. | ||
* `width`: a specific pixel width for the oval. | ||
* `height`: a specific pixel height for the oval. | ||
* `center`: do the coordinates specific the oval's center? (true or false) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that many of these kinds of elements can access their gui so I figured I'd add this for uniformity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the consistency of doing that. I've run into a number of cases where I had to add the
gui
to something just because no one had needed it quite yet.Although I don't like meta-programming the world away, almost makes me wonder if one of our
Common
modules ought to just enforce that everyone getsapp
,parent
,gui
and maybedimensions
... Food for later thought.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that. E.g. Shoes::CommonMethods or establish a base class. Something.
That wouldn't even need meta programming. You can just declare the readers in the module and bam they are there! (might be overridden in the actual class but hey no harm done!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I open an issue with this idea, or were you guys thinking to chunk that in with this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separate issue please!