Skip to content

WIP: Lint on FocusGained/VimResume #2494

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.

Sign up for GitHub

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

Closed
andreypopp wants to merge 1 commit into from
Closed

WIP: Lint on FocusGained/VimResume #2494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions autoload/ale/events.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ function! ale#events#FileChangedEvent(buffer) abort
endif
endfunction

function! ale#events#FocusGained(buffer) abort
call setbufvar(a:buffer, 'force_changedtick', 1)
Copy link
Member

@w0rp w0rp May 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We shouldn't force updates to be sent to language servers here and below. We should only send updates if the buffer has actually changed. If b:changedtick isn't updated because some other process updated the buffer and Vim didn't increment b:changedtick, we can do something that causes b:changedtick to increment in response to the buffer being changed, maybe comparing the text before and after Vim lost focus. If the buffer hasn't been changed, no messages should be sent to the language servers.

Copy link
Contributor Author

@andreypopp andreypopp May 12, 2019

Choose a reason for hiding this comment

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

This is done so we can request server to recompute diagnostics. Unfortunate LSP lacks such method and I see you’ve also noted that — microsoft/language-server-protocol#737

Copy link
Member

@w0rp w0rp May 12, 2019

Choose a reason for hiding this comment

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

I understand. I think we shouldn't request diagnostics again if the buffer hasn't changed.

Copy link
Contributor Author

@andreypopp andreypopp May 12, 2019

Choose a reason for hiding this comment

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

Do you think we can add such feature (requesting diagnostics via sending didChange events) configurable for specific LSP linters? In my case I'm working on ocamlmerlin-lsp which only works per-file — and if I run a build system — I want to re-request diagnostics from the linter no matter if buffer was changed or not (there could be new/no errors just because of the build process finished).

Copy link
Contributor Author

@andreypopp andreypopp May 12, 2019

Choose a reason for hiding this comment

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

Btw such behaviour can be also useful for non-LSP linters — they don't work with "push" model (I think LSP is designed around this) also, instead you ask / "pull" diagnostics from them.

Copy link
Member

@w0rp w0rp May 12, 2019

Choose a reason for hiding this comment

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

It won't work for all language servers. Languages servers don't always respond to textDocument/didChange with diagnostics. The Language Server Protocol needs to be changed to support explicitly requesting diagnostics for such a thing to work. The protocol doesn't offer any guarantees about what will trigger diagnostics being sent, when they will arrive, or if they will ever be sent. I think it's one of the flaws of the protocol.

tsserver does have an explicit command for requesting diagnostics and pretty much guarantees a response, so we could add a command to request for diagnostics for tsserver.

Copy link
Contributor Author

@andreypopp andreypopp May 12, 2019

Choose a reason for hiding this comment

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

Right, I understand that.

That's why I'm asking if we can make this configurable per-linter so it works with ocamlmerlin-lsp (either via textDocument/didChange or I can add some new type of request to query for diagnostics as a protocol extension).

Copy link
Member

@w0rp w0rp May 12, 2019

Choose a reason for hiding this comment

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

I think we should follow the protocol and only send a message for a document being changed when it has changed, and push for the protocol to be extended to allow user to request diagnostics explicitly, like you can with tsserver.

Copy link
Contributor Author

@andreypopp andreypopp May 12, 2019

Choose a reason for hiding this comment

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

Unfortunately I don't think I can wait till it's standardised in LSP — standard bodies move slowly. I prefer standards instead be informed by real world usage. I think I'll be able to patch it up on top of ALE, not optimal but doable.

call ale#Queue(0, 'lint_file', a:buffer)
Copy link
Member

@w0rp w0rp May 12, 2019

Choose a reason for hiding this comment

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

Check g:ale_lint_on_focus_gained here again, and probably ale_enabled like we do above. Otherwise it will be difficult to turn this event off after the autocmd command has been set.

endfunction

function! ale#events#Init() abort
" This value used to be a Boolean as a Number, and is now a String.
let l:text_changed = '' . g:ale_lint_on_text_changed
Expand Down Expand Up @@ -127,6 +132,19 @@ function! ale#events#Init() abort
\)
endif

if g:ale_lint_on_focus_gained
if exists('##FocusGained')
autocmd FocusGained * call ale#events#FocusGained(
\ str2nr(expand('<abuf>'))
\)
endif
if exists('##VimResume')
autocmd VimResume * call ale#events#FocusGained(
\ str2nr(expand('<abuf>'))
\)
endif
endif

if g:ale_lint_on_insert_leave
autocmd InsertLeave * call ale#Queue(0)
endif
Expand Down
4 changes: 3 additions & 1 deletion autoload/ale/lsp.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,8 +492,10 @@ function! ale#lsp#NotifyForChanges(conn_id, buffer) abort

if !empty(l:conn) && has_key(l:conn.open_documents, a:buffer)
let l:new_tick = getbufvar(a:buffer, 'changedtick')
let l:force = getbufvar(a:buffer, 'force_changedtick', 0)

if l:conn.open_documents[a:buffer] < l:new_tick
if l:conn.open_documents[a:buffer] < l:new_tick || l:force
call setbufvar(a:buffer, 'force_changedtick', 0)
if l:conn.is_tsserver
let l:message = ale#lsp#tsserver_message#Change(a:buffer)
else
Expand Down
3 changes: 3 additions & 0 deletions plugin/ale.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ let g:ale_lint_on_save = get(g:, 'ale_lint_on_save', 1)
" This flag can be set to 1 to enable linting when the filetype is changed.
let g:ale_lint_on_filetype_changed = get(g:, 'ale_lint_on_filetype_changed', 1)

" This flag can be set to 1 to enable linting when vim gains focus.
let g:ale_lint_on_focus_gained = get(g:, 'ale_lint_on_focus_gained', 1)
Copy link
Member

@w0rp w0rp May 12, 2019

Choose a reason for hiding this comment

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

Document the option and turn it off by default. There are thousands of people using ALE, and many of them will have told ALE not to lint their buffers when they enter a file. They will be surprised if it starts linting their buffers when Vim regains focus after they update ALE.

Copy link
Contributor Author

@andreypopp andreypopp May 12, 2019

Choose a reason for hiding this comment

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

Hm... maybe we should use an existent check g:ale_lint_on_enter, what do you think? Not sure adding an extra option is worth it considering the semantics of "entering" is so poorly defined.

Copy link
Contributor Author

@andreypopp andreypopp May 12, 2019

Choose a reason for hiding this comment

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

Also — maybe we can make such behaviour configurable per-linter?

Copy link
Member

@w0rp w0rp May 12, 2019

Choose a reason for hiding this comment

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

I think a new option is appropriate, but it should be off by default.


" This Dictionary configures the default LSP roots for various linters.
let g:ale_lsp_root = get(g:, 'ale_lsp_root', {})

Expand Down