lukas-reineke / lsp-format.nvim

A wrapper around Neovims native LSP formatting.
559 stars 27 forks source link

Insert mode formatting, see #42 #45

Closed tombh closed 2 years ago

tombh commented 2 years ago

(Note: draft PR against develop branch)

Firstly, you can see here my general idea to support Insert mode formatting. I'm trying to manage 2 known problems here:

  1. Applying formatting updates if changes could have occurred beyond the reach of vim.b.changedtick. Namely, when leaving Normal mode for Insert mode. Note that I am also thinking of solving a currently unsupported edge case where it is theoretically possible to both leave and reenter Normal mode before a LSP format request returns.
  2. Formatting not applied when in Insert mode. My solution to this is just to force a sync LSP request if the request was triggered from Insert Mode itself.

My general approach to all these is to create a new autocommand that toggles a flag on InsertEnter during an async LSP request. So I'm suggesting changing the format-blocking criteria from: M._has_changedtick_changed(ctx.bufnr) or vim.startswith(vim.api.nvim_get_mode().mode, "i") to: M._has_changedtick_changed(ctx.bufnr) or vim.b.format_has_entered_insert_mode

It's just a suggestion, I'm very open to other approaches. We can just explore the space for now.

However, the main hurdle right now is actually testing the mode changes. There are 2 commits; in the first I'm just testing the existing behaviour, and I control mode detection with a stub. In the second commit I introduce the autocommand, so I want to actually make Nvim change mode in order to trigger the testable behaviour.

I've spent a good while trying to figure it out. I feel like it's because Plenary places a floating window, even in the headless UI, with set nomodifiable. But no matter what commands I send to try to override this, nothing seems to make Insert mode stick. I'll keep thinking about it though. This is just my WIP for now.

lukas-reineke commented 2 years ago

Note that I am also thinking of solving a currently unsupported edge case where it is theoretically possible to both leave and reenter Normal mode before a LSP format request returns.

This is supported. When you exit insert mode, changedtick will be updated.

lukas-reineke commented 2 years ago

I don't like the idea that formatting automatically turns sync when in insert mode. Even though it will never format the buffer then, one of my main goals for this plugin is that you never notice formatting at all. It shouldn't start blocking if you don't specifically tell it to.

I think an easier solution to this might be to add a force option. If you set force, it will just ignore the mode and changedtick and overwrite the buffer. Then you can set sync and force together to get what you want.

tombh commented 2 years ago

I've just updated the PR with a force option instead, as you suggested. So default is to retain the current behaviour and enabling format in insert mode would be like this: require("lsp-format").setup( {}, { prioritize_async_over_formatting = false } )

lukas-reineke commented 2 years ago

Sorry you are still making this way more complicated than it needs to be.

I implemented it myself now. It really was only one line of code. I just added force as a format option. It's in the 2.4.0 release.

tombh commented 2 years ago

It seems that option has to be set on a per-filetype basis? There's no way to globally set it?

Also, it's clobbering, why not make the force option sync?

What's a use case for what you've implemented?

What I'm interested in is simply using Neovim like a conventional editor. I don't use Neovim in Normal mode. If there's a genuine reason that Neovim shouldn't be used like that, I'd like to know it.

lukas-reineke commented 2 years ago

There's no way to globally set it?

No, currently not. It might be nice to add global format options

why not make the force option sync?

Because sync is already an option. You can use them together or individually.

What's a use case for what you've implemented?

For you to be able to use the plugin from insert mode!? You can now set both force and sync, and it will work how you want it to.

tombh commented 2 years ago

Maybe I'm missing something but sync and force are per-filetype options. They're exceptions not rules. In a narrow, technical sense that does indeed provide support for Insert mode. But having to write out every single filetype you want to be formatted isn't an indication that this plugin welcomes Insert mode formatting.

Also, as an aside, it seems there's a bug with your implementation. I don't think params is captured and passed as ctx.params.options when calling M._handler in the if format_options.sync then branch condition.

lukas-reineke commented 2 years ago

Please stop.. I went out of my way to support your use-case that no one else will benefit from. If this is still not good enough for you, please just stop using my plugins.

I released a fix for the bug