Closed fidian closed 7 years ago
Patch looks good to me. Tested it a bit manually, and everything seems fine (though everything was fine for me already).
The only weird bit is that when using vim -b doc/some-file.txt.gz
This is expected. Because IsHexmodeEditable
prioritizes checking for gzipped help files over checking for binary
flag, since manually adding the binary
flag is indistinguishable for gzipped help files.
I can never edit it in hexmode
You can manually use :Hexmode
after opening the file. But It'd be pretty weird for someone to want to hex edit a temporarily extracted text file.
:help
works, and editing files, manually switching to/from Hexmode works.
Only problem here: opening a .txt.gz
file which is not in a doc
folder (error message from gzip.vim
, then the compressed file is opened in Hexmode).
Maybe just remove the doc
from the regex in line 70? It's not really needed I think…
@chaoren and @hcgraf - Thanks for testing this patch so quickly!
Ok, I've changed it so the gzip plugin can always win, even if it isn't loaded or doesn't properly edit the file. I'd welcome all feedback and suggestions for improvements.
I've removed the .*
from the beginning of the pattern. I didn't realize that you had to escape special characters in the pattern, so the .*
was from some of my early attempts to make it work. Good catch.
The matching on file extensions comes from the gzip plugin. They use this line:
autocmd BufReadPre,FileReadPre *.gz,*.bz2,*.Z,*.lzma,*.xz setlocal bin
Because I don't know how to make a BufReadPre
operate on all extensions except the ones that the gzip plugin uses, I thought that this was the cleanest way to test the file extension.
One thing I was pondering is if I should only exclude those file extensions if the gzip plugin is loaded. I decided to not worry about it because it's typically shipped with the system and so almost all installations of Vim have that plugin.
if exists("loaded_gzip") && ...
The gzip plugin doesn't use .zip. If other plugins conflict with Hexmode, I suppose I will have to exclude them as well. :-/ For instance, I found a plugin that does transparent encryption with GPG and, if it ever shows up here as a bug, I will add another comment and test to ensure we're not editing GPG files if that plugin is loaded.
Because I don't know how to make a BufReadPre operate on all extensions except the ones that the gzip plugin uses
Don't think it's possible.
I thought that this was the cleanest way to test the file extension.
Agreed.
The gzip plugin doesn't use .zip.
Maybe worth looking at the zip plugin and see if it uses set bin
too.
I peeked at those plugins and they seem to do things differently than gzip. I'm not entirely sure how they work, but I tested Hexmode versus a zip and a tar file and they both seemed to work as expected: Hexmode didn't ever kick in even with vim -b
.
Shortened the comment and fixed the regular expression. I only intended to delete two characters, so it's great that you spotted my mistake.
Looks good. 👍
Looks good to me too!
Thanks for the review!
Seems to close #27. Would love others to see if this also solves the problems.
This was inspired by https://github.com/vim/vim/issues/1841#issuecomment-315794374 and the suggestion that we were overwriting
l:binary
.The only weird bit is that when using
vim -b doc/some-file.txt.gz
, I can never edit it in hexmode. I think this is acceptable.