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

Notification Center #1132

Closed
wants to merge 6 commits into from
Closed

Notification Center #1132

wants to merge 6 commits into from

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Sep 21, 2012

This allows users to send Quicksilver's user notifications through Apple's new Notification Center, as discussed in #867.

Some decisions had to be made. Here are some comments on that:

  • I invented a bundle ID for Notification Center. After my change in 325651c, we could just use "Notifications" as the identifier, since such an icon already exists in /System/Library/CoreServices/CoreTypes.bundle/Contents/Resources, but it just isn't that descriptive. Note that since the bundle ID isn't real, I had to define said icon's location in ResourceLocations.plist.
  • To prevent the new Notifier from appearing on 10.7 or older, there's a test in QSHelpersPrefPane.m. Ugly, but it works. The far more complicated alternative would be to implement NC as an internal plug-in and prevent it from loading on older OSes.
  • We talked about making NC the default, but if we do that (in QSDefaults.plist), what will happen for 10.7 users? We could add more ugly conditional code to choose different defaults for different OSes.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 25, 2012

I think it would be good to have NC as default for 10.8, definitely something to tout as a 'new' feature.
I'd be prepared to use an ugly hack to choose between the OSes with an appropriate macro for when we drop 10.7 and a nice comment :)

Of course, I can't test the notification stuff but I can say these things:

  • Things look right on 10.7. No mention of NC anywhere :)
  • This won't build for anybody who doesn't have the 10.8 SDK. We may have to wrap the both the QSNotificationCenterNotofier files in a #ifdef so that it's just 'blank' for anybody who doesn't have the 10.8 SDK.
  • We're still not set on the right format for the drop10.7 comment/macro/warning, just flagging it for when we do decide :)
  • I had no merge conflict

Once we sort out the platform compatibility issues, this'll be good to go

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 27, 2012

OK, I've put in an ugly hack as requested. Given what we need to do here, I'm not sure how it could be accomplished with macros and #ifdef, so I used the comment “tag” again. :-(

To test, defaults delete com.blacktree.Quicksilver QSNotifiers and see what it gets set to when you launch QS. I originally tried the opposite (leaving the plist as is and setting the new bundle ID for 10.8 only, instead of changing the plist and setting the old bundle ID if not 10.8 like it is now). When I did that, notifications would be correctly sent through NC, but if you check the prefs, it was still referring to the default in the plist. So, I'm curious how this will shake out on 10.7. I'm sure it will use the correct notifier, but I wonder what it will show in the prefs since the default won't exist in that case.

Let me know.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 27, 2012

Oh, and for the merge conflicts here, just “choose both” changes.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 4, 2012

Merged

@pjrobertson pjrobertson closed this Oct 4, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants