preservim / nerdtree

A tree explorer plugin for vim.
Do What The F*ck You Want To Public License
19.56k stars 1.44k forks source link

NERDTreeFind: Exception not caught: NERDTree.InvalidArgumentsError: <file> should be under <path> #1344

Closed dangibson closed 10 months ago

dangibson commented 1 year ago

Windows, NERDTree 6.10.16 Open a folder in NERDTree, c:\dev. This has a sub folder called Project - ie c:\dev\Project. Open a file in vim, c:\dev\project\file.txt Note the path is called "c:\dev\Project" while the file is opened as "c:\dev\project\file.txt" - casing is the issue here. Run command NERDTreeFind

You get an error: Error detected while processing function 28_findAndRevealPath[47].... E605: Exception not caught: NERDTree.InvalidArgumentsError: c:\dev\project\file.txt should be under c:\dev\Project

I'll submit a pull request to fix this.

rzvxa commented 11 months ago

@dangibson Hi, May I ask why you closed your PR? I've recently submitted a PR to solve another problem with case-insensitive file systems when using the move operation(#1375), and introduced a flag called g:NERDTreeCaseInsensitiveFS instead of guessing based on the operating system as both macOS and Windows can support case sensitive file systems. I think with the use of this flag we can also fix this problem across platforms instead of specifically on Windows machines. Would you like to reopen your PR and use this new flag to solve this issue? Even if you don't want to work on it anymore I will create a PR based on your commits to keep the credits for them. Let me know if you want to continue working on it.

dangibson commented 11 months ago

I fixed the issue I had and it's been working fine for me. I didn't think I closed it.

rzvxa commented 11 months ago

Oh, I don't know why I thought it was closed, what do you think about using the same flag in your PR? do you think it's a good idea?

dangibson commented 11 months ago

I would probably default to assuming it is case insensitive instead of sensitive. While that is technically wrong for linux, it's unlikely people would have a file or folder called "abc" and another called "Abc" in the same folder. Although the end result is the same, the difference is that on Windows you wouldn't have to set the flag manually, and on linux you probably wouldn't need to change it so it would be a slightly better out of the box experience.

If you have a pull request that resolves the issue then I'm happy to scrap mine - no credit needed. I resolved the issue for me and submitted a PR but nothing happened with it and I don't really have an interest in working further on it.

rzvxa commented 11 months ago

I've set it to Case sensitive by default to make it backward compatible, this way we don't break anything for users who are used to this behavior and may have put their workspace(although stupid) around this thing. You can have something like this in your .vimrc file if you work with the same vim configuration on different machines

 if nerdtree#runningWindows()
     let g:NERDTreeCaseInsensitiveFS = 1
 elseif has('gui_mac') || has('gui_macvim') || has('mac') || has('osx')
     let g:NERDTreeCaseInsensitiveFS = 1
 else
     let g:NERDTreeCaseInsensitiveFS = 0
 endif

If we assume case sensitivity wrong in critical places it can cause files to be overwritten which is not a good thing to happen, Not showing some files or throwing an error is much safer than overwriting it that's why I thought it is much better to give the choice to the end user. We can reuse this flag for many purposes, And in the future, we have the option to turn it on by default for Windows and macOS as they have case-insensitive file systems by default.

Let me know what you think about it.

dangibson commented 11 months ago

I think on Windows / mac it should default to insensitive without the user needing to explicitly set it since that is what the users would reasonably expect to happen. If it defaults to sensitive then they would consider it a bug and spend time searching for the solution. I think it is better to set the flag now instead of 'in the future' since 'in the future' there would be resistance to changing it since it wouldn't be backwards compatible.

rzvxa commented 11 months ago

I don't have a strong take on whether or not this flag should be on by default or not, But I think you agree with the fact that this flag is needed in the first place, Right?

dangibson commented 11 months ago

The flag is a good idea - testing a flag is easier than testing is windows or is osx or is mac or is macvim etc