Fix mixin/inheritance chain

[pjfitzgibbons]

I had to code lots of classes before I ran into a problem with this structure, so I think it was a completely reasonable idea. We adapt.

2 Answers:

A: Yes, currently, SwtShoes::Button is a mixin to Shoes::Button, and likewise for other Shoes classes and SwtShoes modules. In the SwtShoes::Button file, there is a reopening of Shoes::Button to mixin the SwtShoes module. It is expected in the sense that everything is currently coded that way.

A: Hmmm. I think we could go either way on this. Shoes::Button::Swt makes sense because the framework buttons kind of belong to the Shoes class. But when I visualize it:

Shoes                   Shoes
 Animation               Animation
 Button                    Swt
 Flow                      Swing
 ...                        ...
 Swt                     Button
   Animation               Swt
   Button                  Swing
   Flow                    ...
   ...                   Flow
 Swing                     Swt
   Animation               Swing
   Button                  ...
   Flow                  ...
   ...
 ...

I think it makes more sense to namespace like Shoes::Swt::Button. Since the gui framework classes are going to be bundled together, it makes sense that they are namespaced together. Compare:

Shoes                   Shoes
 Swt                     Animation
   Animation               Swt
   Button                Button
   Flow                    Swt
   ...                   Flow
                           Swt
                           ...

[pjfitzgibbons]

So Shoes::Animation is loaded, then by framework-loader, Shoes::Swt::Animation is loaded.

Should the GUI interface use before/after hooks to do its work? Do you have a vision for this?

[wasnotrice]

I kind of like the idea of before/after hooks, because it allows you to hook into the behavior rather than overriding it. It's potentially a significant amount of overhead, though.

class Shoes::Button
  def click
    @gui.before_click
    # all the code
    @gui.after_click
  end
end

(or some metaprogrammatical version of the same).

It may be desirable to override the behavior of a Shoes object sometimes. For that, what would we do? Something like this?

class Shoes::Button
  def initialize
    self.extend Shoes::Swt::Button::Overrides
  end
end

class Shoes::Swt::Button
  module Overrides
    def click
      # Different click behavior
    end
  end
end

Note that this is exactly the same thing we are doing now, except that it fixes the inheritance order.

[steveklabnik]

The before after turns them into middlewares, like Rack.

I'm gonna give all of this a big think today...

[pjfitzgibbons]

I will try to take time to dive into the latest code and refresh my memory

[steveklabnik]

TL;DR: Rack works like this:

class ZomgMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    puts "Before, yo!"
    @app.call(env)
    puts "after, playa"
  end
end

class LolMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    puts "lol!"
    @app.call(env)
    puts "wtfffff!"
  end
end


class MyRackApp
  def call(env)
    puts "So useful!"
  end
end

whatevs = Rack.builder do
  use ZomgMiddleware
  use LolMiddleware
end

whateves.mount MyRackApp

whatevs.run

... not 100% sure about the mounting. I'm sure you can see what this does.

[pjfitzgibbons]

I've not had time yet... need to get my elbows into this.

[wasnotrice]

Middlewares are great for building up systems. What other layers might work into a middleware stack?

This middleware idea also inverts the composition idea, so that the implementation becomes a wrapper around the DSL layer. If we wanted to stay with a 1:1 relationship like we have now, you could also make a case that Shoes::Swt::Button ought to be a decorator for Shoes::Button

[steveklabnik]

Oh, and Vagrant uses this as an architecture: http://www.youtube.com/watch?v=fcNaiP5tea0

[steveklabnik]

So I was toying around with this this evening, and I noticed one thing: the fact that Shoes is an instance_eval-y DSL makes some things hard.

First, a sample: https://gist.github.com/2785728

Basically none of the logic is in class Shoes, it's all in the adapter. So I tried doing this:

class Shoes
  Adapter = Tk

  def self.app(&blk)
    Adapter::App.new.instance_eval(&blk)
  end
end

But... then the instance_eval will kick off, and make further calls into the TK class' namespace. Which means we can't keep implementing logic up in Shoes proper...

[wasnotrice]

This is true. There has to be a certain open-endedness about the structure. If we are trying to make this DSL open to pluggable backends, then it seems like we want to keep as much logic as possible in the Shoes DSL proper (i.e. not repeat it for every implementation). So putting all the logic in the adapter might not be what we want anyway...

[wasnotrice]

I liked the talk about Vagrant and middleware. The wins for Vagrant were clear: composition through middleware enables simplification and reuse. In Shoes, I can see the simplification of objects (and after looking through the specs tonight, we desperately need this), but I can't yet see the reuse. Do we get that benefit?

[steveklabnik]

I'm not sure we do. I actually think that the pattern has a lot of issues, but just trying to get a number of options on the table...

[wasnotrice]

Yeah, I think that's a good idea. I just keep feeling like there's something there that I'm missing. So help me think about this differently. Take our current Shoes::Shape class. It begins like this:

module Shoes
  class Shape
    include Shoes::CommonMethods
    include Shoes::Common::Paint
    include Shoes::Common::Style

and also gets the SwtShoes::Button class mixed in:

module Shoes
  class Button
    include SwtShoes::Button
  end
end

This is starting to look like a multilayered system. What if we wrote it like this:

button = Shoes.builder do
  use Shoes::Common
  use Shoes::Common::Paint
  use Shoes::Common::Style
  use Shoes::Button
  use SwtShoes::Button
end

?

[ashbb]

So Shoes::Animation is loaded, then by framework-loader, Shoes::Swt::Animation is loaded.

Umm,... Am I missing something?
Why do you have to add the namespace Swt?
Can't you choose the Swt or Swing before loading their own Shoes::Animation?
If user want to add his/her own method Button#hello(), does he/her have to write like this???

class Shoes::Swt::Button
  def hello
    # bla bla bla
  end
end

Shoes.app do
  b.button 'my own button'
  b.hello
end

[wasnotrice]

I hope not :)

But this is a good point to consider. I hadn't thought about where a user would extend Shoes::Button. It would be nice if the implementation code was not visible to the user, so that the user only sees Shoes::Button and friends.

Keep in mind this is all to make it easy to reuse the Shoes DSL with different implementations.

[ashbb]

Keep in mind this is all to make it easy to reuse the Shoes DSL with different implementations.

I see. But, can't you choose Swt or Swing before loading their own Shoes::Button?

[pjfitzgibbons]

HI Folks,

Clarity here.

Shoes and Shoes:: should be invisible to the "User". Remember
we're on the dev team and hold a different context than our end-user.

As far as construction, I believe we're going to run into significant
testability problems if the goes about just overriding all the
methods in Shoes. Also note that the methods in Shoes::* provide a DSL,
and our Shoes:: are trying to apply GUI interactions inside
that DSL.

[wasnotrice]

Yes. As it is right now, Shoes mixes in the Swt modules automatically. The user doesn't see them. See shoes/brown_shoes#14 for the beginning of the discussion.

[wasnotrice]

Shoes and Shoes:: should be invisible to the "User".

Well, Shoes can't be entirely invisible to the user.

b = button "squish"

Now b is a Shoes::Button

I'm not sure whether it's important that users can open that class to add/modify behavior, but it's a good question to ask right now.

[pjfitzgibbons]

Oh, I mis-worded that sentence.

[ashbb]

I read shoes/brown_shoes#14 and understood the background of this discussion.
I like Shoes::Swt::Button than mixining SwtShoes::Button into Shoes::Button.
But, I still don't understand why you need <Framewrorks> namespace...
Isn't it possible to use Swt::Shoes::Button instead of Shoes::Swt::Button?
If it's possible, you might not have to use <Framewrorks> namespace, though...

There are two directories, lib/swt_shoes and lib/swing_shoes. We can load each of them.
Can't you say enough?

[wasnotrice]

We could do it with either namespace. I prefer Shoes::Swt::Button because then everything is in the Shoes namespace. But that might just be my personal preference.

require 'shoes'
require 'shoes/swt'

vs

require 'shoes'
require 'swt/shoes'

[ashbb]

Umm, ... but OK for now. Can you close this issue?
After studying a little bit more about current implementation, I'll reopen if necessary. :)

[pjfitzgibbons]

The issue remains open until we've done the refactoring work.

An additional question is outstanding for #4, not sure if it's answerable
through discussion:
Does our want to do _before/_after hooks, or do we need to
create specific "do_gui_stuff" hooks ?

You'll see in the code that I have "do_gui_stuff" type hooks, since the
operation of the SWT lib is sometimes an exact implementation of the
necessary widget (ie Button), and sometimes needs some mapping into the
Shoes DSL (ie Flow, WIndow, etc)

Thoughts?

Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

On Fri, May 25, 2012 at 9:11 PM, ashbb <
reply@reply.github.com

wrote:

Umm, ... but OK for now. Can you close this issue?
After studying a little bit more about current implementation, I'll reopen
if necessary. :)

---

Reply to this email directly or view it on GitHub:
#4 (comment)

[wasnotrice]

In Shoes::App#initialize, there are two "do_gui_stuff" calls. gui_open could be run as an _after hook, but it would take some fiddling to get gui_init to run as either a _before or _after hook.

Here's how I propose we solve this issue:

• Change implementation namespace from SwtShoes to Shoes::Swt
• Change Shoes::Swt modules to classes: Shoes::Swt::App
• Rewrite (and drastically simplify) specs for Shoes::Swt modules to test the new classes.
• Add an instance variable to the Shoes classes (Shoes::App) that is an instance of the Swt class (Shoes::Swt::App). The possible names for this instance variable that come to mind are @real or @gui. Preferences?
• change "do_gui_stuff" hooks from calls on the Shoes::Swt mixins to calls on the Shoes::Swt instance variables.

I still don't know about the whole middleware idea, but I think that this refactoring puts us in a better place for future refactorings. Less coupling.

Thoughts?

[ashbb]

This is not direct answer, sorry. But I don't know yet why implementation namespace, i.e. Swt, is necessary. :(

I changed SwtShoes to Shoes in shoes4/lib/swt_shoes/app.rb like this:

C:\tmp\shoes4\lib\swt_shoes>diff app.rb app_new.rb
5c5
< module SwtShoes
---
> module Shoes
10c10
<   module App
---
>   class App
42,47d41
<   end
< end
<
< module Shoes
<   class App
<     include SwtShoes::App

and did C:\tmp\shoes4>bin\swt-shoooes samples\test1.rb. It works well.

The samples/test1.rb is Shoes.app{}.

If possible, I'd like to use just Shoes, neither Shoes::Swt nor SwtShoes...

[pjfitzgibbons]

HI Ash,

What you're proposing is a code smell. I can't recall a documentation
point for you, unfortunately.
Having two libraries that are completely different internally gets very
very confusing. This is pretty much the whole reason mixins and
inheritance exist... code-encapsulation. Some encapsulation is not just
about objects or methods... when we build large libraries, whole groups of
methods need encapsulation, if for no other reason than to organize our
brains and be able to compartmentalize the library. That encapsulation
also allows code-reuse, though code-reuse is not the sole, pure motivation
for the original creation of mixins.
Some of this comes from Smalltalk, and the creators of Smalltalk were way
more than hardline pragmatists... beautiful code was/is extremely important
to them.

Thoughts?

[ashbb]

Umm,... No. My point is different.
Please look at line 8 in bin/swt-shoooes

Shoes.configuration.framework = 'swt_shoes'

and line 9-11 in lib/shoes/configuration.rb

def framework=(value)
  @framework = value
  require value
end

You've already choose the framework (require the file) in the above. So, it's okay to use the same module name 'Shoes' both in lib/swt_shoes/app.rb and lib/swing_shoes/app.rb even if they are completely different.

Am I missing something?

[wasnotrice]

@ashbb let me see if I understand you correctly. I think you are saying that because we choose the framework once, and don't change it, there is no functional difference between mixing in a module like SwtShoes::Shoes and monkey patching the Shoes class. So why introduce a new namespace?

Assuming I do understand :)

Peter's point is still true for me—it is easier (for me) to reason about the code when it has a different name. However, your idea does solve the problem that I first noticed. It makes the code in SwtShoes override the code in Shoes.

But I am suggesting that we turn those modules into classes. So we would have Shoes::App and Shoes::Swt::App that are different classes. I don't think you can we could do that without a separate namespace, could we?

[pjfitzgibbons]

HI Eric,

Not sure why these need to be classes? As mixins the framework can still
introduce instance-variables and do the most-proper _before/_after hook
defs.

Could you tell me about other motivations to make the framework be classes
instead of modules?

[wasnotrice]

Oh, I didn't think you were suggesting Draper :) I'm enjoying stretching the mind a little...

Your example instantiation code is pretty much what I was thinking, except that we would want to instantiate a Shoes::Swt::Button, not a Shoes::Swt, right? So all this kind of ugliness:

Shoes.backend = Shoes::Swt # this would be the default, but could be overriden like this.

module Shoes
  class Button
    def initialize
      @gui = Shoes.backend.const_get("Button").new
    end
  end
end

[pjfitzgibbons]

Does this video have visual for y'all? #my-new-os-install-is-broken?

[pjfitzgibbons]

Shoes.backend::Button should also work? (How do I prove this in IRB?)

[steveklabnik]

@wasnotrice Ahhh yeah, that's what happens when it's from memory. ;)

@pjfitzgibbons yes, it does:

$ irb
1.9.3-p194 :001 > module Shoes
1.9.3-p194 :002?>   attr_accessor :backend
1.9.3-p194 :003?>   class Swt
1.9.3-p194 :004?>     end
1.9.3-p194 :005?>   class Button
1.9.3-p194 :006?>     def initialize
1.9.3-p194 :007?>       @gui = Shoes.backend::Button.new
1.9.3-p194 :008?>       end
1.9.3-p194 :009?>     end
1.9.3-p194 :010?>   end
 => nil 
1.9.3-p194 :011 > Shoes::Button.new
NoMethodError: undefined method `backend' for Shoes:Module
    from (irb):7:in `initialize'
    from (irb):11:in `new'
    from (irb):11
1.9.3-p194 :012 > module Shoes
1.9.3-p194 :013?>   class << self
1.9.3-p194 :014?>     attr_accessor :backend
1.9.3-p194 :015?>     end
1.9.3-p194 :016?>   end
 => nil 
1.9.3-p194 :017 > Shoes::Button.new
TypeError:  is not a class/module
    from (irb):7:in `initialize'
    from (irb):17:in `new'
    from (irb):17
1.9.3-p194 :018 > Shoes.backend = Shoes::Swt
 => Shoes::Swt 
1.9.3-p194 :019 > Shoes::Button.new
NameError: uninitialized constant Shoes::Swt::Button
    from (irb):7:in `initialize'
    from (irb):19:in `new'
    from (irb):19
1.9.3-p194 :020 > 

I forgot to actually make a Shoes::Swt::Button, hence the last error.

And yes, it has video. It uses the HTML5 player by default iirc...

[wasnotrice]

Video didn't work for me either

[steveklabnik]

Oh! Well, I posted the wrong one, anyways: https://vimeo.com/42622511 was what I meant. Sigh...

[pjfitzgibbons]

I'm really liking the direction of this. Feels like an actual plugin.

Does the following outline our usage correctly?

I assigned this issue to me... I'd really like to do this one as soon as we
agree on a direction.

I got this, as a known-good example :

irb(main):001:0> module Shoes
irb(main):002:1> class << self
irb(main):003:2> attr_accessor :backend
irb(main):004:2> end
irb(main):005:1> end
=> nil
irb(main):006:0> module Shoes::Swt
irb(main):007:1> class Button
irb(main):008:2> end
irb(main):009:1> end
=> nil
irb(main):010:0> Shoes.backend
=> nil
irb(main):011:0> Shoes.backend = Shoes::Swt
=> Shoes::Swt
irb(main):012:0> Shoes.backend::Button
=> Shoes::Swt::Button

[steveklabnik]

👍

[wasnotrice]

+1

[pjfitzgibbons]

HI Folks,

./lib/shoes contains all the base classes, Shoes::Button, Shoes::Window,
etc.

Filepath for Shoes::Swt ends up being ./lib/shoes/swt ... this is
confusing... "is it part of base-Shoes?"

I'm thinking of placing the backends in ./lib/backends/swt/shoes/swt
The loadpath can be added in ./bin/swt-shoooes:
$: << "./lib/backends/swt"

Then this will work ok :
require 'shoes/swt'

Thougths?

[steveklabnik]

Maybe it's the Rails in me, but I really think that if you require 'shoes/swt', it should be lib/shoes/swt.rb.

[pjfitzgibbons]

We now have two problems :

1. ./lib/shoes/swt will be "buried" in the forest of the other Shoes
   modules.
2. swt gem defines module Swt, which contains this :
   Swt.constants
   => [:Browser,
   :Layout,
   :CucumberRunner,
   :VERSION,
   :EventLoop,
   :RRunnable,
   :DND,
   :SWT,
   :Events,
   :Graphics,
   :Widgets,
   :Custom]

So, Swt::Shoes is buried in the forest of Swt gem.

I believe that my choice to name the backend "SwtShoes" was an attempt to
escape these two namespace conflicts.

Thoughts?

Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

On Sun, May 27, 2012 at 9:21 PM, Steve Klabnik <
reply@reply.github.com

wrote:

Maybe it's the Rails in me, but I really think that if you require 'shoes/swt', it should be lib/shoes/swt.rb.

---

Reply to this email directly or view it on GitHub:
#4 (comment)

[wasnotrice]

I definitely think Shoes::Swt should be in lib/shoes/swt. This is POLS.

It shouldn't produce that much confusion. To me, it says, "Hey, this is part of Shoes...the Swt part." That makes more sense than "Hey, this is part of Swt...the Shoes part." And also less confusion than manipulating the load path to include files from other directories. Although admittedly, adding the spec directory to the load path was part of getting Shoes to build on Travis :)

If we are worried that Shoes::Swt will be lost among other Shoes business, we can always extract it into its own gem at the point where it causes problems.

[ashbb]

Through all the above discussion, it would appear to me that the following is the code structure you want. Right?

module Shoes
  class << self
    attr_accessor :backend
  end

  def self.app &blk
    App.new &blk
  end

  class App
    def initialize &blk
      instance_eval &blk
    end
    def button
      Shoes::Button.new
    end
  end

  class Button
    def initialize
      @gui = Shoes.backend::Button.new
    end
    attr_accessor :gui
  end
end

module Swt # instead of Shoes::Swt
  class Button; end
end

Shoes.backend = Swt # instead of Shoes::Swt

Shoes.app do
  p self       #=> #<Shoes::App:0xf6b148>
  b = button
  p b.class    #=> Shoes::Button
  p b.gui      #=> #<Swt::Button:0xf6b0b8>
end

[steveklabnik]

If we don't namespace our Swt stuff, won't we run afoul of the official Swt stuff?

[pjfitzgibbons]

HI Ash,

You're exactly right... with one exception, which I'll make sure to cover :

Shoes.app do
p self #=> #Shoes::App:0xf6b148
b = button
p b.class #=> Shoes::Button
p b.gui #=> #Swt::Button:0xf6b0b8 # I want this to return
#Shoes::Swt::Button:0xdeadbeef
end

Thanks for the confirmation.

[pjfitzgibbons]

Well, yes we will run into the Swt namespace in the gem.
This is why I named the backend "SwtShoes"

Other than "hoping" that we don't clash the namespace, I'm open to
suggestions better than SwtShoes and avoiding the Swt namespace.

Thoughts?

[steveklabnik]

I don't have any kind of problem with Shoes::Swt.

[pjfitzgibbons]

Here's a test inside the new namespace, with some runtime results (pry
prompts redacted)

module Shoes
module Swt
class Test
def self.test
Swt # => Shoes::Swt
Swt.constants # => [:Test]
::Swt # => Swt
::Swt.constants # =>
#[:Browser,
#:Layout,
#:CucumberRunner,
#:VERSION,
#:EventLoop,
#:RRunnable,
#:DND,
#:SWT,
#:Events,
#:Graphics,
#:Widgets,
#:Custom]
end
end
end
end

I think we're clear. I'm going for it.

Shoes On!
Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

On Mon, May 28, 2012 at 9:21 AM, Steve Klabnik <
reply@reply.github.com

wrote:

I don't have any kind of problem with Shoes::Swt.

---

Reply to this email directly or view it on GitHub:
#4 (comment)

[pjfitzgibbons]

Check me :

The global #window, currently defined in ./lib/swt_shoes.rb really belongs on ./lib/shoes... no?

def window(_a, &b)
Shoes::App(_a, &b)
end

[steveklabnik]

Yes.

[wasnotrice]

I think this all looks good except that we should keep our Swt stuff in the Shoes namespace. That's what a namespace is for :)

If we dont like saying

Shoes.backend = Shoes::Swt

we can certainly code Shoes.backend so that

Shoes.backend = Swt # => Shoes::Swt

[wasnotrice]

This issue is HUGE. Would it be possible/helpful to separate into 2 issues, so that we can parallelize the tasks?

1. Mixins vs composition
2. Namespace (see Fix namespace of Swt: SwtShoes::* -> Shoes::Swt::* #6)
3. Cleaning up methods/libs that actually belong somewhere else

[ashbb]

Okay. Move to each separated threads.

1. Mixins vs composition ==> issue Mixins vs composition #34
2. Namespace (see Fix namespace of Swt: SwtShoes::* -> Shoes::Swt::* #6) ==> issue Fix namespace of Swt: SwtShoes::* -> Shoes::Swt::* #6
3. Cleaning up methods/libs that actually belong somewhere else ==> issue Cleaning up methods/libs that actually belong somewhere else #35

[pjfitzgibbons]

Ok, I see what happened here.
I'll attempt to cleanup later, tomorrow, adding info to the forking threads.

Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

[wasnotrice]

Closing as fixed. See #34, #35