Skip to content
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: run :edit command with nvim_cmd to escape special characters #2056

Merged
merged 4 commits into from Oct 14, 2023

Conversation

ZyX-II
Copy link
Contributor

@ZyX-II ZyX-II commented Sep 28, 2023

I tested this commit by dragging and dropping file named W:\tmp\neovide\(abc)\example#abc%def.txt and it was opened successfully.

Fixes #1983

What kind of change does this PR introduce?

  • Fix

Did this PR introduce a breaking change?

A breaking change includes anything that breaks backwards compatibility either at compile or run time.

  • No

@ZyX-II
Copy link
Contributor Author

ZyX-II commented Sep 28, 2023

Wondering whether somebody knows less verbose way to spell

:call nvim_cmd({'cmd': 'edit', 'args': [file], 'magic': {'file': v:false}}, {})

in Rust. I only found Neovim::cmd which removes one nesting level.

@ZyX-II
Copy link
Contributor Author

ZyX-II commented Sep 28, 2023

Note about magic and missing setting bar to false: I have tested running

call nvim_cmd({'cmd': 'edit', 'args': ['abc#def|ghi'], 'magic': {'file': v:false}}, {})

and

call nvim_cmd({'cmd': 'edit', 'args': ['abc#def|ghi'], 'magic': {'file': v:false, 'bar': v:true}}, {})

and it looks like bar is ignored even though it is not mentioned in documentation (both commands open abc#def|ghi). Makes sense though.

@fredizzimo
Copy link
Member

I think it would be less verbose to use nvim.command instead of nvim.cmd and use string formatting to build the command.

You could also use nvim.execute_lua with a single arg the filename. In that case no string formatting is needed, you just write the lua code.

@ZyX-II
Copy link
Contributor Author

ZyX-II commented Oct 2, 2023

@fredizzimo nvim.command requires proper escaping and it is exactly it being used without escaping is what #1983 is about. As for lua: using

                nvim.exec_lua(
                    "vim.cmd.edit{ args = { ... }, magic = { file = false } }",
                    vec![Value::String(path.to_owned().into())],
                )
                .await
                .ok();

works and is certainly shorter, but I do not like extra roundtrips. Also rg lua finds basically three occurrences of neovide using lua: as an implementation detail of <CR> shortcut of DisplayAvailableFonts commands, in clipboard provider implementation and in intro message implementation.


There is one additional thing though: while determining what is wrong with faulty variants of the above expression I found out that expressions executed like this do not show errors, neither directly from them, nor from my autocommand which automatically switches to the directory of the entered file (and fails due to the deficiency of fnameescape(): fnameescape('W:\tmp\neovide\(abc)') yields W:\tmp\neovide\(abc) which is treated as W:\tmp\neovide(abc) which does not exist). In my opinion this is wrong.

Note that errors do not show in log either. Not that I consider having them only in a log a good thing, in my opinion it makes sense to display errors in main UI.

@fredizzimo
Copy link
Member

This will add a bit more lua #2062. I think the absolutely cleanest solution would be to add a new function there that can be called through RPC from neovide.

For the error handling:
You need to deal with the errors inside the function you create, so that the error can be reported to neovim instead of neovide. All the nvim.* calls return the error back to neovide, which is not what we want here, since we can't do anything with them other than appending them to the log. The .ok() you have at the end just tells it to ignore any errors.

@ZyX-II
Copy link
Contributor Author

ZyX-II commented Oct 2, 2023

I think it makes sense to merge this as-is, then (not) change how errors are dealt with later and basically for everything in ui_commands: I just realised that errors for almost every command there can appear and are silenced: it is rather common to have autocommands triggering at commands like split, but I just remembered (or, actually, seen it in #2062) that there is OptionSet autocommand which can trigger for setlocal commands too.

Also, possibly not with function in lua, but with replacement for ok on Rust side which will call something like nvim.err_write.

@MultisampledNight
Copy link
Contributor

Looks like we don't even support drag-and-drop on Wayland atm, but tested through Xwayland and it works perfectly fine, thanks!

I'll take care of making the callback a bit more readable with ad-hoc placed closures, then I'd merge this.

@MultisampledNight
Copy link
Contributor

Turns out that wasn't even necessary, nvim_rs::Value handily offers From/Into implementations for its contained values.

Thank you! Chooooooooooo chooooooooooooo 🚂

@MultisampledNight MultisampledNight changed the title Run :edit command with nvim_cmd to escape special characters fix: run :edit command with nvim_cmd to escape special characters Oct 14, 2023
@MultisampledNight MultisampledNight merged commit 999f37e into neovide:main Oct 14, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

drag-n-drop to open file that has "()" in its PATH
3 participants