-
Notifications
You must be signed in to change notification settings - Fork 219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(input): pass bufnr and winid to event instead #1398
Conversation
Why is esc being unmapped in normal mode? esc being used to exit in normal mode seems fine, just shouldn't be used to exit in insert mode. Just the first mapping may be good enough |
No each line is an example of what you can do with the handler. You don't need to use all of them. You may want esc for normal but start with insert mode -> just use the second example. Or even write your own map (eg |
Ahh that's perfect then, thanks for the clarification |
I agree that its confusing as it's written. There should just be two completely separate examples each with it's own description. One for starting in normal mode and one for just allowing normal mode even though it still starts in insert mode. |
No other event has even a single example nor a type definition and I kinda feel it's awkward to make such a long example for this single event. I might start to think to delete the whole example altogether to align with other events. |
I have a few more thoughts for discussion: Do we really want to pass the NuiInput just to enable mapping? I think it would be better for the user if we just used standard nvim buffer local mapping so that they can use familiar nvim syntax for changing those mappings. BUT, if we do pass the NuiInout, we should include links to the Nui docs: And also the unnmap example should use the unmap method and not set the mapping to an empty fnction. |
No definitely not. This is pretty common functionality so it's worth having a longer doc. Also, it's not at all a good thing that the rest of the docs are sparse, it's a huge problem to be fixed. We should always endeavor for more docs. |
TBH I'm not interested in this feature and never gonna use this so I don't care how much docs there will be. If you don't want to pass NuiInput, what do you think we should pass? Bufnr and winid? |
Maybe, but I don't think you really need to pass anything because the popup would be the active window/buffer when the event runs. In the future we should be passing the type of window it is running in, such as |
This is not guaranteed as the event is called inside a vim.schedule.
Are you sure? This will require a complete rewrite and does anyone have the motivation to do that? |
FYI |
NuiInput
as args to event
If that were true, then suggesting that they use the
Honestly, I still use vimscript for mappings and have never used the native lua API so I didn't realize this. Either way, for memory leak purposes I'd rather send two integers than a reference to a complex object with an unknown dependency graph.
LOL, I think you have an itchy trigger finger on the whole "complete rewrite" thing. It's a simple thing to just pass one more argument when creating an input and then carry that through to the event. Like I said though, "in the future." I would be happy to do it when I get the time. |
OK fix already pushed.
I hate when the fix is to add one more argument that can be nil to solve the problem. It creates more complexity and a lot of bugs related to wrong number of args / wrong type passed which is difficult to detect at first glance. You don't know how long it took me to debug this issue: #1227. |
I would not make it |
I was talking about the other nil-able arguments that come before type. It will be something in the line of How many nils should I put in between? I dunno. Is there a test that validates before a PR? No. What if I typoed with Also to make it work with nio and to align with other nvim apis, callback should always be the last argument. Anyways I'm not interested in changing ui/inputs (at least for now) and I think this is outside of the scope of this PR. |
Oh absolutely. Remember that I had said "in the future". As far as args, the new argument would be at the beginning. I never put non-nil args after nilable args. |
Closes: #1396
Warning
Breaking change.
I've deprecated
config.enable_normal_mode_for_inputs
entirely and it does absolutely nothing now.I've also added examples to the help page.