Wow, very cool! Didn't know about the systemPreferences API here, that's awesome. Code change looks great 👍

One thing I did notice though is that it looks like your submodules are out-of-date - you might need to run git submodule update to bring in the latest, so they don't show as diffs here.

@bryphe thanks for the heads up re the submodules

Codecov Report

Merging #2331 into master will increase coverage by 0.85%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #2331      +/-   ##
=========================================
+ Coverage   37.34%   38.2%   +0.85%     
=========================================
  Files         298     300       +2     
  Lines       12352   12518     +166     
  Branches     1632    1647      +15     
=========================================
+ Hits         4613    4782     +169     
+ Misses       7490    7481       -9     
- Partials      249     255       +6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c09bcf...77dfe67. Read the comment docs.

Does this setting get changed globally for ApplePressAndHoldEnabled? That would be weird.

How are we reading input in Oni? Is it from this file https://github.com/onivim/oni/blob/master/browser/src/Input/KeyboardInput.tsx which uses an <input> field? I think <input> elements process key inputs differently than a raw/global event handler.

If we attach key event handlers on window, there should be no rate-limiting on keys being held.

I tried this in the Oni devtools console (on mac os) and it correctly spams keys being held down: window.addEventListener('keydown', e => console.log(e.key))

Are we using window.addEventListener or can we use it for input events? Is there a reason not to use it? I use this method in my electron app on multiple macs, and I never had to mess with the ApplePressAndHoldEnabled flag. But maybe I'm wrong/missing something.

@Breja the documentation regarding that tbh is a bit ambiguous I still need to test the behaviour of this locally, but from what I have seen even if it were the case that the action were global which I agree would be unexpectedly weird, there are additional methods available to restore a users setting before the window is closed.

I personally would cc @bryphe re. the input handling side of things its not a part of the code base I've looked at much

Ah ya, good question @Breja! It does seem like the behavior is different when using window.addEventListener vs <input /> events.

Originally we only used window.addEventListener (which is simpler), but we switched to the <input /> in order to support a few international scenarios:

Dead keys:

IME:

As far as I could tell, there wasn't a good way to handle these cases from just the keydown event - if you use the <input>, you can at least benefit from the browser's built-in IME support.

And good point about the setting - we definitely shouldn't be setting it globally! We should validate that the API only sets it for our app.

@bryphe ah interesting. I did not know that - learn something new everyday.

Thanks for taking the time to answer my question. 😃

@Breja - nice! Sure thing, it's great to see you around here. We're big fans of veonim too, really impressive work!

Gave this a try on my machine, this is how it sets it:

So its local just to Oni!

For completeness sake, now I've looked into it there is also registerDefaults in Electron 2+ which takes key:value pairs and registers them (ie temporary), rather than setting them.

For something so critical, I think this being a set makes sense, since otherwise we'd have to do it every time.

@CrossR its cool to have it confirmed thanks for having a look 👍 , is this working for you locally personally it throws an error for me re. the third argument not being able to be converted to a boolean, couldn't find any reason to explain why ended up trawling through the code to find out why but never got to the bottom of it

@Akin909 I originally got that error, but have added the changes I made as review comments, which then made this work.

@CrossR thanks for checking in on this I kind of let this one fall by the wayside anyway all up to date just built it locally with no issues as well 👍

Great to see platform adequate handling for this! One nit: for future reference and the sake of searchability, this PR should not reference “iOS” in its title: that is the iPhone / iPad OS. The desktop OS was called, until recently, “OS X” and is now called “macOS”. Mac users are highly unlikely to search for a different OS when looking up issues for their platform.

This PR has Changes Requested by @CrossR but it looks like they were resolved. Is this PR waiting on anything?

I think its waiting on one of us testing it again now with the changes.

I was unable to get it to work, but I'm not convinced that its the codes problem and more me deleting the old key caused this way to not work.

I was unable to get it to work, but I'm not convinced that its the codes problem and more me deleting the old key caused this way to not work.

@CrossR tbh the same goes for me I'm not entirely sure what a good way to test this specific functionality would be I have globally on my mac already turned pressAndHold off, if I turn it on I think it globally overrides all my apps' behaviours and not sure if this command should be able to override it if ive set it via the cli, I need to try turning it off for oni alone and seeing if that ignores it but I'm not sure either if thats a valid test if it works.