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

Use a scratch buffer with syntax highlighting only for previews #467

Merged
merged 12 commits into from
Nov 20, 2024

Conversation

jelmansouri
Copy link
Contributor

This is a proof of concept based on telescope and trouble implementation for previews. I can work further on the PR to make it worthy of pushing if the maintainer is ok with this.

@github-actions github-actions bot requested a review from stevearc August 22, 2024 15:51
@jelmansouri jelmansouri changed the title Use a scratch buffer with syntax highlighting only Use a scratch buffer with syntax highlighting only for previews Aug 22, 2024
@jelmansouri jelmansouri marked this pull request as draft August 22, 2024 15:53
@stevearc
Copy link
Owner

What is the purpose of a scratch-based preview instead of loading the buffer itself?

@jelmansouri
Copy link
Contributor Author

It's an attempt of fixing this #435, but also speeding up previews in general by not having the LSP run on them as usually people just cycle through files quickly (at least it's usually what I do when enabling previews). It seems that other previewers on other popular plugins operate this way.
I can make it so it's an option as well or try to factor the code so users can customize how the buffer gets created.
Thanks for taking the time to discuss this and thanks for this awesome plugin as well !

@stevearc
Copy link
Owner

Hmmm...that linked bug seems like something that should be fixed upstream. If an async process is trying to access an invalid buffer, it needs to check if the buffer is valid before operating.

The performance increase is interesting, but how much of a boost is it? Is it just tied to not attaching the LSP client? This is a decently-sized chunk of code and complexity, so I just want to make sure that the benefit is worth the cost. If the goal is just to avoid attaching the LSP, could we instead do something simpler like suppress the FileType autocmd and manually trigger treesitter highlighting?

@jelmansouri
Copy link
Contributor Author

Sorry for getting back to you promptly on this, it really depends on the kind of LSP you are running, when I have the Unreal codebase open with clangd running, browsing CPP files makes at least one of my CPU spike. Browsing my lua config seems ok, on rust it depends. The thing is I don't really nead LSP on preview of files, I know the bug is on neovim side and it's still gonna happen if you enable the LSP on previews, for me it just pointed out that the LSP is not needed, I rather that it has minimal impact on my CPU. But I agree that I should try to make a leaner implementation, the code above has been heavily inspired by previews in popular plugins thus the complexity.

@RayJameson
Copy link

I tested this branch and I really like it. I'll use it until upstream fixes semantic tokens or this PR will be merged.

RayJameson added a commit to RayJameson/astronvim_config that referenced this pull request Oct 12, 2024
@RayJameson
Copy link

Update: I don't really know what causes this, but sometimes I have this error when I move cursor over directories
image

@jelmansouri
Copy link
Contributor Author

Update: I don't really know what causes this, but sometimes I have this error when I move cursor over directories image

I'll check this week, thanks for reporting.

@bdtomlin
Copy link

I'm having this same issue. Is there any hope at getting this into the master branch? I'm going to try the scratch-preview branch for now as well, but it would be nice to have a long-term solution.

@bdtomlin
Copy link

FYI - I tried the scratch-preview branch from @jelmansouri. It is an improvement. The error still happens, just much more infrequently.

@bdtomlin
Copy link

Strike that last comment. It's been a long day :) I must have forgot to restart neovim because now the branch is working with no issues.

@pkazmier
Copy link

pkazmier commented Nov 9, 2024

The thing is I don't really nead LSP on preview of files, … I rather that it has minimal impact on my CPU. … the code above has been heavily inspired by previews in popular plugins thus the complexity.

I agree with you completely. In fact, this is the primary reason I prefer mini.files over oil—much better handling of previews. Mini is so much faster in this regard. It also skips previews of binary files. But I really prefer how oil uses the current window when opened for the reasons stated in the famous oil and vinegar blog post. Unfortunately the previews in oil are too heavyweight—especially on large files. I’m interested to see how this work progresses in this PR. Will give a try this weekend.

[edit]: I just noticed #511 was opened yesterday to disable previews on large files. I don't think that is the right approach to take because mini.files handles large previews instantaneously because it only reads the number of lines needed to fill the preview buffer. The solution in #511 is really a workaround for the bigger issue of preview handling in oil.

@pkazmier
Copy link

pkazmier commented Nov 9, 2024

In addition to eliminating LSP, to better handle large files, I modified @jelmansouri'sread_file_to_scratch_buffer to only read a small portion of the file like mini.files does. Here is the modified version which performs as well as mini does now:

M.read_file_to_scratch_buffer = function(path, opts)
  local bufnr = vim.api.nvim_create_buf(false, true)
  vim.bo[bufnr].bufhidden = "wipe"
  vim.bo[bufnr].buftype = "nofile"
  vim.b[bufnr].oil_preview_buffer = true

  -- Next two lines lifted from mini.files
  local has_lines, read_res = pcall(vim.fn.readfile, path, "", vim.o.lines)
  local lines = has_lines and vim.split(table.concat(read_res, "\n"), "\n") or {}

  if not vim.api.nvim_buf_is_valid(bufnr) then
    return
  end
  local ok = pcall(vim.api.nvim_buf_set_lines, bufnr, 0, -1, false, lines)
  if not ok then
    return
  end
  local ft = vim.filetype.match({ filename = path })
  if ft and ft ~= "" then
    local lang = vim.treesitter.language.get_lang(ft)
    if not pcall(vim.treesitter.start, bufnr, lang) then
      vim.bo[bufnr].syntax = ft
    end
  end
  return bufnr
end

I would love to see oil adopt something like this for better performance of previews—eliminating LSP and reading only a portion of the file.

@pkazmier
Copy link

pkazmier commented Nov 9, 2024

@jelmansouri One thing I noticed with the PR is that directory previews are not always updated. Do you notice the same on your side?

@jelmansouri
Copy link
Contributor Author

@jelmansouri One thing I noticed with the PR is that directory previews are not always updated. Do you notice the same on your side?

Yes, I Updated to latest and integrated your change, I'll iterate a bit more on it to clean it up.

@jelmansouri jelmansouri marked this pull request as ready for review November 12, 2024 23:40
@jelmansouri
Copy link
Contributor Author

@pkazmier Compared to the original code where reading and opening the file was asynchronous, now it is sync. I decided to leave your code to address the complexity concern raised by @stevearc.
The problem with limiting the number of read lines is that if you move to the preview window, you'll get an incomplete file. Left it as an option since it doesn't add much complexity to the code but will not be the default.
The directory preview should now be fixed.

@jelmansouri
Copy link
Contributor Author

@pkazmier you need to add this now to enable the behaviour in oil setup

			preview_win = {
				scratch_buffer = true,
                                limit_scratch_buffer = true,
			},

@onexbash
Copy link

I was pulling my hairs out on this issue, I thought I messed something up in my lsp config, but seems not to be the case.
I hope this PR gets merged asap as it looks promising to fix the issues.

@jelmansouri
Copy link
Contributor Author

I was pulling my hairs out on this issue, I thought I messed something up in my lsp config, but seems not to be the case. I hope this PR gets merged asap as it looks promising to fix the issues.

To be fair the issue itself needs to be fixed within neovim, this is more another take on previews which make them lighter weight (similar to how other plugins operate)

@stevearc
Copy link
Owner

Thanks all for the hard work iterating on this PR! I did a pass on refactoring some of the logic and behavior:

  • Use a single config enum value instead of two booleans to control the preview behavior
  • Removed the max_file_size_mb option
  • Entering a scratch buffer will replace it with the real buffer. This should mitigate the downsides of using the scratch buffer or the partial scratch buffer.

LMK if these changes work for you

@jelmansouri
Copy link
Contributor Author

All makes sense, thank you so much for this.

@stevearc stevearc merged commit 21705a1 into stevearc:master Nov 20, 2024
9 checks passed
@stevearc
Copy link
Owner

Thanks for the collaboration, everyone! I think this ended up in a good place.

@bdtomlin
Copy link

Thank you guys so much for you work on this. Oil.nvim is by far the best vim file browser. It's great to see it's maintained well.

@pkazmier
Copy link

I think the next area of optimization could be dealing with large directories. With mini.files, I can move my cursor up and down with no delay at all even when one of the middle items is a directory with 10,000 files in it. Today, with oil and preview window open, there is a noticeable lag on the order of seconds doing the same. Not sure how to go about that one though.

@stevearc
Copy link
Owner

@pkazmier I recently made a series of commits designed to improve performance on large directories. Out of the box it should now be quite fast, but there are some view options that will slow it down significantly when enabled (natural_order = true is a big one, and that used to be the default). If you're still seeing poor performance on the most recent master, feel free to file a new issue and we can work out how to make it faster.

If you're curious about how I've been benchmarking or profiling oil, check out the 3 Makefile targets I added recently:

  • make benchmark - times how long it takes oil to fully render a directory of 100k files
  • make profile - uses LuaJIT's built-in profiler to find where the time is spent
  • make flame_profile - uses profile.nvim to generate a profile in the Chrome trace format. Less accurate than the LuaJIT profile, but way more information and much more visual.

@pkazmier
Copy link

pkazmier commented Nov 23, 2024

The issue is due to the foldmethod option that I have configured:

vim.opt.foldmethod  = "expr"
vim.opt.foldexpr    = "v:lua.vim.treesitter.foldexpr()"

This same issue was reported in mini.files as well due to an upstream neovim bug. Ultimately, the author resolved by explicitly setting foldmethod to manual.

Fortunately, oil has win_options setting that allowed me to resolve the multi-second lockup when navigating over a large directory when previews are enabled with these settings:

  win_options = {
    foldenable = false,
    foldmethod = "manual",
  },

pkazmier added a commit to pkazmier/nvim that referenced this pull request Nov 23, 2024
@pkazmier
Copy link

Thanks for fixing @stevearc! I saw the commit you made to address the above.

Alienover added a commit to Alienover/dotfiles that referenced this pull request Dec 3, 2024
because [#467](stevearc/oil.nvim#467) fixed the
LSP issue in previewing
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.

6 participants