feat!: Option key as meta configuration #2486

9mm · 2024-04-14T13:58:57Z

updating #2046

Here we re-implement support for option-as-meta by using the winit API: window.set_option_as_alt(oa) ...where oa is of type winit::platform::macos::OptionAsAlt. By relying on the winit API this way, we are now able to map only the left, only the right, or both of the two physical 'option' keys to the virtual key 'meta', which leaves the other 'option' key free so that it can be used to access the additional two layers of the input source. (E.g. with the u.s. english input source, option-c produces "ç" and option-shift-c produces "Ç".) This is done by setting the nvim variable: vim.g.neovide_input_macos_option_key_is_meta Possible values (defined by init::platform::macos::OptionAsAlt) are: "Both" "OnlyLeft" "OnlyRight" "None" The preëxisting variable vim.g.neovide_input_macos_alt_is_meta can still be used, and if true, it will internally result in the use of OptionAsAlt::Both. Separately, this commit fixes an issue where, in the event of a key-press like option-shift-command-y (with vim.g.neovide_input_macos_alt_is_meta set to true), neovide would produce the string "<M-S-D-y>" instead of "<M-D-Y>" because the `option` key was down, and format_normal_key() took this to imply that the key is "special" (i.e. in the same category as escape, return, F1, etc. as opposed to the category of printable characters), which led format_modifier_string() to include "S-" in its output. Also, in format_modifier_string(), the test for whether 'meta' is pressed has been simplified.

9mm · 2024-04-14T14:19:35Z

i think i nuked his stuff, it was just too hard to resolve that without just starting over ... if i get this working i will try to attempt again because then i will know what to change, and ill keep his stuff

fredizzimo · 2024-04-14T14:23:17Z

Yes, I think you should be able to keep most of the stuff in the Keyboard manager, but the rest probably needs to be rewritten anyway

9mm · 2024-04-14T16:04:41Z

@fredizzimo so we are killing the entire old setting right? it seems like a lot of this window_wrapper code is trying to map old settings etc, but if we just make it a breaking change i assume all of this is a lot simpler

9mm · 2024-04-14T16:16:07Z

so.... that is my first attempt. i really have no idea what a lot of this code was but i basically took a bunch of critical thinking guesses...

my test case (pasted below) i wrote in the original issue, i notice the Option (right) + c produces ç case works actually, and if i disable the setting then it produces ç for both option keys, which makes me think......... the most rudimentary raw level it is working.

Now i have to go test the different edge cases, test all settings values, etc.

Can you see if I made any blindingly stupid mistakes? I left a lot of code out that seemed to not be needed anymore.

Btw the test here did not work however...

Meta (left) + Command + y produces <M-D-y> instead of <M-S-D-Y>

So maybe this is a different problem unrelated to the issue, as i can definitely tell the meta key left/right IS working

I can confirm that with the following setting:
vim.g.neovide_input_macos_option_key_is_meta = 'OnlyLeft'
Option (right) + c produces ç

Meta (left) + Command + y produces <M-D-y> instead of <M-S-D-Y>

I tested OnlyLeft, OnlyRight, Both, None and all work as expected

I dont use M in hotkeys much so if there is more specific test of what to look for please let me know. As of now I only tested by going:

Command + Meta (left option) + x = <M-D-x>

or

Command + Meta (left option) + Shift + x = <M-D-x> = <M-D-X>

fredizzimo · 2024-04-14T17:24:37Z

@9mm, the general approach looks fine.

@fredizzimo so we are killing the entire old setting right? it seems like a lot of this window_wrapper code is trying to map old settings etc, but if we just make it a breaking change i assume all of this is a lot simpler

Ideally, we would want that the old setting continues to work or do nothing and give a warning about deprecation. But if it's too much work, we can add it as a breaking change in the release notes, and maybe add a sticky issue.

Btw the test here did not work however...
Meta (left) + Command + y produces instead of

Do you mean that pressing Meta (left) + Command + Shift + y (note shift is included) still produces <M-S-D-Y> instead of <M-D-y>, I'm pretty sure that was part of the original fix. And should be fixed in your version as well.

9mm · 2024-04-14T17:28:13Z

I take that back, it actually did work. i think i had the config wrong

also, i dont think its necessarily hard to keep the old option, but how do we manage that in the settings? i suppose it seems weird to keep old settings floating around as opposed to just putting breaking change info on the release notes as it seems like it's not a "hard" breaking change to fix, but this is my uneducated 2 cents

9mm · 2024-04-14T17:28:48Z

specifically im referring to the key name in the struct. like we would have 2 keys for 2 different option settings

fredizzimo · 2024-04-14T17:34:29Z

src/window/keyboard_manager.rs

+
+                    #[cfg(not(target_os = "macos"))]
+                    {
+                        self.meta_is_pressed = self.modifiers.state().alt_key();


This confuses me a bit, since no one calls the alt key meta anymore. More typically the windows key is called meta.

But I guess in this context it's ok, since Vim(Neovim) also refers to it as meta.

fredizzimo · 2024-04-14T17:40:22Z

For the old value, in the code we should only ever use the new values, but at the parsing stage, we can support the old setting, and convert it to both automatically.

We can then show a warning using this system:

neovide/src/error_handling.rs

Lines 18 to 37 in fcf5e87

    
           fn show_error(explanation: &str) -> ! { 
        
               error!("{}", explanation); 
        
               panic!("{}", explanation.to_string()); 
        
           } 
        
           pub fn show_nvim_error(msg: &str) { 
        
               send_ui(ParallelCommand::ShowError { 
        
                   lines: msg.split('\n').map(|s| s.to_string()).collect_vec(), 
        
               }); 
        
           } 
        
           /// Formats, logs and displays the given message. 
        
           #[macro_export] 
        
           macro_rules! error_msg { 
        
               ($($arg:tt)+) => { 
        
                   let msg = format!($($arg)+); 
        
                   log::error!("{}", msg); 
        
                   $crate::error_handling::show_nvim_error(&msg); 
        
               } 
        
           }

But support for warnings need to be added, instead of just errors.

9mm · 2024-04-14T17:41:06Z

ooooh great idea. ok I can do that

9mm · 2024-04-14T17:45:11Z

Note to self of things to fix:

parse old setting
None doesnt appear to be parsing properly as a default:

ERROR [neovide::window::settings] Setting neovide_input_macos_option_key_is_meta expected one of OnlyLeft, OnlyRight, Both, or None, but received Error("unknown variant \"None\", expected one of OnlyLeft, OnlyRight, Both, None"): String(Utf8String { s: Ok(""None"") })

9mm · 2024-04-14T17:58:17Z

So one area I havent really learned about yet in rust is serde/serialization/deserialization. how would you handle this part?

ERROR [neovide::window::settings] Setting neovide_input_macos_option_key_is_meta expected one of OnlyLeft, OnlyRight, Both, or None, but received Error("unknown variant "None", expected one of OnlyLeft, OnlyRight, Both, None"): String(Utf8String { s: Ok(""None"") })

It looks like its returning "None" as a string

#[cfg(target_os = "macos")]
impl ParseFromValue for OptionAsMeta {
    fn parse_from_value(&mut self, value: Value) {
        let s = value.as_str().unwrap();
        match OptionAsAlt::deserialize(s.into_deserializer()) as Result<OptionAsAlt, value::Error> {
            Ok(oa) => *self = OptionAsMeta(oa),
            Err(e) => error!("Setting neovide_input_macos_option_key_is_meta expected one of OnlyLeft, OnlyRight, Both, or None, but received {:?}: {:?}", e, value),
        };
    }
}

I notice even if something is specified in the config, it comes back Some("None") which I think is correct, however, it also makes me think there should be no error there then, otherwise it prints the error before immediately getting the settings changed event.

9mm · 2024-04-14T18:06:13Z

Ah, it seems the first time it does include extra quotes the first time 🤔 ill have to look at what this as_str is doing

[src/window/settings.rs:92:9] s = "\"None\""
ERROR [neovide::window::settings] Setting neovide_input_macos_ ......
[src/window/settings.rs:92:9] s = "OnlyLeft"

fredizzimo · 2024-04-14T18:14:49Z

That seems like something that would have to be fixed in Winit, their code looks like this
https://rust-windowing.github.io/winit/src/winit/platform/macos.rs.html#429, it probably needs a serde rename attribute https://serde.rs/field-attrs.html#field-attributes

You can try by defining your own enum type, and add the correct attributes, that also allows you to rename all values to kebab-case, like we do here

neovide/src/settings/config.rs

Line 37 in fcf5e87

#[serde(rename_all = "kebab-case")]

You then need to convert that enum to the winit one, but that should be easy with a simple match statement.

9mm · 2024-04-14T18:48:51Z

rust is intense lol

I got a lot of what you just said done, however... I suppose the same problem exists. I dont fully understand why initially it passes a string with extra quotes, and then it passes a standard string.

I do see that its properly doing kebab case... sort of.......

[src/window/settings.rs:104:17] value = "\"none\""
[src/window/settings.rs:104:17] value = "OnlyLeft"

2f59222

This is what the output is for my dbg! print here

9mm · 2024-04-14T18:55:27Z

Oh nevermind...... i see, the expected values they actually set in the config are now properly kebab case as well. duh

the question is still however... why is it double quoted the first time and not the other time... hmm

[src/window/settings.rs:104:17] value = "\"none\""
[src/window/settings.rs:104:17] value = "only_left"

9mm · 2024-04-14T19:16:41Z

For the record I know I can use (_) => however then I can't differentiate between an empty value, and an invalid value, because both would match that.

Is this expected behavior that to_str returns extra quotes if its a default?

fredizzimo · 2024-04-14T19:19:46Z

It should not, but did you try to rename the None option yourself, above it
#[serde(rename = "none")]

9mm · 2024-04-15T18:56:15Z

Ok, all flags are removed.

Everything works great... There's only one final issue...

Right now I have a simple deprecation warning for the old setting via a WindowSettingsChanged::InputMacosAltIsMeta listener (which also requires input_macos_alt_is_meta old setting on WindowSettings.

This is only a warning though, it doesn't attempt to set the Both behavior if that legacy setting is set to true.

#[cfg(target_os = "macos")]
WindowSettingsChanged::InputMacosOptionKeyIsMeta(option) => {
    self.set_macos_option_as_meta(option);
}
#[cfg(target_os = "macos")]
WindowSettingsChanged::InputMacosAltIsMeta(enabled) => {
    if enabled {
        error_msg!(concat!(
            "neovide_input_macos_alt_is_meta has now been deprecated. ",
            "Use neovide_input_macos_option_key_is_meta instead. ",
            "Please check https://neovide.dev/configuration.html#macos-option-key-is-meta for more information.",
        ));
    }
}

IF I were to attempt to set Both if I detect the legacy setting to true, that's where it starts quite hairy because...

I've actually re-typed this about 50 times and there's really no simple way to explain it... maybe it would be better to simply just ask, how would you do it?

I need to basically update 2 things....

I have to call self.skia_renderer.window().set_option_as_alt() which happens as a result of WindowSettingsChanged events
I have to set the state of self.meta_is_pressed with custom logic which directly looks at SETTINGS.get::<WindowSettings>() to detect the value

And both of these needs to stay in sync, if i were to do this legacy setting.

The tricky bit is that 1) can be set for default events even if that setting isnt even in their config, so it creates race conditions, and even if i do use a flag, that flag must be kept in sync with 2) which doesnt look at flags at all, it just looks at the WindowSettings value

fredizzimo · 2024-04-15T19:00:09Z

The message is an error, not a warning, right? In that case I think it's fine to not do anything, it clearly tells what to do and if things don't work the users can blame themselves for not reading the message. So, it should be fine as it is.

fredizzimo · 2024-04-15T19:01:42Z

We just need to change deprecated to removed.

9mm · 2024-04-15T19:01:46Z

Ok cool. it is an error_msg!. I will change 'deprecated' to 'no longer supported' so its not implying it still works

sorry for all the tons of noise on this... I just dont want to add junk to this amazing repo, and this tbh is my first time writing actual rust 💀

9mm · 2024-04-15T19:01:58Z

Ah beat me to it

fredizzimo · 2024-04-15T19:19:06Z

I did not fix the macOS Clippy warnings in my PR, so do you think you can do it here?

I think the window parameter can be removed completely and I don't think we need to deprecate the function, just because the feature is deprecated for the users.

9mm · 2024-04-15T19:25:47Z

definitely, I was actually going to fix that stuff but stopped in case they were there for a reason haha.

fredizzimo · 2024-04-15T19:30:13Z

I actually don't see any code that would fix this bug:

Meta (left) + Command + y produces instead of .

As far as I can see the condition for include_shift has not changed. But I don't see why it would have been broken before either. Maybe it was fixed elsewhere even without these changes, otherwise I'm missing something subtle.

Can you test that without these changes?

9mm · 2024-04-15T19:38:55Z

Heres the most recent result of testing this PR. I copied the test cases I did above but just re-ran through them all again. LMK if this works. I am not sure the issue with shift, but heres the results:

vim.g.neovide_input_macos_option_key_is_meta = 'only_left'

Option (right) + c produces ç

Meta (left) + Command + y produces <M-D-y> instead of <M-S-D-Y>

Command + Meta (left option) + x = <M-D-x>

Command + Meta (left option) + Shift + x = <M-D-X>

fredizzimo · 2024-04-15T19:42:13Z

Ah it's probably this change:

       #[cfg(target_os = "macos")]
        if self.meta_is_pressed {
            return key_event
                .key_without_modifiers()
                .to_text()
                .map(|text| self.format_key_text(text, false));
        }

And yes, those results look correct. If you just can verify that Meta (left) + Command + y produces <M-S-D-Y> on main and is indeed fixed here.

fredizzimo · 2024-04-15T19:43:31Z

Don't worry about those other clippy warnings, we will make sure to merge this first.

chore: Fix clippy warnings #2483

9mm · 2024-04-15T19:44:05Z

Ah ok, I will revert those last 2

and i will test main quick

9mm · 2024-04-15T19:49:22Z

I tested main. Assuming what you wrote is a typo and you meant the following, then this is what occurs on main:

Meta (left) + Command + Shift + y = <M-S-D-Y>

fredizzimo

Great! Thank you, everything looks good now.

9mm · 2024-04-15T19:50:46Z

awesome!! Thank you for all your help

JamesWidman and others added 2 commits October 17, 2023 16:07

Merge branch 'main' into option-key

eb32347

9mm added 5 commits April 14, 2024 07:27

resolve keyboard manager

07b0b84

straggler

9553025

settings and configuration deserialization

ae793f4

rustfmt

19529d5

use consistent quotes for docs

8db4957

9mm added 2 commits April 14, 2024 09:09

window wrapper... untested

17d69a8

cargo fmt

9b6de96

fredizzimo reviewed Apr 14, 2024

View reviewed changes

remove unused

a12dd4b

convert to in-house enum, convert at end

2f59222

update docs

43f6f9d

oops

b82560c

update wording

3c0e5c1

clippy: remove window argument

a9cfac6

clippy: dont deprecate function (warning to user still remains)

362f5dc

9mm force-pushed the option-key branch from 0d6315c to 362f5dc Compare April 15, 2024 19:44

fredizzimo approved these changes Apr 15, 2024

View reviewed changes

fredizzimo marked this pull request as ready for review April 15, 2024 19:51

fix nb

cedb66a

fredizzimo changed the title ~~Option key~~ feat: Option key as meta configuration Apr 15, 2024

fredizzimo linked an issue Apr 15, 2024 that may be closed by this pull request

Add option to map only the left option key to meta on macOS #1877

Closed

fredizzimo mentioned this pull request Apr 15, 2024

Fix option as meta #2046

Closed

fredizzimo changed the title ~~feat: Option key as meta configuration~~ feat!: Option key as meta configuration Apr 15, 2024

accidentally removed this

8ff99c0

Kethku merged commit bf5eda1 into neovide:main Apr 19, 2024
13 checks passed

fredizzimo mentioned this pull request Apr 23, 2024

macOS configuration options for using only the left or right alt key … #1979

Closed

moonfruit mentioned this pull request May 17, 2024

neovide 0.13.0 Homebrew/homebrew-core#171965

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Option key as meta configuration #2486

feat!: Option key as meta configuration #2486

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024

fredizzimo commented Apr 14, 2024

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024 •

edited

fredizzimo commented Apr 14, 2024

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024

fredizzimo Apr 14, 2024

fredizzimo commented Apr 14, 2024

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024 •

edited

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024 •

edited

fredizzimo commented Apr 14, 2024

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024 •

edited

9mm commented Apr 14, 2024

fredizzimo commented Apr 14, 2024 •

edited

9mm commented Apr 15, 2024 •

edited

fredizzimo commented Apr 15, 2024

fredizzimo commented Apr 15, 2024

9mm commented Apr 15, 2024

9mm commented Apr 15, 2024

fredizzimo commented Apr 15, 2024

9mm commented Apr 15, 2024

fredizzimo commented Apr 15, 2024

9mm commented Apr 15, 2024

fredizzimo commented Apr 15, 2024

fredizzimo commented Apr 15, 2024 •

edited

9mm commented Apr 15, 2024

9mm commented Apr 15, 2024

fredizzimo left a comment

9mm commented Apr 15, 2024

feat!: Option key as meta configuration #2486

feat!: Option key as meta configuration #2486

Conversation

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024

fredizzimo commented Apr 14, 2024

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024 • edited

fredizzimo commented Apr 14, 2024

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024

fredizzimo Apr 14, 2024

Choose a reason for hiding this comment

fredizzimo commented Apr 14, 2024

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024 • edited

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024 • edited

fredizzimo commented Apr 14, 2024

9mm commented Apr 14, 2024

9mm commented Apr 14, 2024 • edited

9mm commented Apr 14, 2024

fredizzimo commented Apr 14, 2024 • edited

9mm commented Apr 15, 2024 • edited

fredizzimo commented Apr 15, 2024

fredizzimo commented Apr 15, 2024

9mm commented Apr 15, 2024

9mm commented Apr 15, 2024

fredizzimo commented Apr 15, 2024

9mm commented Apr 15, 2024

fredizzimo commented Apr 15, 2024

9mm commented Apr 15, 2024

fredizzimo commented Apr 15, 2024

fredizzimo commented Apr 15, 2024 • edited

9mm commented Apr 15, 2024

9mm commented Apr 15, 2024

fredizzimo left a comment

Choose a reason for hiding this comment

9mm commented Apr 15, 2024

9mm commented Apr 14, 2024 •

edited

9mm commented Apr 14, 2024 •

edited

9mm commented Apr 14, 2024 •

edited

9mm commented Apr 14, 2024 •

edited

fredizzimo commented Apr 14, 2024 •

edited

9mm commented Apr 15, 2024 •

edited

fredizzimo commented Apr 15, 2024 •

edited