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

[READY] Java support with asynchronous diagnostics and messages #2863

Merged
merged 3 commits into from
Feb 10, 2018

Conversation

puremourning
Copy link
Member

@puremourning puremourning commented Dec 23, 2017

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

  • I have read and understood YCM's CONTRIBUTING document.
  • I have read and understood YCM's CODE_OF_CONDUCT document.
  • I have included tests for the changes in my PR. If not, I have included a
    rationale for why I haven't.
  • I understand my PR may be closed if it becomes obvious I didn't
    actually perform all of these steps.

Why this change is necessary and useful

This change is required for a better user experience when using native
java support

This implements an asynchronous message system using a long-poll request
to the server.

The server provides an endpoint /receive_messages which blocks until
either a timeout occurs or we receive a batch of asynchronous messages.
We send this request asynchronously and poll it 4 times a second to see
if we have received any messages.

The messages may either be simply for display (such as startup progress)
or diagnostics, which override the diagnostics returned by
OnFileReqdyToParse.

In the former case, we simply display the message, accepting that this
might be overwritten by any other message (indeed, requiring this), and
for the latter we fan out diagnostics to any open buffer for the file in
question.

Unfortunately, Vim has bugs related to timers when there is something
displayed (such as a "confirm" prompt or other), so we suspend
background timers when doing subcommands to avoid vim bugs. NOTE: This
requires a new version of Vim (detected by the presence of the
particular functions used).

NOT_READY because:

Notes:

  • Part 3 (I think) of the Java support PRs. This one actually adds the minimal changes for working java support
  • There are about 2 or 3 other PRs to come to add things like automatic module imports, etc.

[Please explain in detail why the changes in this PR are needed.]


This change is Reviewable

@puremourning puremourning force-pushed the java-async-diagnostics branch 2 times, most recently from 2cc487c to c4840c8 Compare December 24, 2017 00:11
@codecov-io
Copy link

codecov-io commented Dec 24, 2017

Codecov Report

Merging #2863 into master will increase coverage by 0.31%.
The diff coverage is 99%.

@@            Coverage Diff             @@
##           master    #2863      +/-   ##
==========================================
+ Coverage   92.33%   92.64%   +0.31%     
==========================================
  Files          20       21       +1     
  Lines        1970     2054      +84     
==========================================
+ Hits         1819     1903      +84     
  Misses        151      151

@bstaletic
Copy link
Collaborator

Reviewed 9 of 10 files at r1, 1 of 2 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


README.md, line 3333 at r4 (raw file):

[mvn-project]: https://maven.apache.org/guides/getting-started/maven-in-five-minutes.html
[eclipse-project]: 
[gradle-projec]: https://docs.gradle.org/current/userguide/tutorial_java_projects.html

s/gradle-projec/gradle-project

Probably my doing.


autoload/youcompleteme.vim, line 64 at r4 (raw file):

  endif

  for poller in keys( s:pollers )

What about other timers of other plugins?
Won't they get in the way?

How were other plugins' timers not a roblem before?


autoload/youcompleteme.vim, line 883 at r4 (raw file):

  " running. This is particularly a problem for DebugInfo which the user
  " probably needs to spend more than 100ms looking at.
  call s:SuspendTimers()

Wasn't this fxed in newer vim?


python/ycm/buffer.py, line 76 at r4 (raw file):

      # or require extra config confirmation, even if we don't actually use the
      # diagnostics.
      self._parse_request.Response()

When do we not care about diagnotics?
Is that the case when LSP server returns a bunch of diagnostics for yet unopened files?


python/ycm/vimsupport.py, line 264 at r4 (raw file):

  # current window (by passing 0 as the window number)

  if buffer_number < 0:

Under what circumstances can buffer number be negative?
LSP server returning diagnostics for unopened files?


python/ycm/vimsupport.py, line 270 at r4 (raw file):

    return 0

  window_number = GetIntValue( "bufwinnr({0})".format( buffer_number ) )

What happens when the same buffer is open in multiple windows?


python/ycm/youcompleteme.py, line 372 at r4 (raw file):

  def UpdateWithNewDiagnosticsForFile( self, filepath, diagnostics ):
    bufnr = vimsupport.GetBufferNumberForFilename( filepath )
    if bufnr in self._buffers and vimsupport.BufferIsVisible( bufnr ):

If I'm reading this correctly, currently we can't differentiate between a non-visible buffer and one that has not been opened.


python/ycm/youcompleteme.py, line 388 at r4 (raw file):

      # - Use the QuickFix list to report project errors?
      # - Use a special buffer for project errors
      # - Put them in the location list of whatever the "current" buffer is

Location list of the current buffer seems reasonable.


Comments from Reviewable

@puremourning
Copy link
Member Author

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


README.md, line 3333 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

s/gradle-projec/gradle-project

Probably my doing.

Done. I also fixed the missing eclipse project link :)


autoload/youcompleteme.vim, line 64 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

What about other timers of other plugins?
Won't they get in the way?

How were other plugins' timers not a roblem before?

Well, yes. That's unfortunate, but I don't feel it's OK for us to go suspending other people's timers. If other people's timers break our stuff, then we can go and moan at them, and moan at vim-dev for the timer implementation causing such issues. For now, this is just a workaround so that our actual implementation isn't totally busted.

It's worth pointing out that this is necessary particularly for the messages request timer, because it runs continuously. Many other timers are one-shot, or short lived. This one fires 10 times a second all the time, which triggers these sorts of glitches way more consistently.]

TBH, this is a problem now, hence the bug reports that timers firing cause redraw and loss of command line/etc. info.


autoload/youcompleteme.vim, line 883 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Wasn't this fxed in newer vim?

No. At least not entirely.

It is/was busted in a number of ways. There were a number of patches, but that didn't fix this particular issue. I'll raise a new issue on GitHub with an example of the confirmation diaglog problem.

vim/vim#1844
vim/vim#2241
vim/vim#1768

As mentioned by Bram, we should actually probably consider changing all of these things to display in a buffer, rather than in the command line, but I don't see how we can make it work with the confirm dialog, etc. anyway.

TBH the async messages shouldn't use the command line either, it's just a handy place to put them, as there is more work required to use a special scratch buffer (and that uses up quite a lot of screen space).


python/ycm/buffer.py, line 76 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

When do we not care about diagnotics?
Is that the case when LSP server returns a bunch of diagnostics for yet unopened files?

Yeah, when there are diagnostics in other project files. This is essentially making sure that any FileReadyToParse exceptions are displayed (such as messages we might from time to time encode there).


python/ycm/vimsupport.py, line 264 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Under what circumstances can buffer number be negative?
LSP server returning diagnostics for unopened files?

I don't think it can, and there is no coverage in the tests of this, so I think I'm going to remove it. I suspect that winbufnr returns something sensible in that case anyway, so this is just dead code.


python/ycm/vimsupport.py, line 270 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

What happens when the same buffer is open in multiple windows?

bufwinnr returns the first window for the buffer (it's not 100% clear what first means, but lowest number seems likely). In practice this seems to be the top-leftmost one, but that's anecdotal. This seems a reasonable compromise, at least initially. I suspect there are improvements in this area as people use and complain about it ;)


python/ycm/youcompleteme.py, line 372 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

If I'm reading this correctly, currently we can't differentiate between a non-visible buffer and one that has not been opened.

Not opened = not in self._buffers (or at least that means we haven't triggered a FileReadyToParse event for it)
Not visible = bufwinnr returns -1 because bufwinnr only looks at the current tab page.

That's why both are required. We need there to be a Buffer object for it (to handle the diagnostics) and we also need it to be visible, otherwise we end up sticking these diagnostics as the current window's location list, which clobbers the existing list. This leads to a horrible experience of the location list popping from one set to another and ending on an arbitrary set.


python/ycm/youcompleteme.py, line 388 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Location list of the current buffer seems reasonable.

Yeah it does, but that's actually irksomely hard. As mentioned above what happens is something like:

  • Get diagnostics set {Y} for file X.java
  • X.java is open in current window, so we set the current windows' location list to {D} diags for X.java. All good.
  • Get diagnostics set {Y} for file Y.java
  • Y.java is not open anywhere (or not visible), so what do we do?

We could:

  • Set the current window's location list to {Y} diags, but that clobbers the {X} diagnostics for X.java, which is clearly wrong, because X.java is open in the window.
  • Append them to current window's location list, but that is problematic, because we don't know when to remove them, or replace them (we receive diagnostic asynchronous, per-file and never know when it is "complete"). We could maintain state, but that requires more engineering.
  • Drop them, which is what we do now. It happens to be simple, and work, and is roughly consistent with the C++ diagnostics being only per-TU (not full project).
  • Any number of other things I listed.

As I said, I think there is a lot of improvement in this area, and this comment is like a big TODO for the future. I think initially I don't want to loose too much (more!) time trying to get this diagnostics system perfect, rather ship something working and iterate on it based on feedback/complaints/insults.


Comments from Reviewable

@bstaletic
Copy link
Collaborator

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


README.md, line 3333 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Done. I also fixed the missing eclipse project link :)

Yeah, I remember me unable to decide what should the eclipse-project be.

Also, you forgot to push the fix.


autoload/youcompleteme.vim, line 64 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Well, yes. That's unfortunate, but I don't feel it's OK for us to go suspending other people's timers. If other people's timers break our stuff, then we can go and moan at them, and moan at vim-dev for the timer implementation causing such issues. For now, this is just a workaround so that our actual implementation isn't totally busted.

It's worth pointing out that this is necessary particularly for the messages request timer, because it runs continuously. Many other timers are one-shot, or short lived. This one fires 10 times a second all the time, which triggers these sorts of glitches way more consistently.]

TBH, this is a problem now, hence the bug reports that timers firing cause redraw and loss of command line/etc. info.

Just as I thought.
We definitely can't suspend others people's timers.


autoload/youcompleteme.vim, line 883 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

No. At least not entirely.

It is/was busted in a number of ways. There were a number of patches, but that didn't fix this particular issue. I'll raise a new issue on GitHub with an example of the confirmation diaglog problem.

vim/vim#1844
vim/vim#2241
vim/vim#1768

As mentioned by Bram, we should actually probably consider changing all of these things to display in a buffer, rather than in the command line, but I don't see how we can make it work with the confirm dialog, etc. anyway.

TBH the async messages shouldn't use the command line either, it's just a handy place to put them, as there is more work required to use a special scratch buffer (and that uses up quite a lot of screen space).

I thought it was fixed in newer vim version... So the timers are still somewhat broken on Vim. That's nice.


python/ycm/vimsupport.py, line 270 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

bufwinnr returns the first window for the buffer (it's not 100% clear what first means, but lowest number seems likely). In practice this seems to be the top-leftmost one, but that's anecdotal. This seems a reasonable compromise, at least initially. I suspect there are improvements in this area as people use and complain about it ;)

That sounds reasonable.


python/ycm/youcompleteme.py, line 388 at r4 (raw file):
Wow, that's a lot of problems for something that seemed like an obvious and straight-forward solution.

As I said, I think there is a lot of improvement in this area, and this comment is like a big TODO for the future. I think initially I don't want to loose too much (more!) time trying to get this diagnostics system perfect, rather ship something working and iterate on it based on feedback/complaints/insults.

Alright, that sounds like a good idea, I too wish that we don't dag the too long.
Especially considering what we currently do:

happens to be simple, and work, and is roughly consistent with the C++ diagnostics being only per-TU (not full project).


Comments from Reviewable

@puremourning
Copy link
Member Author

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


autoload/youcompleteme.vim, line 883 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I thought it was fixed in newer vim version... So the timers are still somewhat broken on Vim. That's nice.

Hmmm. I'm not able to trivially reproduce it anymore in a simple test, so perhaps there is something else afoot.


Comments from Reviewable

@puremourning
Copy link
Member Author

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


autoload/youcompleteme.vim, line 883 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Hmmm. I'm not able to trivially reproduce it anymore in a simple test, so perhaps there is something else afoot.

OK I can't report on latest Vim. I will try on the minimum supported Vim that we declare.


Comments from Reviewable

@puremourning
Copy link
Member Author

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


autoload/youcompleteme.vim, line 883 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

OK I can't report on latest Vim. I will try on the minimum supported Vim that we declare.

ROFL. In the min version we declare (v7.4.1578), timers don't work at all. Which means YCM doesn't work.

I tested with the 16.04 LTS version (v7.4.1689) and timers do work, and generally these things are ok, but it's not perfect (see the linked issues etc.).

Importantly, and this applied to all of our versions, our timer callback can call :echo, which does mess up the command line anyway, so suspending them where possible is a good plan.


Comments from Reviewable

@puremourning puremourning force-pushed the java-async-diagnostics branch 3 times, most recently from 3b57240 to 1f4c6f5 Compare December 26, 2017 16:06
@bstaletic
Copy link
Collaborator

Reviewed 1 of 1 files at r5, 1 of 4 files at r6, 1 of 1 files at r7.
Review status: 10 of 12 files reviewed at latest revision, 3 unresolved discussions.


autoload/youcompleteme.vim, line 883 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

ROFL. In the min version we declare (v7.4.1578), timers don't work at all. Which means YCM doesn't work.

I tested with the 16.04 LTS version (v7.4.1689) and timers do work, and generally these things are ok, but it's not perfect (see the linked issues etc.).

Importantly, and this applied to all of our versions, our timer callback can call :echo, which does mess up the command line anyway, so suspending them where possible is a good plan.

Okay, thanks for looking into this. I'm fine with suspending timers, but every time I see us changing the vimscript part I think of the lack of tests there.


Comments from Reviewable

@bstaletic
Copy link
Collaborator

Didn't expect 99% coverage, good job, @puremourning!

The only line not covered is in buffer.py, line number 76.


Review status: 10 of 12 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@zzbot
Copy link
Contributor

zzbot commented Jan 1, 2018

☔ The latest upstream changes (presumably #2866) made this pull request unmergeable. Please resolve the merge conflicts.

@bstaletic
Copy link
Collaborator

Shouldn't the title be changed to (at least) [RFC]?


Reviewed 2 of 4 files at r6, 1 of 2 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 6 of 6 files at r11.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@puremourning puremourning changed the title [NOT READY] Java support with asynchronous diagnostics and messages [READY] Java support with asynchronous diagnostics and messages Jan 24, 2018
@puremourning
Copy link
Member Author

puremourning commented Jan 24, 2018

Yes, this is ready for review.

@tracyone
Copy link

cool

@bstaletic
Copy link
Collaborator

Review status: all files reviewed at latest revision, 3 unresolved discussions.


autoload/youcompleteme.vim, line 47 at r1 (raw file):

      \   'receive_messages': {
      \     'id': -1,
      \     'wait_milliseconds': 100

This was originally 250ms. I was the one complaining that is too much because I assumed the delay in a simple project dignostics update I saw is because of this.

I apologise for not giving this another, proper test. I've just tried using comparing 100ms with 250ms timer for messages and, as much as I don't like to admit this, I don't see ay difference. So we should probably make this timer fire every 250ms again.


autoload/youcompleteme.vim, line 64 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Just as I thought.
We definitely can't suspend others people's timers.

You've implemented s:SuspendTimers() only after we chaged the receive_messages timer to fire every 100ms. Was this not needed before?


Comments from Reviewable

@puremourning
Copy link
Member Author

Review status: all files reviewed at latest revision, 3 unresolved discussions.


autoload/youcompleteme.vim, line 47 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

This was originally 250ms. I was the one complaining that is too much because I assumed the delay in a simple project dignostics update I saw is because of this.

I apologise for not giving this another, proper test. I've just tried using comparing 100ms with 250ms timer for messages and, as much as I don't like to admit this, I don't see ay difference. So we should probably make this timer fire every 250ms again.

I haven't noticed any issues with 100ms. We still only have one outstanding actual poll request, so I think we can keep it like this. We can tweak it if there are performance or other issues.


autoload/youcompleteme.vim, line 64 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

You've implemented s:SuspendTimers() only after we chaged the receive_messages timer to fire every 100ms. Was this not needed before?

It was needed before, I just hadn't realised it yet.


Comments from Reviewable

@bstaletic
Copy link
Collaborator

Review status: all files reviewed at latest revision, 3 unresolved discussions.


autoload/youcompleteme.vim, line 47 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I haven't noticed any issues with 100ms. We still only have one outstanding actual poll request, so I think we can keep it like this. We can tweak it if there are performance or other issues.

Alright, I'm fine with that.


autoload/youcompleteme.vim, line 64 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

It was needed before, I just hadn't realised it yet.

I thought so, just wanted to make sure.


Comments from Reviewable

@puremourning
Copy link
Member Author

Review status: all files reviewed at latest revision, 2 unresolved discussions.


python/ycm/client/messages_request.py, line 86 at r11 (raw file):

        PostVimMessage( notification[ 'message' ],
                        warning = False,
                        truncate = True )

This isn't working properly. I just hit a "Press enter to continue" when jdt.ls was building a gradle project.
Off-by-one somewhere?


Comments from Reviewable

@bstaletic
Copy link
Collaborator

Review status: all files reviewed at latest revision, 2 unresolved discussions.


python/ycm/client/messages_request.py, line 86 at r11 (raw file):

Previously, puremourning (Ben Jackson) wrote…

This isn't working properly. I just hit a "Press enter to continue" when jdt.ls was building a gradle project.
Off-by-one somewhere?

I've successfully debugged PostVimMessage(). PR will be ready at some point tonight.


Comments from Reviewable

@bstaletic
Copy link
Collaborator

Review status: all files reviewed at latest revision, 1 unresolved discussion.


python/ycm/youcompleteme.py, line 388 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Wow, that's a lot of problems for something that seemed like an obvious and straight-forward solution.

As I said, I think there is a lot of improvement in this area, and this comment is like a big TODO for the future. I think initially I don't want to loose too much (more!) time trying to get this diagnostics system perfect, rather ship something working and iterate on it based on feedback/complaints/insults.

Alright, that sounds like a good idea, I too wish that we don't dag the too long.
Especially considering what we currently do:

happens to be simple, and work, and is roughly consistent with the C++ diagnostics being only per-TU (not full project).

Considering that we've said that this is working, I'm inclined to sasy that this is :lgtm:.


Comments from Reviewable

@micbou
Copy link
Collaborator

micbou commented Feb 2, 2018

Reviewed 9 of 10 files at r1, 1 of 1 files at r2, 3 of 4 files at r6, 6 of 6 files at r11.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


README.md, line 297 at r11 (raw file):

  calling `./install.py`.
- Java support: install [JDK8 (version 8 required)][jdk-install] and add
  `--jdt-completer` when calling `./install.py`.

--java-completer.


README.md, line 366 at r11 (raw file):

  calling `./install.py`.
- Java support: install [JDK8 (version 8 required)][jdk-install] and add
  `--jdt-completer` when calling `./install.py`.

--java-completer.


README.md, line 461 at r11 (raw file):

  calling `install.py`.
- Java support: install [JDK8 (version 8 required)][jdk-install] and add
  `--jdt-completer` when calling `./install.py`.

--java-completer.


README.md, line 534 at r11 (raw file):

  calling `./install.py`.
- Java support: install [JDK8 (version 8 required)][jdk-install] and add
  `--jdt-completer` when calling `./install.py`.

--java-completer.


README.md, line 741 at r11 (raw file):

    - Java support: install [JDK8 (version 8 required)][jdk-install]. Navigate
      to `YouCompleteMe/third_party/ycmd/third_part/eclipse.jdt.ls` and run
      `./mvnw clean package`.`

These instructions need to be updated.


README.md, line 823 at r11 (raw file):

### Java

**NOTE**: Java support is currently experrimental. Please let us know your

experimental


README.md, line 1170 at r11 (raw file):

### Java Semantic Completion

**NOTE**: Java support is currently experrimental. Please let us know your

experimental


README.md, line 1297 at r11 (raw file):

check that the classpath is set correctly.

For anything else, [contact us](#contact). Java support is experrimental at

experimental instead of experrimental.


autoload/youcompleteme.vim, line 891 at r11 (raw file):

    endfor
  finally
    call s:ResumeTimers()

This is not doing what it's supposed to do. The finally clause is immediately called after echoing the debugging info so the timers are only paused during the request and the output will still disappear without user intervention if a timer kicks in and print a message. Same comment for the :YcmShowDetailedDiagnostic command. Honestly, I would completely remove the workarounds. Until we find a better solution (like using a scratch window to display messages), I don't think we have much choice but to live with those UI issues.


python/ycm/vimsupport.py, line 260 at r7 (raw file):

  # buffer that actually gets parsed.
  #
  # The heuristic we use is to determie any open window for a specified buffer,

determine


python/ycm/client/messages_request.py, line 23 at r7 (raw file):

from __future__ import absolute_import
from future import standard_library
standard_library.install_aliases()

Those two lines should be replaced with

# Not installing aliases from python-future; it's unreliable and slow.

python/ycm/tests/mock_utils.py, line 113 at r7 (raw file):

# TODO: In future, implement MockServerResponse and MockServerResponseException
# for syncronous cases when such test cases are needed.

synchronous.


python/ycm/tests/youcompleteme_test.py, line 672 at r2 (raw file):

                                                         *args  ):

  # This test simulates asyncronous diagnostic updates associated with a single

asynchronous


python/ycm/tests/youcompleteme_test.py, line 789 at r7 (raw file):

                                                      *args  ):

  # This test simulates asyncronous diagnostic updates which are delivered per

asynchronous


python/ycm/tests/youcompleteme_test.py, line 792 at r7 (raw file):

  # file, including files which are open and files which are not.

  # Ordered to ensure that the calls to to update are deterministic

Superfluous to.


Comments from Reviewable

@zzbot
Copy link
Contributor

zzbot commented Feb 10, 2018

☔ The latest upstream changes (presumably #2902) made this pull request unmergeable. Please resolve the merge conflicts.

puremourning and others added 2 commits February 10, 2018 16:29
This implements an asynchronous message system using a long-poll request
to the server.

The server provides an endpoint /receive_messages which blocks until
either a timeout occurs or we receive a batch of asynchronous messages.
We send this request asynchronously and poll it 4 times a second to see
if we have received any messages.

The messages may either be simply for display (such as startup progress)
or diagnostics, which override the diagnostics returned by
OnFileReqdyToParse.

In the former case, we simply display the message, accepting that this
might be overwritten by any other message (indeed, requiring this), and
for the latter we fan out diagnostics to any open buffer for the file in
question.

Unfortunately, Vim has bugs related to timers when there is something
displayed (such as a "confirm" prompt or other), so we suspend
background timers when doing subcommands to avoid vim bugs. NOTE: This
requires a new version of Vim (detected by the presence of the
particular functions used).
@puremourning
Copy link
Member Author

Review status: 5 of 11 files reviewed at latest revision, 15 unresolved discussions.


README.md, line 297 at r11 (raw file):

Previously, micbou wrote…

--java-completer.

Done.


README.md, line 366 at r11 (raw file):

Previously, micbou wrote…

--java-completer.

Done.


README.md, line 461 at r11 (raw file):

Previously, micbou wrote…

--java-completer.

Done.


README.md, line 534 at r11 (raw file):

Previously, micbou wrote…

--java-completer.

Done.


README.md, line 741 at r11 (raw file):

Previously, micbou wrote…

These instructions need to be updated.

Done.


README.md, line 823 at r11 (raw file):

Previously, micbou wrote…

experimental

Done.


README.md, line 1170 at r11 (raw file):

Previously, micbou wrote…

experimental

Done.


README.md, line 1297 at r11 (raw file):

Previously, micbou wrote…

experimental instead of experrimental.

Done.


autoload/youcompleteme.vim, line 891 at r11 (raw file):

Previously, micbou wrote…

This is not doing what it's supposed to do. The finally clause is immediately called after echoing the debugging info so the timers are only paused during the request and the output will still disappear without user intervention if a timer kicks in and print a message. Same comment for the :YcmShowDetailedDiagnostic command. Honestly, I would completely remove the workarounds. Until we find a better solution (like using a scratch window to display messages), I don't think we have much choice but to live with those UI issues.

It seems that the major bug this was avoiding no longer happens on my current vim. The issue was that it cleared the screen when any timer fired, even if it didn't write to the screen. This made particularly FixIt unusable because it would clear the screen we open to offer suggested actions.

i can no longer reproduce that, so i will remove the workaround.

we can always invest the time (as i know you have been looking at it) in a better solution than :echo in future.


python/ycm/vimsupport.py, line 260 at r7 (raw file):

Previously, micbou wrote…

determine

Done.


python/ycm/client/messages_request.py, line 23 at r7 (raw file):

Previously, micbou wrote…

Those two lines should be replaced with

# Not installing aliases from python-future; it's unreliable and slow.

Done.


python/ycm/tests/mock_utils.py, line 113 at r7 (raw file):

Previously, micbou wrote…

synchronous.

Done.


python/ycm/tests/youcompleteme_test.py, line 672 at r2 (raw file):

Previously, micbou wrote…

asynchronous

Done.


python/ycm/tests/youcompleteme_test.py, line 789 at r7 (raw file):

Previously, micbou wrote…

asynchronous

Done.


python/ycm/tests/youcompleteme_test.py, line 792 at r7 (raw file):

Previously, micbou wrote…

Superfluous to.

Done.


Comments from Reviewable

@micbou
Copy link
Collaborator

micbou commented Feb 10, 2018

:lgtm:

@zzbot r+


Reviewed 6 of 7 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@zzbot
Copy link
Contributor

zzbot commented Feb 10, 2018

📌 Commit 690a763 has been approved by micbou

@zzbot
Copy link
Contributor

zzbot commented Feb 10, 2018

⌛ Testing commit 690a763 with merge 0d49557...

zzbot added a commit that referenced this pull request Feb 10, 2018
[READY] Java support with asynchronous diagnostics and messages

# PR Prelude

Thank you for working on YCM! :)

**Please complete these steps and check these boxes (by putting an `x` inside
the brackets) _before_ filing your PR:**

- [x] I have read and understood YCM's [CONTRIBUTING][cont] document.
- [x] I have read and understood YCM's [CODE_OF_CONDUCT][code] document.
- [x] I have included tests for the changes in my PR. If not, I have included a
  rationale for why I haven't.
- [x] **I understand my PR may be closed if it becomes obvious I didn't
  actually perform all of these steps.**

# Why this change is necessary and useful

This change is required for a better user experience when using native
java support

This implements an asynchronous message system using a long-poll request
to the server.

The server provides an endpoint /receive_messages which blocks until
either a timeout occurs or we receive a batch of asynchronous messages.
We send this request asynchronously and poll it 4 times a second to see
if we have received any messages.

The messages may either be simply for display (such as startup progress)
or diagnostics, which override the diagnostics returned by
OnFileReqdyToParse.

In the former case, we simply display the message, accepting that this
might be overwritten by any other message (indeed, requiring this), and
for the latter we fan out diagnostics to any open buffer for the file in
question.

Unfortunately, Vim has bugs related to timers when there is something
displayed (such as a "confirm" prompt or other), so we suspend
background timers when doing subcommands to avoid vim bugs. NOTE: This
requires a new version of Vim (detected by the presence of the
particular functions used).

NOT_READY because:

- the submodule commit points at my repo and requires ycm-core/ycmd#857 to be merged
- my spider sense suggest i have more testing to do...

Notes:

- Part 3 (I think) of the Java support PRs. This one actually adds the minimal changes for working java support
- There are about 2 or 3 other PRs to come to add things like automatic module imports, etc.

[Please explain **in detail** why the changes in this PR are needed.]

[cont]: https://github.com/Valloric/YouCompleteMe/blob/master/CONTRIBUTING.md
[code]: https://github.com/Valloric/YouCompleteMe/blob/master/CODE_OF_CONDUCT.md

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2863)
<!-- Reviewable:end -->
@zzbot
Copy link
Contributor

zzbot commented Feb 10, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 0d49557 to master...

@zzbot zzbot merged commit 690a763 into ycm-core:master Feb 10, 2018
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