Uses Vim's timer instead of CursorHold trigger #374

Kazy · 2017-02-02T18:01:56Z

Related issue: #373

ktonga

First of all thank you so much for taking the time to open this PR, it's really good stuff what you're doing here.

I've left a few inline comments but there is also one more general thing that we should consider, currently there is a big outgoing refactor (#367) which touches everywhere including the very same places you're modifying. So I think we should hold on this change until the refactor lands and then you should rebase your PR.

I have already added a comment about this, but I want to emphasize that we should keep the old implementation to support older versions of Vim and Neovim which don't count with this new feature.

Cheers,
Gaston.

ktonga · 2017-02-03T13:25:18Z

autoload/ensime.vim

@@ -43,14 +43,6 @@ function! ensime#au_buf_leave(filename) abort
    return s:call_plugin('au_buf_leave', [a:filename])
 endfunction

-function! ensime#au_cursor_hold(filename) abort


I think we shouldn't discard this implementation totally, we should use timers when +timers is present and fallback to this implementation otherwise.

Maybe we can encapsulate the scheduling implementation details inside Editor class as much as possible.

ktonga · 2017-02-03T13:30:07Z

ensime_shared/client.py

@@ -602,7 +603,7 @@ def unqueue_and_display(self, filename):
            self.editor.lazy_display_error(filename)
            self.unqueue()

-    def on_cursor_hold(self, filename):
+    def refresh(self, filename):
        """Handler for event CursorHold."""


This comment should be changed accordingly

tick might be an apt name for this?

ktonga · 2017-02-03T13:32:29Z

ensime_shared/editor.py

-        # http://vim.wikia.com/wiki/Timer_to_execute_commands_periodically
-        self._vim.command(r'call feedkeys("f\e")')
+        self._vim.eval("timer_start({}, 'EnRefreshMessages', {{'repeat': -1}})"
+                       .format(500))


Why the half of the time than before? Have you tried different values and this is the most suitable?
Also why the time as parameter and everything else inline?

It's based on nothing, I'll revert back. And the parameter is a left over from the updatetime config variable.

ktonga · 2017-02-03T13:33:10Z

ensime_shared/ensime.py

-        client.on_cursor_move(filename)
+    def refresh_messages(self, client):
+        filename = client.editor.path()
+        if filename.endswith(".scala"):


We also support java files

There are a few lines of code in fun_en_complete_func that could be factored out into a function for checking "is the buffer one that ensime-vim supports?". It uses Vim's notion of filetype, which I think is a better way to go than file extension.

ktonga · 2017-02-03T13:36:20Z

rplugin/python/ensime.py

-    @neovim.autocmd('CursorMoved', **autocmd_params)
-    def au_cursor_moved(self, *args, **kwargs):
-        super(NeovimEnsime, self).au_cursor_moved(*args, **kwargs)
+    @neovim.function('EnRefreshMessages', sync=True)


This is only for Neovime, you should also define this function for Vim in plugin/ensime.vim

ktonga · 2017-02-03T13:41:59Z

ensime_shared/client.py

@@ -170,6 +170,7 @@ def lazy_initialize_ensime():

                try:
                    self.ensime = self.launcher.launch()
+                    self.editor.start_timer()


I can't see this method implemented in Editor I think you just kept the old name cursorhold. Also I think that this method could use a more descriptive name, something like start_refresh_timer

Some uncommited modifications, my bad.

ktonga · 2017-02-03T13:44:09Z

ensime_shared/editor.py

-        # Keys with no effect, just retrigger CursorHold
-        # http://vim.wikia.com/wiki/Timer_to_execute_commands_periodically
-        self._vim.command(r'call feedkeys("f\e")')
+        self._vim.eval("timer_start({}, 'EnRefreshMessages', {{'repeat': -1}})"


You should keep a reference to the timer id returned by timer_start so it can be stopped when the plugin is disabled

This makes me think that each Editor instance shouldn't maintain its own timer as state. There could be a single timer reference as an attribute of Ensime. That's kind of the spirit of the todo comment just above this diff.

There can be more than one live instance of Editor—currently one is created for each EnsimeClient instance, meaning if you edit two ENSIME-enabled projects in one Vim session you'd end up with two timers. That could be changed (in fact #367 changes it, though I didn't think about that much), but I feel leery of having one Editor that needs to be treated as a singleton. As an abstraction it might make perfect sense that there is one editor, but practically speaking singletons get ugly. Running with the extensions idea of #366, it'd be nice if third-party code can simply do editor = Editor(vim) for UI conveniences, instead of there being some global. It currently has virtually no significant state.

Ensime is already effectively a singleton, so it's nice to keep stuff of this nature there as long as it feels like a reasonable design and not overloaded, I think. Thoughts?

ches · 2017-02-03T15:10:15Z

I haven't reviewed the changes closely yet, but just wanted to say that if this PR gets to a state that everyone is happy with quickly, then I can rebase #367 to include it. It's my fault that big PR has been open for so long so I'd rather deal with conflicts than ask @Kazy to do so.

Generally I love the idea of being able to use a proper timer instead of the hacky CursorHold stuff, but I guess we probably should keep around the old solution for when +timer isn't available. I agree with @ktonga that it would be nice to pull the behavior into an abstraction that has the same Python API regardless of what Vim feature is available to back it. That could be a boon for testing too.

Kazy · 2017-02-04T00:32:21Z

Thanks all for the comments. As explained in the issue, that was more a proof of concept than a proper PR. I'll implement your remarks in the next few days to get this PR in an acceptable state.

Kazy · 2017-02-04T09:40:01Z

I've put the logic about the periodic function into Ticker, responsible for starting the timer and calling tick on a client depending if +timers is present or not. Based on @ches comment, the ticker is only present in the main Ensime class, calls the tick method on each client with the current filename, and repeats the timer if needed.
Let me know what you think, in particular about the name (if you prefer to revert to simply refresh_messages) and about having all that logic in a separate class.

I've let the cursor hold and cursor move handler for now, so they trigger the tick method as well. I wanted to dynamically create the autocmd for those depending on the timer value, but 1) you can't define them after the Neovim plugin class has been created 2) I had issues depending when I called _vim.eval('timers'), making it hard to know where to initialize the ticker. I guess I ran into the race condition mentioned in the Ensime class comment.

A solution would be to define those purely in Vim, including starting the timer.

ktonga · 2017-02-05T12:37:29Z

ensime_shared/ensime.py


    @execute_with_client()
    def fun_en_complete_func(self, client, findstart_and_base, base=None):
        """Invokable function from vim and neovim to perform completion."""
-        current_filetype = self._vim.eval('&filetype')
-        if current_filetype not in ['scala', 'java']:
+        if not is_buffer_ensime_compatible(self._vim):


I think this is a good time to achieve what this TODO says and get rid of this unnecessary check

ktonga · 2017-02-05T12:38:16Z

ensime_shared/util.py

@@ -41,6 +41,11 @@ def extract_package_name(lines):
                break
        return package

+def is_buffer_ensime_compatible(_vim):


Please move this method to Editor, we are trying to reduce the dependency on the Vim object to only the editor module. This is the best for many reasons, but mostly for making the testing easier.

ktonga · 2017-03-05T23:44:12Z

Hey @Kazy are you still keen/able to wrap this PR up?

If you rebase it on latest master we maybe could ask the folks in the gitter channel to try the branch for a few days, I can test it on neovim myself.

Cheers.

Kazy · 2017-03-06T17:31:30Z

Yes sure, sorry for the delay. I should be able to do that in the next few days.

ktonga

Nice work, I've been using it and seems to be working, but in my case that's only for neovim (0.2.0-dev).
Let's sit on it for a while just in case the rest of the guys have something to add.

ktonga · 2017-03-13T00:21:14Z

plugin/ensime.vim

    autocmd BufLeave *.java,*.scala call ensime#au_buf_leave(expand("<afile>"))
-    autocmd CursorHold *.java,*.scala call ensime#au_cursor_hold(expand("<afile>"))
-    autocmd CursorMoved *.java,*.scala call ensime#au_cursor_moved(expand("<afile>"))
+    if !has('timers'):


You haven't done the same for neovim, is that on purpose?

I didn't find a way to conditionally define autocmd using Neovim's library. More precisely, neovim.autocmd decorator is used when the class (NeovimEnsime here) is loaded. You can't call it after the fact to define an autocmd.
A possibility would be to declare the CursorHold and CursorMoved autocmd in ensime.vim for Neovim and not in Python.

Ok @Kazy, I've been asking to the nvim guys what to do about it (start of chat) and we should be ok not defining auto commands when in nvim since timers have been around for an year now and people already switched to nvim like new stuff that's for sure :)
So let's remove cursorhold and friends from nvim as it is not acceptable to have both mechanisms running at the same time.
What you reckon? I can test it for a while after you change it.

Cheers.

Thanks for getting info on that ! It's been implemented. I had to create an autocmd on BufEnter that does nothing to trigger the creation of a client and to start the trigger.

ktonga · 2017-03-13T12:09:33Z

@Kazy I think it would be useful if you add a few logging (debug) lines, we could use a bit of info about what's going on, this kind of things tend to be difficult to troubleshot :)

With the removal of CursorHold and CursorMove autocmd, this is needed to start the connection to the Ensime server without having to execute a command manually.

ktonga · 2017-03-20T12:17:58Z

Thanks a lot @Kazy it's looking great. I'll give it a go for a couple of days.

Since it's not a trivial change I could use at least a second pair of eyes :) @ches @jvican anything to add? If you are using anything else other than Neovim it would be nice guys if you could try this branch for a while.

Let's sit on it for a few days and if everything goes fine we are ready to merge it.

Cheers.

ktonga · 2017-03-22T12:30:22Z

@Kazy bad news, for me in neovim the server is never starting but the plugin is activated tho, as in when I trigger a plugin action it tries to send the request but obviously nothing is happening as there is no server to handle it. Nothing on the logs that can give me a clue.

ktonga · 2017-03-22T12:57:28Z

Sorry 'bout the false alarm, it was an unrelated problem.

ktonga · 2017-03-25T13:34:29Z

Hey @Kazy I'm cool with this for Neovim, how was it going for you on Vim? If it went ok we could merge it.

Kazy · 2017-03-25T13:44:49Z

One of my last commit fixed the support for vim. I haven't used it extensively though, just tried a few commands.

ktonga · 2017-03-25T14:05:06Z

Are you keen to fix any bug that could come up? I'm keen to merge it already.

jvican · 2017-03-25T20:04:59Z

@ktonga Let's just merge it. If a bug comes up, we can fix it later down the road. This PR has been for too long in the queue 😄.

ktonga · 2017-03-26T00:28:29Z

Thank you so much @Kazy for this valuable contribution!

Kazy · 2017-03-26T10:47:44Z

Yes I'm available if bugs come up. Ping me if you see one that might be related !

ches · 2017-04-07T15:53:01Z

Thanks @Kazy, this turned out really nicely.

ktonga suggested changes Feb 3, 2017

View reviewed changes

ktonga reviewed Feb 5, 2017

View reviewed changes

Kazy added 4 commits March 12, 2017 18:50

Uses Vim's timer instead of CursorHold trigger

1a1f0b6

Addresses PR remarks.

7a86f9b

Removes duplicate function

19a6b52

Moves is_buffer_ensime_compatible into the Editor class

814a980

ktonga reviewed Mar 13, 2017

View reviewed changes

ktonga approved these changes Mar 13, 2017

View reviewed changes

Kazy changed the title ~~WIP: Uses Vim's timer instead of CursorHold trigger~~ Uses Vim's timer instead of CursorHold trigger Mar 13, 2017

Kazy added 2 commits March 20, 2017 09:36

Removes CursorHold and CursorMove autocmd for nvim

f4e157f

Trigger Ensime Client creation using BufEnter autocmd w/ timers

c10e011

With the removal of CursorHold and CursorMove autocmd, this is needed to start the connection to the Ensime server without having to execute a command manually.

Various fixes to make it work with vim

a20e1f3

ktonga merged commit bdc8d35 into ensime:master Mar 26, 2017

This was referenced Aug 8, 2017

Remove timer tick functions from public interfaces #403

Open

WIP: Remove server lifecycle management from EnsimeClient #367

Open

ches mentioned this pull request Oct 1, 2017

Browsing doc via EnDocBrowse results in error & EnDocUri does not work #407

Open

Uses Vim's timer instead of CursorHold trigger #374

Uses Vim's timer instead of CursorHold trigger #374

Uh oh!

Conversation

Kazy commented Feb 2, 2017

Uh oh!

ktonga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktonga Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ches commented Feb 3, 2017

Uh oh!

Kazy commented Feb 4, 2017

Uh oh!

Kazy commented Feb 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktonga commented Mar 5, 2017

Uh oh!

Kazy commented Mar 6, 2017

Uh oh!

ktonga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktonga commented Mar 13, 2017

Uh oh!

ktonga commented Mar 20, 2017

Uh oh!

ktonga commented Mar 22, 2017

Uh oh!

ktonga commented Mar 22, 2017

Uh oh!

ktonga commented Mar 25, 2017

Uh oh!

Kazy commented Mar 25, 2017

Uh oh!

ktonga commented Mar 25, 2017

Uh oh!

jvican commented Mar 25, 2017

Uh oh!

ktonga commented Mar 26, 2017

Uh oh!

Kazy commented Mar 26, 2017

Uh oh!

ktonga Feb 3, 2017 •

edited

Loading