sunaku / wmiirc

Ruby configuration for WMII window manager
ISC License
82 stars 26 forks source link

Moving from TOPLEVEL_BINDING to Button#instance_eval in commit 71bb5a57668ac54c00866c8857e36f2daea09c27 doesn't work #4

Closed nougad closed 14 years ago

nougad commented 14 years ago

Minimal setup:

display:
  status:
    - test:
      click: status 'test'

Fails with "wrong number of arguments (1 for 0) (ArgumentError)" because it wants to call Button(Thread)#status not status on TOPLEVEL_BINDING.

If you using status_click instead of status for example the method is found correctly but all instance-variables are nil because they are only defined in TOPLEVEL_BINDING not in Button-scope.

sunaku commented 14 years ago

Thanks for the detailed description. :-)

The solution I have in mind is to use composition, instead of inheritance (as is the case now), to define the Button class:

class Button
  attr_reader :thread
end

The @thread will only be used for its true purpose: scheduling of periodic status button label updates. And the Button class itself will serve as the target of instance_eval(). Since the Button class derives from Object, it will receive the same status() method defined elsewhere in config.rb and we won't have any conflict with the Thread#status method.

What do you think?

nougad commented 14 years ago

Thank you for your fast reply.

Using thread as an instance variable will avoid the name collision with status method. But the instance variables in TOPLEVEL_BINDING (in exampe: @status_button_by_name) aren't accessible anyway.

I thought about an small change to fix this problem but I didn't see a short way for that.

One possibility is to move all methods and instance variables from load_config into an special object and use this as instance variable at Button class. But this will end in an larger modification of config.rb.

sunaku commented 14 years ago

I see, yes, using a common/special object to house all of the settings is a better long-term solution. And I don't like that I used instance variables (really globals) throughout the config.rb file. I feel that this configuration has, over time, become a pile of hacks and is long due for a refactoring.

sunaku commented 14 years ago

I started work on the refactoring, namely in the "import" branch: http://github.com/sunaku/wmiirc/tree/import

The status bar button/applet logic is now well insulated and consistent with the evaluation environment that is provided to Ruby code in other parts of the configuration. See the Wmiirc::Sandbox class for details.

nougad commented 14 years ago

Your changes are looking great. I hadn't time to migrate my own config but I hope I could do this in the next days and give you a better feedback.

sunaku commented 14 years ago

Thanks. I suggest holding off on upgrading to my new "import" branch until I officially announce it on the wmii mailing list. It is not stable right now.

sunaku commented 14 years ago

Okay, I've announced it here: http://thread.gmane.org/gmane.comp.misc.suckless/551

Proceed with the upgrade. :-)

sunaku commented 14 years ago

The global status("foo") method has been removed. Use Wmiirc::Status["foo"].refresh instead.