Notifier Tmux: bg color -- save and restore tmux state #372

Merged
merged 2 commits into from Dec 21, 2012

Conversation

Projects
None yet
4 participants
@rudicode
Contributor

rudicode commented Dec 11, 2012

Here is the work in progress pull request, as suggested by @netzpirat
I hope it's understandable.

@rudicode

This comment has been minimized.

Show comment Hide comment
@rudicode

rudicode Dec 11, 2012

Contributor

I'm not sure how to send the notifier options hash in the loops for Notifier#turn_on and Notifier#turn_off
Right now they don't send anything, but the methods I wrote in ::Tmux#turn_on and ::Tmux#turn_off, do accept a hash of options.

Contributor

rudicode commented Dec 11, 2012

I'm not sure how to send the notifier options hash in the loops for Notifier#turn_on and Notifier#turn_off
Right now they don't send anything, but the methods I wrote in ::Tmux#turn_on and ::Tmux#turn_off, do accept a hash of options.

@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Dec 11, 2012

Contributor

That's a nice start!

Each notifier module has its own options, which are stored when the notification is added. So you can grab it from self.notifications:

# Turn notifications off.
#
def turn_off
  notifications.each do |notification|
    notifier = get_notifier_module(notification[:name])
    notifier.turn_off(notification[:options]) if notifier.respond_to?(:turn_off)
  end
  ENV['GUARD_NOTIFY'] = 'false'
end
Contributor

netzpirat commented Dec 11, 2012

That's a nice start!

Each notifier module has its own options, which are stored when the notification is added. So you can grab it from self.notifications:

# Turn notifications off.
#
def turn_off
  notifications.each do |notification|
    notifier = get_notifier_module(notification[:name])
    notifier.turn_off(notification[:options]) if notifier.respond_to?(:turn_off)
  end
  ENV['GUARD_NOTIFY'] = 'false'
end
@rudicode

This comment has been minimized.

Show comment Hide comment
@rudicode

rudicode Dec 12, 2012

Contributor

Ok more work on this.
Changed the logic on this slightly.
Tmux#save_tmux_state now saves the tmux state as it is shown by the tmux command

tmux show

Any options are saved into a hash @@tmux_state
Is this an appropriate way to save the info inside the module? It works, but is there a different solution?

Tmux#restore_tmux_state, loops through the saved options and sets them to what they were or un-sets the option which was created by the Tmux module.

I hope this helps

Contributor

rudicode commented Dec 12, 2012

Ok more work on this.
Changed the logic on this slightly.
Tmux#save_tmux_state now saves the tmux state as it is shown by the tmux command

tmux show

Any options are saved into a hash @@tmux_state
Is this an appropriate way to save the info inside the module? It works, but is there a different solution?

Tmux#restore_tmux_state, loops through the saved options and sets them to what they were or un-sets the option which was created by the Tmux module.

I hope this helps

@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Dec 12, 2012

Contributor

Each notifier module extends itself

module GNTP
  extend self
end

which is basically the same as opening the eigenclass

module GNTP
  class << self
  end
end

You do not need to to have a class variable, a simple instance variable like @tmux_state is fine.

Contributor

netzpirat commented Dec 12, 2012

Each notifier module extends itself

module GNTP
  extend self
end

which is basically the same as opening the eigenclass

module GNTP
  class << self
  end
end

You do not need to to have a class variable, a simple instance variable like @tmux_state is fine.

@rudicode

This comment has been minimized.

Show comment Hide comment
@rudicode

rudicode Dec 12, 2012

Contributor

Thanks , I'll update the code to reflect that.

Contributor

rudicode commented Dec 12, 2012

Thanks , I'll update the code to reflect that.

@rudicode

This comment has been minimized.

Show comment Hide comment
@rudicode

rudicode Dec 13, 2012

Contributor

I need to write some specs for this.

Contributor

rudicode commented Dec 13, 2012

I need to write some specs for this.

@rudicode

This comment has been minimized.

Show comment Hide comment
@rudicode

rudicode Dec 13, 2012

Contributor

Ok I added specs for the save_tmux_state and restore_tmux_state, methods.
This is working good on my work and home machines. But I'd like to get some input from anybody using this. :-)

Contributor

rudicode commented Dec 13, 2012

Ok I added specs for the save_tmux_state and restore_tmux_state, methods.
This is working good on my work and home machines. But I'd like to get some input from anybody using this. :-)

@cpjolicoeur

This comment has been minimized.

Show comment Hide comment
@cpjolicoeur

cpjolicoeur Dec 18, 2012

Member

This is greatly needed for Tmux users has having our bg and fg settings overwritten completely outside of the guard processes is annoying at best.

Member

cpjolicoeur commented Dec 18, 2012

This is greatly needed for Tmux users has having our bg and fg settings overwritten completely outside of the guard processes is annoying at best.

@rudicode

This comment has been minimized.

Show comment Hide comment
@rudicode

rudicode Dec 18, 2012

Contributor

@cpjolicoeur would you mind testing my branch of guard. It's available here https://github.com/rudicode/guard/tree/tmux-bg-color

I'm using it in one of my projects now. But I put it in my gem file like this
gem 'guard', :git => "https://github.com/rudicode/guard.git", :branch => "tmux-bg-color"

but for some reason can only run it using:
bundle exec guard
I think it has something to do with bundler handling git repos.

Before running it for the first time make sure your tmux settings are they way you want them.
Anything displayed with tmux show will be saved and then restored on exit. Any extra settings made by the notifier will be unset.

Contributor

rudicode commented Dec 18, 2012

@cpjolicoeur would you mind testing my branch of guard. It's available here https://github.com/rudicode/guard/tree/tmux-bg-color

I'm using it in one of my projects now. But I put it in my gem file like this
gem 'guard', :git => "https://github.com/rudicode/guard.git", :branch => "tmux-bg-color"

but for some reason can only run it using:
bundle exec guard
I think it has something to do with bundler handling git repos.

Before running it for the first time make sure your tmux settings are they way you want them.
Anything displayed with tmux show will be saved and then restored on exit. Any extra settings made by the notifier will be unset.

@netzpirat

View changes

lib/guard/notifiers/tmux.rb
+
+ def turn_on(options = { })
+ save_tmux_state
+ system(" #{ DEFAULTS[:client] } set quiet on") # quiets tmux output

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Dec 19, 2012

Contributor

In Guard we have all methods and constants documented, so we have superior API docs on rubydoc.info. The inline comment should be moved to the method comment. We also have always an empty comment line before the method:

# Quiets tmux output
#
def turn_on(options = { })
@netzpirat

netzpirat Dec 19, 2012

Contributor

In Guard we have all methods and constants documented, so we have superior API docs on rubydoc.info. The inline comment should be moved to the method comment. We also have always an empty comment line before the method:

# Quiets tmux output
#
def turn_on(options = { })
+ restore_tmux_state
+ end
+
+ # Saves current tmux options that will be restored when notifications are turned off.

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Dec 19, 2012

Contributor

Same here.

@netzpirat

netzpirat Dec 19, 2012

Contributor

Same here.

@netzpirat

View changes

lib/guard/notifiers/tmux.rb
+ 'status-left-fg' => nil,
+ 'status-right-fg' => nil,
+ 'message-bg' => nil,
+ 'message-fg' => nil

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Dec 19, 2012

Contributor

The unnecessary space can be removed, but aligning them is fine.

@netzpirat

netzpirat Dec 19, 2012

Contributor

The unnecessary space can be removed, but aligning them is fine.

@netzpirat

View changes

lib/guard/notifiers/tmux.rb
+ @tmux_state[option]
+ end
+
+ def ready_to_restore

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Dec 19, 2012

Contributor

This method can be replaced by a simple attribute accessor attr_accessor.

@netzpirat

netzpirat Dec 19, 2012

Contributor

This method can be replaced by a simple attribute accessor attr_accessor.

@rudicode

This comment has been minimized.

Show comment Hide comment
@rudicode

rudicode Dec 20, 2012

Contributor

Thanks for the feedback, I'll start on the changes.

Contributor

rudicode commented Dec 20, 2012

Thanks for the feedback, I'll start on the changes.

@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Dec 20, 2012

Contributor

Can you also merge the upstream changes? Would be nice to have it also in the release at the end of the week.

Contributor

netzpirat commented Dec 20, 2012

Can you also merge the upstream changes? Would be nice to have it also in the release at the end of the week.

@rudicode

This comment has been minimized.

Show comment Hide comment
@rudicode

rudicode Dec 20, 2012

Contributor

Sure what would be the proper way to merge the upstream changes into my branch? I'm a little new to this, and don't want to make a mistake. Should I:
git fetch
git rebase upstream/master

Contributor

rudicode commented Dec 20, 2012

Sure what would be the proper way to merge the upstream changes into my branch? I'm a little new to this, and don't want to make a mistake. Should I:
git fetch
git rebase upstream/master

@rymai

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Dec 20, 2012

Owner

Yeah it should work, and then you could squash your commits into one using the following method: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

and force push the squashed commit.

Owner

rymai commented Dec 20, 2012

Yeah it should work, and then you could squash your commits into one using the following method: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

and force push the squashed commit.

@rudicode

This comment has been minimized.

Show comment Hide comment
@rudicode

rudicode Dec 20, 2012

Contributor

Having a small issue, on exit guard does not call ::Guard::Notifier.turn_off , I added it to lib\guard.rb
This works, but i don't think it's the appropriate place.
Any ideas?

Contributor

rudicode commented Dec 20, 2012

Having a small issue, on exit guard does not call ::Guard::Notifier.turn_off , I added it to lib\guard.rb
This works, but i don't think it's the appropriate place.
Any ideas?

@netzpirat netzpirat merged commit 8900a42 into guard:master Dec 21, 2012

1 check passed

default The Travis build passed
Details
@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Dec 21, 2012

Contributor

I've merged it and made some refactoring:

The biggest change was to inline the save_tmux_state and restore_tmux_state directly into its correlating on/off methods, but extracted the internal state handling into a separate method reset_options_store. I also removed accessors to the internal state, which was only used for accessing within the specs. The idea of BDD (testing in general) is not to rely on internal state and only describe the behavior (input/output) of the system from the outside. You can see this good on the rewriting of the specs (because of the removed internal accessors).

The turn off of the notifier was fine, I just added the missing spec and the execution order when Guard stops.

All the rest is adding missing specs, documentation and code style changes (also from previous merges of other pull request).

Thanks a lot for this pull request!

Contributor

netzpirat commented Dec 21, 2012

I've merged it and made some refactoring:

The biggest change was to inline the save_tmux_state and restore_tmux_state directly into its correlating on/off methods, but extracted the internal state handling into a separate method reset_options_store. I also removed accessors to the internal state, which was only used for accessing within the specs. The idea of BDD (testing in general) is not to rely on internal state and only describe the behavior (input/output) of the system from the outside. You can see this good on the rewriting of the specs (because of the removed internal accessors).

The turn off of the notifier was fine, I just added the missing spec and the execution order when Guard stops.

All the rest is adding missing specs, documentation and code style changes (also from previous merges of other pull request).

Thanks a lot for this pull request!

@rudicode

This comment has been minimized.

Show comment Hide comment
@rudicode

rudicode Dec 22, 2012

Contributor

Wow lots of changes to grok, over the weekend. Thanks for the help along the way.

Contributor

rudicode commented Dec 22, 2012

Wow lots of changes to grok, over the weekend. Thanks for the help along the way.

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