Updating Oval to use new styles #736

KCErb · 2014-05-28T22:15:21Z

This isn't quite ready yet. I've only found one problem (which really means two problems since in the meta issue #711 we plan to change the way style specs work )

The issue is in setting oval styles via the style method:

Shoes.app do
  style(Shoes::Oval, fill: green)
  oval 10, 10, 50
end

produces a black oval. I haven't figured that out yet, but figured I'd commit what I have and stick it up here until next chance I get to look at this.

KCErb · 2014-05-28T22:15:50Z

lib/shoes/arc.rb

@@ -7,7 +7,7 @@ class Arc
    include Common::Clickable
    include DimensionsDelegations

-    attr_reader :app, :parent, :dimensions


I noticed that many of these kinds of elements can access their gui so I figured I'd add this for uniformity.

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 gets app, parent, gui and maybe dimensions... Food for later thought.

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!)

Also: good idea!

Should I open an issue with this idea, or were you guys thinking to chunk that in with this PR?

separate issue please!

coveralls · 2014-05-28T22:23:54Z

Coverage decreased (-0.27%) when pulling e179525 on KCErb:oval_style into 3b1960d on shoes:master.

PragTob · 2014-05-29T16:04:38Z

spec/shoes/oval_spec.rb

+      else
+        supported_styles << style
+      end
+    end


all of the above could/should go in a method that gathers the supported styles to avoid duplicating that code everywhere

I agree. One of the (meta issues to be addressed later) is about how to centralize styles better. So it seems to me that in this PR (and in similar ones i.e. rect or star) I'll need to put something together quickly to handle specs, and then redo it later when we attack the specs meta issue.

Is that right? I ask because I'm not sure to what extent I should tackle the duplication issue in this PR :)

I like to tackle duplication as soon as I see it. You know because otherwise the amount of work just gets bigger, e.g. in the next PRs you'd need to copy that same code... and then when you make it all good again you have to refactor all of them.

So I'd like to tackle this as early as possible.

PragTob · 2014-05-29T16:05:05Z

Looks good overall!

jasonrclark · 2014-05-30T04:26:24Z

Agreed, this is definitely on a good path forward. 🚀

KCErb · 2014-06-03T13:57:34Z

spec/shoes/spec_helper.rb

@@ -1 +1,18 @@
 require 'spec_helper'
+


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 it_behaves_likes) gets addressed.

Whaddya think?

I'm good with pulling these out in this fashion as long as it's an interim step, which seems to be your plan.

coveralls · 2014-06-03T14:01:13Z

Coverage decreased (-0.25%) when pulling c9bbc28 on KCErb:oval_style into 3b1960d on shoes:master.

coveralls · 2014-06-03T14:01:13Z

Coverage decreased (-0.25%) when pulling c9bbc28 on KCErb:oval_style into 3b1960d on shoes:master.

PragTob · 2014-06-03T15:15:20Z

spec/shoes/spec_helper.rb

+def it_styles_with(*styles)
+  #unpack styles 
+  supported_styles = []
+    styles.each do |style|


should not be indented

coveralls · 2014-06-04T02:57:12Z

Coverage decreased (-42.92%) when pulling be703d2 on KCErb:oval_style into 3b1960d on shoes:master.

KCErb · 2014-06-04T02:59:19Z

Wow, that's a big coverage decrease . . . just a sec

coveralls · 2014-06-04T03:14:37Z

Coverage decreased (-0.29%) when pulling 4986d22 on KCErb:oval_style into 382bbd5 on shoes:master.

coveralls · 2014-06-04T03:19:48Z

Coverage decreased (-0.29%) when pulling 4986d22 on KCErb:oval_style into 382bbd5 on shoes:master.

PragTob · 2014-06-04T18:23:43Z

@KCErb do you think this is ready for merge?

KCErb · 2014-06-04T19:05:26Z

Nearly, I just haven't been able to solve the issue in the first comment yet. Everything I've looked at seems to be working correctly so I don't know why oval won't listen to style(Shoes::Oval, fill: green) once that works then I'm ready to merge.

wasnotrice · 2014-06-05T07:11:39Z

@KCErb I think the problem is in lib/shoes/dsl.rb. We don't call #style_for_element when we create an oval in #oval. Looking at it a little, I think we are also missing a way to merge

element defaults
current app styles
element styles

In #style_for_element, we merge element defaults and element styles. In #oval, we merge app styles and element styles, but I think we need to merge all three.

I can't get to it right now, but we should add a spec for this merging of styles, so we know exactly how we expect it to behave, and since it doesn't seem to be tested.

coveralls · 2014-06-05T14:13:12Z

Changes Unknown when pulling bc88438 on KCErb:oval_style into * on shoes:master*.

coveralls · 2014-06-05T14:13:14Z

Changes Unknown when pulling bc88438 on KCErb:oval_style into * on shoes:master*.

KCErb · 2014-06-05T14:31:19Z

I think this is ready to merge.

KCErb · 2014-06-05T14:34:34Z

lib/shoes/dsl.rb

@@ -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


@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! :)

jasonrclark · 2014-06-07T04:46:01Z

Read over this and code-wise things are looking good for a merge. I'm liking how the new styling is playing out... and nice catch on the manual as well!

I decided to do some testing, and found one issue with samples/expert-pong.rb. It doesn't draw the ball and spews a bunch of paint exceptions on the console:

SWALLOWED PAINT EXCEPTION - go take care of it#9B7 does not have a backend class defined for Shoes::Swt
Unfortunately we have to swallow it because it causes odd failures :(

This looks a lot like the color string isn't getting translated into a Color object on it's way through the system.

Do you want to take a look at it @KCErb? Or I'm happy to debug and/or give you any assistance I can!

Thanks again for all the hard work!

Good work on

KCErb · 2014-06-08T01:09:41Z

OK, I think I fixed it. I just needed to normalize the styles before they go in. In my test cases I'd been passing arc and oval Shoes Colors like blue but I never tried things like hex strings so I didn't catch this. Thanks @jasonrclark!

It does make me want to change this aspect of how Styles are handled . . . though I can't think of a better way. . . I'm just not a fan of creating a StyleNormalizer class, just so that we can pass fill and stroke arguments to pattern.

I may put something over in style that does this. Any suggestions?

PS. Now I think this is ready to merge :)

coveralls · 2014-06-08T01:14:31Z

Changes Unknown when pulling c9cb687 on KCErb:oval_style into * on shoes:master*.

wasnotrice · 2014-06-08T12:51:00Z

In my test cases I'd been passing arc and oval Shoes Colors like blue but I never tried things like hex strings so I didn't catch this.

This is a good time to add a spec for this case 😉

PragTob · 2014-06-08T13:39:29Z

In my test cases I'd been passing arc and oval Shoes Colors like blue but I never tried things like hex strings so I didn't catch this.

This is a good time to add a spec for this case 😉

Just building upon what Eric said: It is arguable the best time to add specs. If an error occurred once it sure can occur twice. Also it's a good way to practice TDD. You have a very concrete problem and you want to write a spec for it. Write the spec. Run it. It should fail. Then implement it, rerun. Spec should pass. Run a demo app to make sure et voila :)

Regression specs are of utmost importance imo :)

Get rid of "global" subject, and explicitly create the "oval" object in each describe block. We were creating a 'circle' object, but running specs against an 'oval' object that was inherited from a parent context.

Add specs for creating ovals with hex strings

KCErb · 2014-06-09T18:09:30Z

Thanks @wasnotrice! I've been so swamped this last week, sorry for being slow!

coveralls · 2014-06-09T18:17:23Z

Changes Unknown when pulling 2437857 on KCErb:oval_style into * on shoes:master*.

PragTob · 2014-06-09T19:11:04Z

So anything missing here? Or is this ready to be merged in? Another review or so?

KCErb · 2014-06-09T19:49:36Z

I believe that everything is working except for the issues I just opened. 🚀

Update Arc and Oval to use Style module First step in a larger conversion of all styled DSL objects

wasnotrice · 2014-06-10T02:57:52Z

Great work, @KCErb! Thanks for all of your hard work on this, and your perseverance! I noticed you had not yet been added to Team Shoes, so I also fixed that oversight!

KCErb · 2014-06-10T04:34:45Z

Hey great! I didn't know about teams. I'll try not to actually use any of those admin privileges. Sounds dangerous 💥

wasnotrice · 2014-06-10T04:39:25Z

One handy thing is that you can create branches in the shoes/shoes4
repository, which makes it a bit easier to collaborate on pull requests.

On Mon, Jun 9, 2014 at 11:34 PM, KCErb notifications@github.com wrote:

Hey great! I didn't know about teams. I'll try not to actually use any of
those admin privileges. Sounds dangerous [image: 💥]

—
Reply to this email directly or view it on GitHub
#736 (comment).

first draft of updating oval with a hint of arc

e179525

KCErb reviewed May 28, 2014
View reviewed changes

PragTob reviewed May 29, 2014
View reviewed changes

added it_styles_with method

c9bbc28

KCErb reviewed Jun 3, 2014
View reviewed changes

PragTob reviewed Jun 3, 2014
View reviewed changes

updated it_styles_with

be703d2

KCErb added 2 commits June 3, 2014 21:08

fixed bug in spec helper

49873d8

merged spec changes

4986d22

KCErb added 2 commits June 5, 2014 07:44

stash changes before merge

ee9e8d6

merged master with spec updates

7c593c9

fixed spec and style issues

bc88438

KCErb reviewed Jun 5, 2014
View reviewed changes

normalize styles

c9cb687

wasnotrice added 4 commits June 9, 2014 12:31

Fix circle specs (using wrong subject)

90851e2

Get rid of "global" subject, and explicitly create the "oval" object in each describe block. We were creating a 'circle' object, but running specs against an 'oval' object that was inherited from a parent context.

Add stroke and fill specs for creating ovals

d87e3c0

Fix spec descriptions to read better

f25b480

Add specs for creating ovals with hex fill/stroke

9384835

This was referenced Jun 9, 2014

Add specs for creating ovals with hex strings KCErb/shoes4#3

Merged

Add #to_s and #inspect for classes that include CommonMethods #747

Merged

Merge pull request #3 from wasnotrice/oval_style

2437857

Add specs for creating ovals with hex strings

KCErb mentioned this pull request Jun 9, 2014

Everyone gets app, parent, gui #752

Closed

wasnotrice added a commit that referenced this pull request Jun 10, 2014

Merge pull request #736 from KCErb/oval_style

9a43417

Update Arc and Oval to use Style module First step in a larger conversion of all styled DSL objects

wasnotrice merged commit 9a43417 into shoes:master Jun 10, 2014

KCErb deleted the oval_style branch July 29, 2014 13:24

Updating Oval to use new styles #736

Updating Oval to use new styles #736

Conversation

KCErb commented May 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragTob commented May 29, 2014

jasonrclark commented May 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 3, 2014

coveralls commented Jun 3, 2014

Choose a reason for hiding this comment

coveralls commented Jun 4, 2014

KCErb commented Jun 4, 2014

coveralls commented Jun 4, 2014

coveralls commented Jun 4, 2014

PragTob commented Jun 4, 2014

KCErb commented Jun 4, 2014

wasnotrice commented Jun 5, 2014

coveralls commented Jun 5, 2014

coveralls commented Jun 5, 2014

KCErb commented Jun 5, 2014

Choose a reason for hiding this comment

jasonrclark commented Jun 7, 2014

KCErb commented Jun 8, 2014

coveralls commented Jun 8, 2014

wasnotrice commented Jun 8, 2014

PragTob commented Jun 8, 2014

KCErb commented Jun 9, 2014

coveralls commented Jun 9, 2014

PragTob commented Jun 9, 2014

KCErb commented Jun 9, 2014

wasnotrice commented Jun 10, 2014

KCErb commented Jun 10, 2014

wasnotrice commented Jun 10, 2014