Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Nsnotify support for OS X 10.8. #311

Merged
merged 8 commits into from Aug 3, 2012

Conversation

Projects
None yet
7 participants
Contributor

foxycoder commented Jul 30, 2012

As requested in #307.

Let me know if there's anything missing or wrong.

This pull request fails (merged 8ba44a0 into 1a75958).

This pull request passes (merged ecdd50d into 1a75958).

@rymai rymai commented on an outdated diff Jul 30, 2012

lib/guard/notifier.rb
@@ -156,7 +158,7 @@ def notify(message, options = { })
#
def auto_detect_notification
# This is a duplication of `NOTIFIER.keys`, but keeps the preferred order on Ruby 1.8.7 also
- available = [:growl_notify, :gntp, :growl, :libnotify, :notifysend, :notifu, :emacs].any? { |notifier| add_notification(notifier, { }, true) }
+ available = [:nsnotify, :growl_notify, :gntp, :growl, :libnotify, :notifysend, :notifu, :emacs].any? { |notifier| add_notification(notifier, { }, true) }
@rymai

rymai Jul 30, 2012

Owner

You should keep the preferred order used at line 56 here.

@rymai rymai commented on an outdated diff Jul 30, 2012

lib/guard/notifiers/nsnotify.rb
@@ -0,0 +1,48 @@
+module Guard
+ module Notifier
+
+ # System notifications using the [nsnotify](https://github.com/foxycoder/nsnotify) gem.
+ #
+ # This gem is available for OS X 10.8 Mountain Lion and sends notifications to the OS X
+ # notification center.
+ #
+ # @example Add the `nsnotify_guard` gem to your `Gemfile`
@rymai

rymai Jul 30, 2012

Owner

Should be nsnotify instead of nsnotify_guard (and also at line 11 & 18).

Owner

rymai commented Jul 30, 2012

Thanks very much, that's awesome! Is there any chance to have the image in the notification soon? :)

This pull request fails (merged eb6fdd5 into 1a75958).

This pull request passes (merged eb3bc28 into 1a75958).

Contributor

foxycoder commented Jul 30, 2012

Thanks for the feedback and looking into it.

Changing the image is harder than I expected: I tried symlinking the icns file and even copying the entire .app bundle but the icons remains cached in OS X. Which means that the icon will stay the same once the app is called, no matter what you do :)

All I can think of is asking @alloy whether he can make a bunch of clones of his terminal-notifier, for each status (success, error, pending, etc.) and each with an icon matching it's status.. I would do it myself, but I don't have an Appl€ developer account.

alloy commented Jul 30, 2012

What does the nsnotify gem do over the terminal-notifier gem?

Changing the image is harder than I expected: I tried symlinking the icns file and even copying the entire .app bundle but the icons remains cached in OS X. Which means that the icon will stay the same once the app is called, no matter what you do :)

All I can think of is asking @alloy whether he can make a bunch of clones of his terminal-notifier, for each status (success, error, pending, etc.) and each with an icon matching it's status.. I would do it myself, but I don't have an Appl€ developer account.

You have to change the bundle identifier of the app for the correct (not cached) image to be used. But this is not how NotificationCenter is supposed to be used, you should have one notification look per app.

For that reason and that I don’t want to be responsible for builds of every version for everyone that wants a custom icon, I regret to say that I won’t do this.

Contributor

foxycoder commented Jul 30, 2012

What does the nsnotify gem do over the terminal-notifier gem?

Why, nothing! I didn't know about your gem! :) In fact, I'd be happy to change the pull to use your gem if that's what you want.

For that reason and that I don’t want to be responsible for builds of every version for everyone that wants a custom icon, I regret to say that I won’t do this.

I can imagine that. It was just a thought.

Contributor

foxycoder commented Jul 30, 2012

What does the nsnotify gem do over the terminal-notifier gem?
Why, nothing! I didn't know about your gem! :) In fact, I'd be happy to change the pull to use your gem if that's what you want.

Ah I see, your gem is newer than mine, that's the reason I could not find it.

This pull request fails (merged 78ccf16 into 1a75958).

Contributor

foxycoder commented Jul 30, 2012

Alright, I altered the implementation to use @alloy's gem instead of mine. Thanks and kudo's to you from Guard and of course Alloy for making the app and the gem.

Owner

rymai commented Jul 30, 2012

Thanks @foxycoder, @netzpirat I'll let you merge this since the Notifier is your "baby"! ;)

alloy commented Jul 30, 2012

Awesome :)

This pull request fails (merged ac293c2 into 1a75958).

This pull request fails (merged ef11bf1 into 1a75958).

This pull request passes (merged 89ddc26 into 1a75958).

Contributor

foxycoder commented Jul 31, 2012

I was thinking of adding another feature: passing options on_error and on_success to execute some command. This way I can e.g. play a sound on errors, by passing options in my Guardfile like so:

notification :terminal_notifier, :app_name => "MyApp ::", :on_error => '/usr/bin/afplay /System/Library/Sounds/Basso.aiff', :activate => 'com.googlecode.iTerm2'

This plays the Basso sound whenever a test fails and compensates for the fact that we can't change the app icon imo. What do you guys think?

BTW: The activate option make sure iTerm2 gets focus when I click the notification, rather than the default Terminal app.

alloy commented Jul 31, 2012

@foxycoder The command is executed when you click the notification, by that time you probably already know the status, no? I.e. playing an extra sound to indicate the status might not be that useful. Anyways, I’m just thinking out loud :)

Contributor

foxycoder commented Jul 31, 2012

@alloy Yeah sorry I was not clear. I want to run the command from the notify method in Ruby, rather than pass it on to the terminal notifier app for the reason you just mentioned.

Contributor

netzpirat commented Aug 1, 2012

This is great! I just returned from holidays and want to get started with this, and boom, here it is already. I'm currently updating my hardware zoo to Mountain Lion and will merge and test it tomorrow. Thanks a lot.

Owner

thibaudgg commented Aug 2, 2012

What about creating a guard-ncnotifier gem with the Guard icon (maybe 3 with different icon apps, success (red), failure (green), normal) ?

Contributor

foxycoder commented Aug 2, 2012

Yeah that'd be nice. In the meantime, I got my hands on an Apple dev account from work, so I could make those if needed. Unless @alloy reconsiders and wants to do it himself ;)

Let me know how I can help.

@ghost ghost assigned netzpirat Aug 3, 2012

@netzpirat netzpirat merged commit 89ddc26 into guard:master Aug 3, 2012

1 check passed

default
Details
Contributor

netzpirat commented Aug 3, 2012

Thanks a lot @foxycoder, works great! I just changed some code style and added complete YARDOC to the notifier, so that all notifiers looks similar.

We should make new pull requests for a guard-terminal-notifier gem or :on_error or :on_success callbacks.

@foxycoder foxycoder referenced this pull request Aug 14, 2012

Merged

Terminal notifier guard #317

Coverage Status

Changes Unknown when pulling 89ddc26 on foxycoder:nsnotify into * on guard:master*.

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