Skip to content

Conversation

@Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Oct 3, 2020

Please don't merge this yet.

This PR adds the deprecated macOS notification API again, so that notifications work when compiling from source without code signing. It was removed in 4e3c6e5.

What did you mean by "compile time version detection" in #2876 (comment)? Simply check if the macOS version is 10.15 (Catalina) or lower and then use the old API? But I think it would be useful to be able to use this API as long as it's still only deprecated and not removed. Do you have an idea, how this could be done best?

@kovidgoyal
Copy link
Owner

I dont see the point in relying on an API we know is going to be
removed. I was mainly talking about your issue of not being able to
compile on older macOS. So at compile time detect that the new API is
not available and if so, compile against the old one. Probably can do so
with Availability.h macros.

@Luflosi
Copy link
Contributor Author

Luflosi commented Oct 3, 2020

Ok, I'm fine with not having this in Big Sur. Is it possible to query MAC_OS_X_VERSION_MAX_ALLOWED from setup.py? I need to do this to not add the UserNotifications framework to platform_libs.

@kovidgoyal
Copy link
Owner

You could just try to compile a small dynamically generated .m file and link with th framework, if it succeeds, use it, if not, use the older API. This is how configure traditionally works.

@Luflosi Luflosi force-pushed the re-add-deprecated-macos-notifications branch from 89cccda to 13d7478 Compare October 4, 2020 11:21
@Luflosi Luflosi marked this pull request as ready for review October 4, 2020 11:21
@Luflosi Luflosi force-pushed the re-add-deprecated-macos-notifications branch 2 times, most recently from 6eab3ef to 4d57d53 Compare October 4, 2020 11:28
The new User Notifications Framework is only available on macOS 10.14 and above, while the old NSUserNotification API is deprecated in macOS 11 (Big Sur) and will probably be removed in the future.
This commit compiles a simple test program to see if the Framework is available and then uses either the new or the old API.
@Luflosi Luflosi force-pushed the re-add-deprecated-macos-notifications branch from 4d57d53 to 0ae1f99 Compare October 4, 2020 11:32
@Luflosi
Copy link
Contributor Author

Luflosi commented Oct 4, 2020

Please review.
I changed test_compile() to pipe stdout and stderr to /dev/null, so the error message is not visible. Does this cause any problems?

@kovidgoyal kovidgoyal merged commit 225e52b into kovidgoyal:master Oct 4, 2020
@Luflosi Luflosi deleted the re-add-deprecated-macos-notifications branch October 4, 2020 12:47
@Luflosi
Copy link
Contributor Author

Luflosi commented Oct 4, 2020

These were all the commits I wanted to add before the next release. Thank you for waiting a couple days with the release, it will make it much easier for me to update the kitty package for nix. You may now release the next version if you like.

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.

2 participants