Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch out hs.notify for asmagill's notification #44

Closed
asmagill opened this issue Oct 17, 2014 · 14 comments
Closed

switch out hs.notify for asmagill's notification #44

asmagill opened this issue Oct 17, 2014 · 14 comments

Comments

@asmagill
Copy link
Member

I would recommend using mjolnir._asm.ui.notification (https://github.com/asmagill/mjolnir_asm.ui) over mjolnir._asm.notify -- it's backwards compatible and offers more features (and it's primed to add scheduled (dated) notifications to it at some-point as well)... I can do this come the weekend if you'd like.

@cmsj
Copy link
Member

cmsj commented Oct 17, 2014

I did start with your notification module, but agreed with Habbie to drop back to the notify module because the newer one seems to require 10.10 (thus sparking the Issue about module OS versioning ;)

@asmagill
Copy link
Member Author

I'll look closer... I thought about using some of the 10.10 only features, but I thought I finally decided against it... maybe I'm remembering wrong...

On Oct 17, 2014, at 5:54 AM, Chris Jones notifications@github.com wrote:

I did start with your notification module, but agreed with Habbie to drop back to the notify module because the newer one seems to require 10.10 (thus sparking the Issue about module OS versioning ;)


Reply to this email directly or view it on GitHub #44 (comment).

@asmagill
Copy link
Member Author

I'm adding a 10.8 virtual machine so I can test stuff against it as well as 10.10... looking at the notification code, I think the only 10.9 and 10.10 stuff currently not commented out is a couple of activation types that we can't even request with the module... I'll remove those as well. If I can confirm it works in 10.8 (the first that I believe supported ANY notification center), I'll put this in as a replacement and generate a new pull request. I've been thinking that if I do decide to add the 10.10 stuff, it would be in a separate module that "adds to" this one, so it will be clearly marked and optional.

@Habbie
Copy link
Contributor

Habbie commented Oct 17, 2014

❤️

@asmagill
Copy link
Member Author

Hmm... the precompiled version of HS doesn't run under 10.8 (require 10.9+). FWIW, Mjolnir does.

So... how far back do we want to go? I personally don't think we should go back further than 10.8, but I can argue 10.8 both ways... definitely 10.9+

@cmsj
Copy link
Member

cmsj commented Oct 18, 2014

The decision to set the minimum at 10.9 was pretty arbitrary. I'm not opposed to setting it to 10.8 if it doesn't break anything.

@cmsj cmsj changed the title hs.notify module suggestion: switch out hs.notify for asmagill's notification Oct 18, 2014
@cmsj cmsj changed the title module suggestion: switch out hs.notify for asmagill's notification switch out hs.notify for asmagill's notification Oct 18, 2014
@asmagill
Copy link
Member Author

I don't know what the time frame is on getting 1.0 out, but I'm having problems with notification... Under some conditions, it crashes when sending a new notification after a restart. I think I know what the problem is, but the fix is non-trivial, and I want to rewrite portions of the code anyways to make it cleaner and easier to extend later... I have confirmed that notify does not have this problem, so if this ends up being a hold out to release, then go with notify for now. I'll update this when I'm ready to submit notification for testing.

I also plan on keeping notification able to functionally drop in and replace notify, so it will provide new functionality without breaking backwards compatibility if notify does get released first.

@cmsj
Copy link
Member

cmsj commented Nov 3, 2014

FWIW, there is no timeframe for 1.0. It'll happen when we're ready for it. There's still a fair amount of documentation/tutorial stuff to write. I don't think the full notification extension is a blocker for 1.0, but it would certainly be nice to have.

@cmsj
Copy link
Member

cmsj commented Jan 10, 2015

@asmagil how close to landable would you say this module is? We've got at least one crashed bug in hs.notify, and looking at the code I'm not at all happy with the module, so I'm wondering if we can replace it with less effort than fixing it!

@asmagill
Copy link
Member Author

I'll take a look at it this weekend and let you know... iirc the problem was with inconsistent garbage collection, which I understand better now, so hopefully it won't be too difficult to finish.

On Jan 10, 2015, at 5:49 PM, Chris Jones notifications@github.com wrote:

@asmagil how close to landable would you say this module is? We've got at least one crashed bug in hs.notify, and looking at the code I'm not at all happy with the module, so I'm wondering if we can replace it with less effort than fixing it!


Reply to this email directly or view it on GitHub #44 (comment).

@asmagill
Copy link
Member Author

Ok, I almost have a version of it ready for deploying for testing -- hope to have it ready tonight. I haven't looked at the github site yet, but is there a specific issue entered which details the hs.notify crash you speak of below? I'd like to see if this suffers from a similar issue before putting it out there.

Also not that this does not yet support scheduled notifications (i.e. those that you want sent at a specific time) or allow you to override the banner/alert style set in the Notification System Preferences panel; I plan to add those as a separate and probably third-party module or 2, as they are either 10.10 specific or use undocumented API's or both.

On Jan 10, 2015, at 5:49 PM, Chris Jones notifications@github.com wrote:

@asmagil how close to landable would you say this module is? We've got at least one crashed bug in hs.notify, and looking at the code I'm not at all happy with the module, so I'm wondering if we can replace it with less effort than fixing it!


Reply to this email directly or view it on GitHub #44 (comment).

@asmagill
Copy link
Member Author

Ok, I think it's ready for public testing/perusal/nit-picking... still need to convert to the more expansive doc style, but that can wait for now...

It's at https://github.com/asmagill/hammerspoon_asm/tree/experiemental https://github.com/asmagill/hammerspoon_asm/tree/experiemental, if you want to take a look at it.

I'm more than happy to merge a hammerspoon only version into the core and issue a pull request, but what name should I give it? I'm leaning towards installing it as hs.notification, leaving hs.notify alone for now... after it's been reviewed and tested a bit by others, we can either drop hs.notify, or replace hs.notify with the portions of hs.notification that apply and have hs.notify require hs.notification. But I'll take your lead and issue the pull then.

On Jan 10, 2015, at 5:49 PM, Chris Jones notifications@github.com wrote:

@asmagil how close to landable would you say this module is? We've got at least one crashed bug in hs.notify, and looking at the code I'm not at all happy with the module, so I'm wondering if we can replace it with less effort than fixing it!


Reply to this email directly or view it on GitHub #44 (comment).

@cmsj
Copy link
Member

cmsj commented Jan 14, 2015

Given that the extension provides backwards compatibility, I think we should kick the tyres a bit and then land it as a replacement for hs.notify. If we land it as hs.notification and then migrate it to hs.notify later, we'll confuse people and probably end up carrying a compatibility shim, which will be confusing for new users.

@cmsj
Copy link
Member

cmsj commented Jan 16, 2015

This is now merged into 0.9.17. Thanks!

@cmsj cmsj closed this as completed Jan 16, 2015
latenitefilms added a commit to latenitefilms/hammerspoon that referenced this issue Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants