Widgets more than 2 levels deep, when :passing => :root raises error #34

Closed
ramontayag opened this Issue Jul 29, 2011 · 19 comments

Comments

Projects
None yet
3 participants
@ramontayag

The error is like this:

A ActionView::Template::Error occurred in pages#edit:
  undefined method `respond_to_event' for nil:NilClass
  activesupport (3.0.9) lib/active_support/whiny_nil.rb:48:in `method_missing'

@apotonick, I know I said I was going to do a failing test case but I couldn't get all tests to pass on a clean install. I've placed that in a separate ticket.

@ramontayag

This comment has been minimized.

Show comment Hide comment
@ramontayag

ramontayag Jul 29, 2011

I placed it on my own branch, so you can just get the commits you want. Hope I did it right!

I placed it on my own branch, so you can just get the commits you want. Hope I did it right!

@ramontayag ramontayag closed this Jul 29, 2011

@ramontayag ramontayag reopened this Jul 29, 2011

@ramontayag

This comment has been minimized.

Show comment Hide comment
@ramontayag

ramontayag Aug 20, 2011

Problem is probably in this line. It can't seem to find the root node, thus returning nil.

Problem is probably in this line. It can't seem to find the root node, thus returning nil.

@apotonick

This comment has been minimized.

Show comment Hide comment
@apotonick

apotonick Aug 22, 2011

Owner

The problem is the after_add hook, look:


Now, b << c is executed first, when c is added to b, the after_add hook is run which adds c's global event handlers to "root", since c thinks b is root - we have to change the entire way of adding global event handlers :-/

Owner

apotonick commented Aug 22, 2011

The problem is the after_add hook, look:


Now, b << c is executed first, when c is added to b, the after_add hook is run which adds c's global event handlers to "root", since c thinks b is root - we have to change the entire way of adding global event handlers :-/

@ramontayag

This comment has been minimized.

Show comment Hide comment
@ramontayag

ramontayag Aug 22, 2011

Wow that sounds like major refactoring. Something out of my league! I just flattened my widgets in the mean time.

Wow that sounds like major refactoring. Something out of my league! I just flattened my widgets in the mean time.

@cameel

This comment has been minimized.

Show comment Hide comment
@cameel

cameel Oct 6, 2011

@apotonick: Could you share your ideas on how you would go about fixing this? Maybe I could implement it and make a pull request? I'm also seeing this issue and I need to have it working in the near future.

I think that we need to change the order in which the widget tree is constructed. There is another issue where this is a problem: #40. I wanted to work around that issue it by constructing widget ids hierarchically like this

has_widgets do
    root << widget(:some_widget, root.name + "_some_widget")
end

but it's not possible if the has_widgets block is inside another widget - root's name is not known there yet.

cameel commented Oct 6, 2011

@apotonick: Could you share your ideas on how you would go about fixing this? Maybe I could implement it and make a pull request? I'm also seeing this issue and I need to have it working in the near future.

I think that we need to change the order in which the widget tree is constructed. There is another issue where this is a problem: #40. I wanted to work around that issue it by constructing widget ids hierarchically like this

has_widgets do
    root << widget(:some_widget, root.name + "_some_widget")
end

but it's not possible if the has_widgets block is inside another widget - root's name is not known there yet.

@apotonick

This comment has been minimized.

Show comment Hide comment
@apotonick

apotonick Oct 6, 2011

Owner

I guess the "only" problem is the operator precedence: << is run from right to left, right? If we would use + things would work as expected, meaning first, root is created, then the child, then added, then the child's has_widget block would be run. Am I right here?

Owner

apotonick commented Oct 6, 2011

I guess the "only" problem is the operator precedence: << is run from right to left, right? If we would use + things would work as expected, meaning first, root is created, then the child, then added, then the child's has_widget block would be run. Am I right here?

@ramontayag

This comment has been minimized.

Show comment Hide comment
@ramontayag

ramontayag Oct 6, 2011

That's right. Evaluate from right to left. Would this entail a lot of refactoring still?

That's right. Evaluate from right to left. Would this entail a lot of refactoring still?

@apotonick

This comment has been minimized.

Show comment Hide comment
@apotonick

apotonick Oct 6, 2011

Owner

Uhm, The << operator would have to be replaced with +... in your code, haha.

Owner

apotonick commented Oct 6, 2011

Uhm, The << operator would have to be replaced with +... in your code, haha.

@cameel

This comment has been minimized.

Show comment Hide comment
@cameel

cameel Oct 6, 2011

But what about has_widgets nested in a widget? If we have

# app/controllers/a.rb
class AController < ApplicationController
  has_widgets do |root|
    root << widget(:b)
  end
end
# app/widgets/b.rb
class BWidget < Apotomo::Widget
  has_widgets do
    self << widget(:c)
  end
end
# app/widgets/c.rb
class CWidget < Apotomo::Widget
  has_widgets do
    self << widget(:d)
  end
end
# app/widgets/d.rb
class DWidget < Apotomo::Widget
end

won't the order be as in the case of << operator? DWidget then CWidget then BWidget then root?

cameel commented Oct 6, 2011

But what about has_widgets nested in a widget? If we have

# app/controllers/a.rb
class AController < ApplicationController
  has_widgets do |root|
    root << widget(:b)
  end
end
# app/widgets/b.rb
class BWidget < Apotomo::Widget
  has_widgets do
    self << widget(:c)
  end
end
# app/widgets/c.rb
class CWidget < Apotomo::Widget
  has_widgets do
    self << widget(:d)
  end
end
# app/widgets/d.rb
class DWidget < Apotomo::Widget
end

won't the order be as in the case of << operator? DWidget then CWidget then BWidget then root?

@cameel

This comment has been minimized.

Show comment Hide comment
@cameel

cameel Oct 6, 2011

OK. I checked the execution order in that situation and it's actually root, BWidget, CWidget and DWidget so it turns out I was wrong.

The strange thing is that my colleague has stumbled into this exact ActionView::Template::Error when using nested widgets with :passing => :root and he was not using << in a way that would change the execution order. I thought it was because Apotomo first created leaf nodes of the widget tree but it seems not to be the case. I'll have to check his failing test case again.

cameel commented Oct 6, 2011

OK. I checked the execution order in that situation and it's actually root, BWidget, CWidget and DWidget so it turns out I was wrong.

The strange thing is that my colleague has stumbled into this exact ActionView::Template::Error when using nested widgets with :passing => :root and he was not using << in a way that would change the execution order. I thought it was because Apotomo first created leaf nodes of the widget tree but it seems not to be the case. I'll have to check his failing test case again.

@apotonick

This comment has been minimized.

Show comment Hide comment
@apotonick

apotonick Oct 6, 2011

Owner

I think.. the problem is that the has_widgets block in a widget is run in the after_initialize hook, which is "too soon". If we change that to use the after_add hook, things should work. Still not sure about the operator precedence. We might need to use + here, or #add.

Owner

apotonick commented Oct 6, 2011

I think.. the problem is that the has_widgets block in a widget is run in the after_initialize hook, which is "too soon". If we change that to use the after_add hook, things should work. Still not sure about the operator precedence. We might need to use + here, or #add.

@apotonick

This comment has been minimized.

Show comment Hide comment
@apotonick

apotonick Oct 6, 2011

Owner

So, I reflected a bit about the entire problem. The only really clean solution I can see right now would be to require the parent widget in the widget's constructor. This way we could provide root - the real root safely in every child widget. Every other solution is a work-around with drawbacks. Of course, that sucks since we all like the dynamic nature of the widget tree, so let me think more...

Owner

apotonick commented Oct 6, 2011

So, I reflected a bit about the entire problem. The only really clean solution I can see right now would be to require the parent widget in the widget's constructor. This way we could provide root - the real root safely in every child widget. Every other solution is a work-around with drawbacks. Of course, that sucks since we all like the dynamic nature of the widget tree, so let me think more...

@cameel

This comment has been minimized.

Show comment Hide comment
@cameel

cameel Oct 6, 2011

Maybe something like this:

  • Every widget has a flag that says whether its subtree is connected to the actual root (reference to the root can serve as this flag - it would be nil if not connected)
  • The actual root gets the flag set in it's constructor
  • A widget that has the flag set, every time a child is being connected to it, executes a method on it (say new_root() or root_added()... or respect_my_authority() ;)) to pass it the flag and root reference
  • A widget that does not have the flag set, does not call the method on children that connect to it. It waits for the flag and when it finally gets it, it executes the method on all the children that are already connected.
  • The method would execute has_widgets block

cameel commented Oct 6, 2011

Maybe something like this:

  • Every widget has a flag that says whether its subtree is connected to the actual root (reference to the root can serve as this flag - it would be nil if not connected)
  • The actual root gets the flag set in it's constructor
  • A widget that has the flag set, every time a child is being connected to it, executes a method on it (say new_root() or root_added()... or respect_my_authority() ;)) to pass it the flag and root reference
  • A widget that does not have the flag set, does not call the method on children that connect to it. It waits for the flag and when it finally gets it, it executes the method on all the children that are already connected.
  • The method would execute has_widgets block
@apotonick

This comment has been minimized.

Show comment Hide comment
@apotonick

apotonick Oct 7, 2011

Owner

Currently, widgets have a 2-steps initialization life-cycle.

  • Creation using the constructor. Now, the has_widgets block is executed, which is too early.
  • After being added, the after_add hook is invoked. Multiple more things, like adding global handlers, happens here.

This is wrong and error-prone. I played around a bit and it turns out that having to pass the parent widget into the kid's constructor is perfect and makes a clean initialization where the real root is available and we don't need flags or maintain internal state.

I will push a branch soon.

Owner

apotonick commented Oct 7, 2011

Currently, widgets have a 2-steps initialization life-cycle.

  • Creation using the constructor. Now, the has_widgets block is executed, which is too early.
  • After being added, the after_add hook is invoked. Multiple more things, like adding global handlers, happens here.

This is wrong and error-prone. I played around a bit and it turns out that having to pass the parent widget into the kid's constructor is perfect and makes a clean initialization where the real root is available and we don't need flags or maintain internal state.

I will push a branch soon.

@apotonick

This comment has been minimized.

Show comment Hide comment
@apotonick

apotonick Oct 10, 2011

Owner

Guys, please check out that new branch (WIP): https://github.com/apotonick/apotomo/tree/no-parent-controller

I changed the Widget.new API, the first arg is the parent widget. This way, we can get rid of the stupid ~after_add hook and things get way more simple in the core. The only problem is that it gets a bit more complicated to add a parent-less widget later, but I'm not sure if this is needed in Apotomo at all. Please let me know how this first refactoring works for you. THX

Owner

apotonick commented Oct 10, 2011

Guys, please check out that new branch (WIP): https://github.com/apotonick/apotomo/tree/no-parent-controller

I changed the Widget.new API, the first arg is the parent widget. This way, we can get rid of the stupid ~after_add hook and things get way more simple in the core. The only problem is that it gets a bit more complicated to add a parent-less widget later, but I'm not sure if this is needed in Apotomo at all. Please let me know how this first refactoring works for you. THX

@apotonick

This comment has been minimized.

Show comment Hide comment
@apotonick

apotonick Oct 12, 2011

Owner

FIXED in the 1.2 release: 1212694

Owner

apotonick commented Oct 12, 2011

FIXED in the 1.2 release: 1212694

@apotonick apotonick closed this Oct 12, 2011

@cameel

This comment has been minimized.

Show comment Hide comment
@cameel

cameel Oct 12, 2011

Thanks. Looks good :)

How does widget() work now? I can see in the unit tests that both it and Widget.new can be used. If it calls the constructor, how does it know what the parent is (it does not get the pararent as argument)?

# test/request_processor_test.rb
...
procs = [Proc.new{ |root| 
  root << widget(:mouse, 'mum') 
  KidWidget.new(root['mum'], 'kid')
}]
...

cameel commented Oct 12, 2011

Thanks. Looks good :)

How does widget() work now? I can see in the unit tests that both it and Widget.new can be used. If it calls the constructor, how does it know what the parent is (it does not get the pararent as argument)?

# test/request_processor_test.rb
...
procs = [Proc.new{ |root| 
  root << widget(:mouse, 'mum') 
  KidWidget.new(root['mum'], 'kid')
}]
...
@apotonick

This comment has been minimized.

Show comment Hide comment
@apotonick

apotonick Oct 12, 2011

Owner

Check that: http://nicksda.apotomo.de/2011/10/released-apotomo-1-2/

In short, the << widget(..) syntax now is a DSL construction, what happens internally is a call to the actual constructor.

Owner

apotonick commented Oct 12, 2011

Check that: http://nicksda.apotomo.de/2011/10/released-apotomo-1-2/

In short, the << widget(..) syntax now is a DSL construction, what happens internally is a call to the actual constructor.

@cameel

This comment has been minimized.

Show comment Hide comment
@cameel

cameel Oct 12, 2011

Nice trick to retain backwards compatibility...

cameel commented Oct 12, 2011

Nice trick to retain backwards compatibility...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment