Yee Cheng Chin

unread,

Oct 4, 2022, 3:18:09 AM10/4/22

to vim/vim, Subscribed

This finally adds +sound support for Vim on macOS. In macOS, there are quite a few audio libraries to choose from. In the end, I decided to go with AppKit which is the user-level library which we already use for interfacing with the system clipboard. That's because the NSSound class from it is the easiest to use and essentially plug-and-play (with the caveat of needing a run loop as described below). Other libraries like CoreAudio are much too powerful and overkill for Vim's use case of just needing to play a file and be done with it, and they also involve a bit more code to set up and use.

One caveat of using NSSound is that it's an Objective C class so I had to do the implementation in os_macosx.m, and call that to/from sound.c. In order for the sound callback to work, I also had to set up a run loop that's ticked periodically in order to make sure the delegate will get invoked whenever the sound finishes playing. The run loop is ticked the same time as when we would have called invoke_sound_callback() in the Linux / Canberra implementation.

For sound event names, I'm using the builtin NSSound's soundNamed function which can locate the set of default installed sound files (e.g. "Tink", "Sosumi", "Submarine", "Bottle", etc). I was looking up how to get the more specific sounds (e.g. the "empty trash can" sound), and I think NSSound doesn't know how to load them (I think Core Audio has functions to load them but I don't think that's worth the trouble of using Core Audio). Looking up from
https://macreports.com/how-to-change-system-sounds-empty-trash-screenshot-sent-message-etc-on-mac/, it seems like those files can be located under
/System/Library/Components/CoreAudio.component/Contents/SharedSupport/SystemSounds/, but I didn't feel like adding special code to map those files to event names. Interested plugin authors or users can simply hard-code those path names on macOS.

You can view, comment on, or merge this pull request online at:

https://github.com/vim/vim/pull/11274

Commit Summary

8483152 Add sound support for macOS

File Changes

(14 files)

Patch Links:

—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.

lgtm-com[bot]

unread,

Oct 4, 2022, 3:45:26 AM10/4/22

to vim/vim, Subscribed

This pull request introduces 1 alert when merging 8483152 into ec32c78 - view on LGTM.com

new alerts:

1 for Local variable hides global variable

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 4, 2022, 4:04:47 AM10/4/22

to vim/vim, Push

@ychin pushed 1 commit.

9957984 Fix LGTM warning about local var masking global one

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

codecov[bot]

unread,

Oct 4, 2022, 4:53:47 AM10/4/22

to vim/vim, Subscribed

Codecov Report

Merging #11274 (9957984) into master (ec32c78) will decrease coverage by 82.22%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@

##           master   #11274       +/-   ##

===========================================

- Coverage   82.52%    0.29%   -82.23%     

===========================================

  Files         152      152               

  Lines      179092   174819     -4273     

  Branches    40650    40226      -424     

===========================================

- Hits       147790      520   -147270     

- Misses      19067   174242   +155175     

+ Partials    12235       57    -12178

Flag	Coverage Δ
huge-clang-none	`?`
huge-gcc-none	`?`
huge-gcc-testgui	`?`
huge-gcc-unittests	`0.29% <0.00%> (ø)`
linux	`0.29% <0.00%> (-82.23%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/getchar.c	`0.00% <ø> (-85.69%)`	⬇️
src/os_unix.c	`0.00% <ø> (-66.88%)`	⬇️
src/sound.c	`0.00% <0.00%> (-84.62%)`	⬇️
src/ui.c	`0.00% <ø> (-73.93%)`	⬇️
src/float.c	`0.00% <0.00%> (-98.42%)`	⬇️
src/sha256.c	`0.00% <0.00%> (-94.90%)`	⬇️
src/gui_gtk_f.c	`0.00% <0.00%> (-94.72%)`	⬇️
src/arabic.c	`0.00% <0.00%> (-94.57%)`	⬇️
src/crypt_zip.c	`0.00% <0.00%> (-94.12%)`	⬇️
src/testing.c	`0.00% <0.00%> (-93.94%)`	⬇️
... and 140 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 4, 2022, 9:35:17 AM10/4/22

to vim/vim, Push

@ychin pushed 1 commit.

75ca725 Use @autoreleasepool instead of manual NSAutoReleasePool objects

—
View it on GitHub or unsubscribe.

You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 4, 2022, 9:37:27 AM10/4/22

to vim/vim, Subscribed

I'm not sure why testing is reporting coverage drops. Looking through it, it appears that it doesn't use Mac testing as part of the consideration? The new sound logic should still be tested by the existing sound unit test functionality.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Bram Moolenaar

unread,

Oct 4, 2022, 9:48:31 AM10/4/22

to vim/vim, Subscribed

Ignore coverage reports on a PR, they are not reliable.

Have you tried playing the killer sheep game? https://github.com/vim/killersheep

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 4, 2022, 10:05:41 AM10/4/22

to vim/vim, Subscribed

Oh that's a good point. Let me try it out. I just booted it and it's not playing sound. Let me take a quick look why.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 4, 2022, 10:20:16 AM10/4/22

to vim/vim, Subscribed

Ok an update. I think KillerSheep incorrectly calculated the g:killersheep_sound_ext value to be "ogg" (Apple doesn't support ogg), and after making the fix to "mp3" it worked! I did manage to kill some poor innocent 🐑 after the fix.

The change to your plugin is as follows:

diff --git a/autoload/killersheep.vim b/autoload/killersheep.vim

index 276c7d7..51f61a2 100644

--- a/autoload/killersheep.vim

+++ b/autoload/killersheep.vim

@@ -158,7 +158,7 @@ func s:Init()

   hi def KillerLevelX ctermbg=yellow guibg=yellow

 

   if !exists('g:killersheep_sound_ext')

-    if has('win32') || len(s:sound_cmd)

+    if has('win32') || has('osx') || len(s:sound_cmd)

       " most systems can play MP3 files

       let g:killersheep_sound_ext = ".mp3"

     else

Also interesting how you are using the command-line tool afplay and spawning it using job_start() to get it working on macOS before. I did wonder about how much difference it makes when implementing this patch as a dedicated user could already use that (albeit a little more clunky than a native API).

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Dominique Pellé

unread,

Oct 4, 2022, 10:31:50 AM10/4/22

to vim/vim, Subscribed

This is strange. On my macOS M1 laptop and without this PR, using vim-9.0.656):

:version shows -sound (so no sound)
running make test_sound skips sound tests (since there is no sound)
yet running :KillKillKill I do get the sound?!

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 4, 2022, 10:55:41 AM10/4/22

to vim/vim, Subscribed

That's what I was talking about. The plug-in used the command-line tool afplay to play audio before since there was no native sound support on Mac. Ultimately it still works albeit needing to spawn another process for each sound clip.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Bram Moolenaar

unread,

Oct 4, 2022, 12:35:38 PM10/4/22

to vim/vim, Subscribed

> Ok an update. I think KillerSheep incorrectly calculated the
> `g:killersheep_sound_ext` value to be "ogg" (Apple doesn't support
> ogg), and after making the fix to "mp3" it worked! I did manage to
> kill some poor innocent 𐟐‘ after the fix.

Well, innocent, I'm not sure.

> The change to your plugin is as follows:
>
> ```diff
> diff --git a/autoload/killersheep.vim b/autoload/killersheep.vim
> index 276c7d7..51f61a2 100644
> --- a/autoload/killersheep.vim
> +++ b/autoload/killersheep.vim
> @@ -158,7 +158,7 @@ func s:Init()
> hi def KillerLevelX ctermbg=yellow guibg=yellow
>
> if !exists('g:killersheep_sound_ext')
> - if has('win32') || len(s:sound_cmd)
> + if has('win32') || has('osx') || len(s:sound_cmd)
> " most systems can play MP3 files
> let g:killersheep_sound_ext = ".mp3"
> else

> ```

I'll commit that, thanks.

> Also interesting how you are using the command-line tool `afplay` and
> spawning it using `job_start()` to get it working on macOS before. I
> did wonder about how much difference it makes when implementing this
> patch as a dedicated user could already use that (albeit a little more
> clunky than a native API).

It more or less worked. Trial and error... I don't have a Mac anymore
thus I can't try it out now.

--
We're knights of the Round Table
Our shows are formidable
But many times
We're given rhymes
That are quite unsingable
We're opera mad in Camelot
We sing from the diaphragm a lot.
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 4, 2022, 5:21:11 PM10/4/22

to vim/vim, Subscribed

If you want another person to try this pull request out first to make sure it works, maybe @dpelle could help? (sorry for volunteering you)

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Dominique Pellé

unread,

Oct 4, 2022, 6:30:54 PM10/4/22

to vim/vim, Subscribed

@ychin wrote:

maybe @dpelle could help?

I can try it, but I have limited time. I'll try in my weekend.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 5, 2022, 7:27:37 PM10/5/22

to vim/vim, Push

@ychin pushed 1 commit.

8c9e49f Add sound support for macOS

—
View it on GitHub or unsubscribe.

You are receiving this because you are subscribed to this thread.

Lifepillar

unread,

Oct 6, 2022, 2:57:02 PM10/6/22

to vim/vim, Subscribed

I'd like to try this. How do you enable the sound feature? It's off in a standard build.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Lifepillar

unread,

Oct 6, 2022, 3:10:55 PM10/6/22

to vim/vim, Subscribed

It's off in a standard build.

My bad, I fired the wrong Vim. It's building and working fine!

The plug-in used the command-line tool afplay to play audio before

Spawning a process for each sound event is not idea, thoughl: having built-in sound as in this PR is much more desirable. Thanks for proposing this!

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Lifepillar

unread,

Oct 6, 2022, 4:02:36 PM10/6/22

to vim/vim, Subscribed

A question: if sound_playfile(p) is called repeatedly with the same path p, is the sound cached by Vim? Or does this rely on the OS to avoid hitting the file system each time? I'm thinking of a use case where a sound is played for each key press, for instance.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 6, 2022, 5:57:04 PM10/6/22

to vim/vim, Subscribed

The implementation doesn't do any caching right now. If you are using sound_playevent that would be cached for sure, but sound_playfile is currently calling the same API every time and doesn't reuse existing loaded sound objects, so we are relying on NSSound to cache for us, which I don't know if it does or not.

We could probably implement caching if we really want to, but it would complicate the behavior a bit in deciding how much to cache etc. I looked at the other implementations and the Linux/Canberra one seems to do some caching via the "volatile" flag although I don't know how much caching that does, and I'm not sure about the Windows implementation. NSSound also doesn't really expose the internals of loading a sound, so caching logic would only work if you play the same sound clip after it has finished playing. If we want a more generic caching mechanism we may need to use a deeper API.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Bram Moolenaar

unread,

Oct 7, 2022, 6:22:42 AM10/7/22

to vim/vim, Subscribed

> The implementation doesn't do any caching right now. If you are using
> `sound_playevent` that would be cached for sure, but `sound_playfile`
> is currently calling the same API every time and doesn't reuse
> existing loaded sound objects, so we are relying on NSSound to cache
> for us, which I don't know if it does or not.
>
> We could probably implement caching if we really want to, but it would
> complicate the behavior a bit in deciding how much to cache etc. I
> looked at the other implementations and the Linux/Canberra one seems
> to do some caching via the "volatile" flag although I don't know how
> much caching that does, and I'm not sure about the Windows
> implementation. NSSound also doesn't really expose the internals of
> loading a sound, so caching logic would only work if you play the same
> sound clip after it has finished playing. If we want a more generic
> caching mechanism we may need to use a deeper API.

Before implementing caching, check the timing. If you have an SSD, like
most recent sytems, the load time is likely very short anyway, less than
a msec. On top of that, the OS will do file caching. File systems have
been optimized over decades, often you can just rely on that. Your
biggest problem might be how you want to measure this...

--
"Intelligence has much less practical application than you'd think."
-- Scott Adams, Dilbert.

/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

—

Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 7, 2022, 7:47:08 PM10/7/22

to vim/vim, Subscribed

Yeah I'm definitely not suggesting I would work on caching functionality for first pass at all. For all I know the current implementation should be good to merge but not sure if you want more Mac people to concur it works first.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Bram Moolenaar

unread,

Oct 8, 2022, 8:50:58 AM10/8/22

to vim/vim, Subscribed

Closed #11274 via 4314e4f.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Reply all

Reply to author

Forward

[vim/vim] Add sound support for macOS (PR #11274)

Yee Cheng Chin

You can view, comment on, or merge this pull request online at:

Commit Summary

File Changes

Patch Links:

lgtm-com[bot]

Yee Cheng Chin

codecov[bot]

Codecov Report

Yee Cheng Chin

Yee Cheng Chin

Bram Moolenaar

Yee Cheng Chin

Yee Cheng Chin

Dominique Pellé

Yee Cheng Chin

Bram Moolenaar

Yee Cheng Chin

Dominique Pellé

Yee Cheng Chin

Lifepillar

Lifepillar

Lifepillar

Yee Cheng Chin

Bram Moolenaar

Yee Cheng Chin

Bram Moolenaar