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: previewers & entry maker file encoding #2430

Merged

Conversation

jamestrew
Copy link
Contributor

@jamestrew jamestrew commented Mar 21, 2023

Description

Previewers and some entry_makers currently has poor support for non UTF8 encoded files.
This PR looks to add a new option file_encoding to pickers with for previewers file_previewer & grep_previewer, qflist_previewer.
This new option is consumed by buffer_previewer.file_maker to do some string processing with vim.iconv. A similar thing also happens in make_entry.gen_from_vimgrep.

Fixes #2415

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list relevant details about your configuration

This change impacts the following pickers:
live_grep
grep_string
find_files
git_files
treesitter
current_buffer_fuzzy_find
oldfiles
buffers
marks
lsp_dynamic_workspace_symbols
lsp_workspace_symbols
lsp_document_symbols
lsp_implementations
lsp_type_definitions
lsp_definitions
lsp_outgoing_calls
lsp_incoming_calls
lsp_references

  • For each picker above, confirm previewer and entries look good for non-UTF8 encoded files
  1. create file with acentuação
  2. write file with latin1 encoding :write ++enc=latin1
  3. test pickers with :Telescope <picker> file_encoding=latin1 and verify previewer/entry
  • Regression test with standard utf8 file and no file_encoding option

Configuration:

  • Neovim version (nvim --version): NVIM v0.9.0-dev-1070+g2630341db
  • Operating system and version: Linux archlinux 6.2.5-arch1-1

Checklist:

  • My code follows the style guidelines of this project (stylua)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (lua annotations)

@jamestrew jamestrew force-pushed the fix/pickers-previewer-entry-encoding branch from 7459b81 to 191fb37 Compare March 21, 2023 23:07
@jamestrew jamestrew marked this pull request as ready for review March 21, 2023 23:08
@tjdevries
Copy link
Member

One other note would be since:

        Converted string if conversion succeeds, `nil` otherwise.

Maybe we should default back to using the input text if we get nil back from the iconv just in case? I'm not super familiar with file encoding stuff though.

@jamestrew
Copy link
Contributor Author

Maybe we should default back to using the input text if we get nil back from the iconv just in case? I'm not super familiar with file encoding stuff though.

I'm not super familiar either but probably a good idea to handle this case. Updated the code.

@@ -60,7 +60,12 @@ local function split(s, sep, plain, opts)
opts = opts or {}
local t = {}
for c in vim.gsplit(s, sep, plain) do
table.insert(t, c)
local line = opts.file_encoding and vim.iconv(c, opts.file_encoding, "utf8") or c
if line ~= nil then
Copy link
Member

Choose a reason for hiding this comment

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

i prefer something like line = line or c and then do the insert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, going back to the handling nil thing, if vim.iconv returns nil wouldn't the above "ternary operator" handle it and just return c.
We should be able to simply table.insert(t, line).

@tjdevries
Copy link
Member

sorry, one little nit and then we can merge. Thanks :)

@Conni2461 Conni2461 force-pushed the fix/pickers-previewer-entry-encoding branch from d717447 to 54474b0 Compare May 24, 2023 09:48
@Conni2461
Copy link
Member

thanks :)

@Conni2461
Copy link
Member

and sorry for my really late response. i just rebased and fixed the conflicts

@Conni2461 Conni2461 merged commit c8b6523 into nvim-telescope:master May 24, 2023
6 checks passed
@jamestrew jamestrew deleted the fix/pickers-previewer-entry-encoding branch May 24, 2023 14:03
Conni2461 pushed a commit that referenced this pull request May 24, 2023
abelmul pushed a commit to abelmul/telescope.nvim that referenced this pull request Jun 6, 2023
@brneor
Copy link

brneor commented Jun 12, 2023

Just tested this change. Even though I can now see the preview correctly I still can't search for this characters.

Fox example, in a file with the content acentuação, using Telescope live_grep file_encoding=latin1 a search for ç gives me no results.

@Conni2461
Copy link
Member

can you open a new issue, for that? also it would be good to know which sorter you use. fzf-native, fzy-native, .... so yeah basically we need information to your configuration, so a new issue would be great

@jamestrew
Copy link
Contributor Author

jamestrew commented Jun 12, 2023

I can take a look at this. I think we simply need to pass the file_encoding=latin1 option to ripgrep.

Edit: #2570
@brneor should work when using this PR.

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.

Live grep can't show non utf-8 content correctly
4 participants