Feature/set press and hold on macOS

[akinsho]

Set a mac users press and hold setting to facilitate ease of use of oni for new users fixes #1888, @CrossR thanks for locating this setting 👍 this PR adds a call to main.ts to see if the setting is false if so it applies it.

Todo

• Test locally

[bryphe]

Wow, super cool! Didn't know we could do this. This will alleviate a common pain point on OSX. Nice find @akin909 💯

[bryphe]

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.

[akinsho]

@bryphe thanks for the heads up re the submodules

[codecov]

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

Impacted Files	Coverage Δ
browser/src/UI/Shell/ShellView.tsx	46.66% <0%> (-20%)	⬇️
browser/src/Platform.ts	30% <0%> (-3.34%)	⬇️
browser/src/UI/components/CommandLine.tsx	30.43% <0%> (-2.9%)	⬇️
browser/src/Services/Sidebar/index.ts	36.84% <0%> (-2.05%)	⬇️
browser/src/Editor/NeovimEditor/NeovimSurface.tsx	63.63% <0%> (-1.59%)	⬇️
browser/src/Services/Sidebar/SidebarStore.ts	11.42% <0%> (-1.21%)	⬇️
browser/src/UI/components/ExternalMenus.tsx	57.14% <0%> (-1.2%)	⬇️
...ervices/Configuration/FileConfigurationProvider.ts	22.89% <0%> (-1.11%)	⬇️
browser/src/UI/components/Tabs.tsx	31.06% <0%> (-0.62%)	⬇️
...rowser/src/Services/WindowManager/WindowManager.ts	67.27% <0%> (-0.32%)	⬇️
... and 23 more

---

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.

[ghost]

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.

[akinsho]

@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

[bryphe]

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.

[bryphe]

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.

[ghost]

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

Thanks for taking the time to answer my question. 😃

[bryphe]

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

[CrossR]

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

So its local just to Oni!

[CrossR]

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.

[akinsho]

@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

[CrossR]

I made the changes specified to my local install that didn't have anything set, it updated Oni fine to have the correct settings.

Should relieve a big pain point!

[CrossR]

This should be false not "true", since it needs to be a bool and it should be setting this value to false.

[akinsho]

yeah that bit is quite confusing as the type for it from electron and the docs state the argument is a string got stuck on that for a while, seems like an issue for electron and their types for this function

[CrossR]

This should be pressAndHold (no !), since if ApplePressAndHoldEnabled is enabled, we want to disable it.

[CrossR]

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

[akinsho]

@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 👍

[kopischke]

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.

[keforbes]

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

[CrossR]

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.

[akinsho]

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.