Skip to content

fix(#2352): windows: escape special filename characters on edit #2374

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

Merged
merged 6 commits into from
Aug 20, 2023
Merged

fix(#2352): windows: escape special filename characters on edit #2374

merged 6 commits into from
Aug 20, 2023

Conversation

davisthedev
Copy link
Contributor

@davisthedev davisthedev commented Aug 14, 2023

fixes #2352

  • add check if environment is windows
  • if windows, escape special characters

This works on windows, will need additional linux/mac testing.

@alex-courtis alex-courtis changed the title Fix escape special characters on windows fix(#2374): escape special characters on windows on edit Aug 14, 2023
@alex-courtis
Copy link
Member

Looks good, behind a feature flag.

Do we need to do the same for other actions such as node.open.replace_tree_buffer or node.open.tab?

We might also be able to enhance utils.canonical_path to achieve this fix.

@gegoune
Copy link
Collaborator

gegoune commented Aug 14, 2023

We definitely should have dedicated function to do that

@davisthedev
Copy link
Contributor Author

I am happy to add this in other areas or let someone with more knowledge handle it and I can test.

I am not sure if there would be any adverse effects to other pieces of code with the escaping.
That is why I only made the change at the last point.

@alex-courtis
Copy link
Member

I am happy to add this in other areas or let someone with more knowledge handle it and I can test.

I am confident it your abilities.

I am not sure if there would be any adverse effects to other pieces of code with the escaping. That is why I only made the change at the last point.

We can't cover every possible case, however we can:

  1. Test and maybe fix the different actions that edit e.g. open in tab, open in place etc.
  2. Test all codepaths that lead to this common function.

This is behind the feature flag so we can be confident.

@davisthedev
Copy link
Contributor Author

I did not have any issues with node.open.replace_tree_buffer or node.open.tab.
I did move the code to its own utils function so it can be used if the problem is found elswhere.

@alex-courtis alex-courtis changed the title fix(#2374): escape special characters on windows on edit fix(#2352): windows: escape special characters on edit Aug 16, 2023
@alex-courtis alex-courtis changed the title fix(#2352): windows: escape special characters on edit fix(#2352): windows: escape special filename characters on edit Aug 16, 2023
Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for your contribution. I'll get back to this one on the weekend.

I was tempted to put this everywhere we use a fnameescape however it's safest just to fix known issues.

@davisthedev
Copy link
Contributor Author

I don't know if the change even needs to go everywhere fnameescape is used. The issue only seems to happen whenever the path is handed over to neovim to create a buffer. It seems like everything is fine inside nvim-tree.

I am working on a similar merge for telescope, and it has the same thing. It only has this problem when it tries to open the file in a buffer or window. So I think the root of might actually be fixable inside neovim, but I am not ready to work on that codebase yet.

@alex-courtis
Copy link
Member

It only has this problem when it tries to open the file in a buffer or window. So I think the root of might actually be fixable inside neovim, but I am not ready to work on that codebase yet.

That sounds quite reasonable. We are escaping paths using vim's api, it's just these special cases that are not correct.

We might encounter an issue if this is fixed in nvim, however we cannot do anything until that time.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for your contribution

@alex-courtis alex-courtis merged commit 7c4c7e4 into nvim-tree:master Aug 20, 2023
@xarthurx
Copy link

It only has this problem when it tries to open the file in a buffer or window. So I think the root of might actually be fixable inside neovim, but I am not ready to work on that codebase yet.

That sounds quite reasonable. We are escaping paths using vim's api, it's just these special cases that are not correct.

We might encounter an issue if this is fixed in nvim, however we cannot do anything until that time.

neovim/neovim#24542

I don't expect they will change it in the near future.

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.

Cannot open file whose filename or folder name has parenthesis "(" on Windows
4 participants