MacGapProject / MacGap1

Desktop WebKit wrapper for HTML/CSS/JS applications.
Other
3.55k stars 208 forks source link

Extended notification support #87

Closed JLarky closed 10 years ago

JLarky commented 10 years ago

This commits give ability to do few new things with native notifications: disable sound, close notifications and listening for activate event (to simulate onclick event on Chrome`s notifications).

JLarky commented 10 years ago

Is there something I can do with this branch to make it more pull-requestable? :)

jeff-h commented 10 years ago

I notice you had to add something to the JSEventHelper class to allow you to pass info back to JavaScript with your event. I'm interested in your thoughts on my pull request at https://github.com/maccman/macgap/pull/97 which adds the ability to pass an NSDictionary back with the event.

It would obviously work with a slight refactor of your code here, but the downside is that in a case like yours here where we only need to pass a single value back, that value is redundantly nested under a parent property representing the NSDictionary (I called it data) i.e. on the JavaScript side of things, you will need to access e.data.yourValue. rather than e.yourValue.

What are your thoughts on this? Is the nesting OK? Or should we have a JSEventHelper method which handles a single pass-back value (i.e. what your commit already has) PLUS my commit for times when you want to pass back a whole object?

JLarky commented 10 years ago

I don't have strong opinion either way. I used one parameter because I needed it only for string id, and it's possible to pass json string if you want to pass object.

Thing that I think I did right is to use CustomEvent and detail which is standardized, and you use document.createEvent which is deprecated.

jeff-h commented 10 years ago

Let's go with the NSDictionary approach, as that means we don't need to serialise to JSON in numerous places over time. Would you mind re-rolling this to use the NSDict support that I've just committed?

Also if you get a chance, it would be great to update from document.createEvent.

mralexgray commented 10 years ago

+1 is this project active?

jeff-h commented 10 years ago

Yeah, it's definitely alive and well... despite what the activity on this issue might indicate :( Sorry for the slowness rolling this in. I'll try to make sure this gets in over the weekend.

jeff-h commented 10 years ago

So, this is now rolled-in. Thanks @JLarky for your efforts on this.

However, I found it hard to test this; the first time it compiled, I received an OS X authenticate-as-admin from Xcode, and the MacGap app fired off a test notification. However on subsequent compiles it never makes any notifications at all for me. @JLarky — are you able to add something to the discussion about this at https://github.com/maccman/macgap/issues/57 ?

mralexgray commented 10 years ago

This is the reasonI ended up looking at this thread... If you'll look, I'll bet that if you scroll down in your NC-proper (the big flop-out thing on the right)...all your Notifications will be there... just only the FIRST shows on the desktop, growl-style... I was only able to make it show up again by either renaming the target... or some other un-sexy means I don't really recall... please advise how / if can help resolve this...it was driving me nuts!

140526-0001

jeff-h commented 10 years ago

@mralexgray — thanks for further clarifying what's happening. Check out what @JLarky has added to the discussion over on https://github.com/maccman/macgap/issues/57 — he's explained exactly why this is the correct behaviour.