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

[neovim] PlugUpdate/Install as start commands don't show any UI #278

Closed
choco opened this issue Sep 7, 2015 · 35 comments · Fixed by neovim/neovim#3321
Closed

[neovim] PlugUpdate/Install as start commands don't show any UI #278

choco opened this issue Sep 7, 2015 · 35 comments · Fixed by neovim/neovim#3321

Comments

@choco
Copy link

choco commented Sep 7, 2015

Hello, a couple of days ago I updated vim-plug to the latest version.
Today I tried to run the usual nvim +PlugUpdate and even though the
update is performed correctly the UI isn't updated/shown until the
process is finished. The same happens when running PlugInstall.

Everything works correctly if I execute both commands when neovim
has already started.

The issue seems to have been introduced with #227 but I don't
understand if it's something expected with that change, or I'm experiencing a bug.

@vheon
Copy link
Contributor

vheon commented Sep 7, 2015

Today I tried to run the usual vim +PlugUpdate and even though the

Everything works correctly if I execute both commands when neovim

So its vim or neovim??

@choco choco changed the title PlugUpdate/Install as start commands don't show any UI [neovim] PlugUpdate/Install as start commands don't show any UI Sep 7, 2015
@choco
Copy link
Author

choco commented Sep 7, 2015

Neovim, sorry for the typo :(

@junegunn
Copy link
Owner

junegunn commented Sep 7, 2015

Running the latest neovim?

@choco
Copy link
Author

choco commented Sep 8, 2015

Yeah, latest neovim version + latest neovim python-client.
I can reproduce the bug both on latest OS X version and Ubuntu 14.04,
even with a minimal vimrc:

call plug#begin('~/.vim/plugged')

" some plugins just so it takes sometime to fetch them
Plug 'tpope/vim-repeat'
Plug 'tpope/vim-surround'
Plug 'tpope/vim-unimpaired'

call plug#end()

To be clear vim isn't affected by this problem.

@junegunn
Copy link
Owner

junegunn commented Sep 8, 2015

Could be a neovim issue. It worked for me, until I updated neovim this morning (brew reinstall neovim --HEAD). Too bad I forgot to write down the version before upgrading it :(

@choco
Copy link
Author

choco commented Sep 8, 2015

Just did a rapid check and found out that this is the commit in neovim causing the bug in vim-plug neovim/neovim@7475c1c and the respective PR neovim/neovim#2846

The first comment to the first diff is probably the reason why it doesn't work.

@choco
Copy link
Author

choco commented Sep 8, 2015

Oh god I fell stupid, this makes more sense

diff --git a/plug.vim b/plug.vim
index 9eb11fc..abef6eb 100644
--- a/plug.vim
+++ b/plug.vim
@@ -1069,6 +1069,7 @@ class Buffer(object):
   def __init__(self, lock, num_plugs, is_pull, is_win):
     self.bar = ''
     self.event = 'Updating' if is_pull else 'Installing'
+    self.firstTime = True
     self.is_win = is_win
     self.lock = lock
     self.maxy = int(vim.eval('winheight(".")'))
@@ -1121,7 +1122,13 @@ class Buffer(object):
           lnum = 3
       else:
         lnum = 3
-      curbuf.append(msg, lnum)
+
+      if self.firstTime:
+        curbuf.append("", lnum-1)
+        curbuf.append(msg, lnum-1)
+        self.firstTime = False
+      else:
+        curbuf.append(msg, lnum)

       self.header()
     except vim.error:

@starcraftman
Copy link
Collaborator

@choco Interesting.I just tried nvim +PlugUpdate with latest neovim and buffer showed fine. My nvim --version.

NVIM 0.0.0-alpha+201509080123 (compiled Sep  8 2015 10:44:34)
Commit: dc9652e68de163290abee880a74bf1727c715a1e
Build type: RelWithDebInfo
Compilation: /usr/lib/ccache/cc -Wconversion -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 -Og -g -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -fstack-protector --param ssp-buffer-size=4 -DINCLUDE_GENERATED_DECLARATIONS -DHAVE_CONFIG_H -I/home/starcraftman/nv/src/neovim/build/config -I/home/starcraftman/nv/src/neovim/src -I/home/starcraftman/nv/src/neovim/.deps/usr/include -I/home/starcraftman/nv/src/neovim/.deps/usr/include -I/home/starcraftman/nv/src/neovim/.deps/usr/include/luajit-2.0 -I/home/starcraftman/nv/src/neovim/.deps/usr/include -I/home/starcraftman/nv/src/neovim/.deps/usr/include -I/home/starcraftman/nv/src/neovim/.deps/usr/include -I/home/starcraftman/nv/src/neovim/.deps/usr/include -I/usr/include -I/home/starcraftman/nv/src/neovim/build/src/nvim/auto -I/home/starcraftman/nv/src/neovim/build/include
Compiled by starcraftman@cosmos

Optional features included (+) or not (-): +acl   +iconv    +jemalloc 
For differences from Vim, see :help vim-differences

   system vimrc file: "$VIM/nvimrc"
     user vimrc file: "~/.nvimrc"
 2nd user vimrc file: "~/.nvim/nvimrc"
      user exrc file: "~/.exrc"
  fall-back for $VIM: "/home/starcraftman/nv/share/nvim"

I don't know what's going on with you guys. I am using my dev box to do that test, maybe it has some local modification. Downloading a vagrant image to do an isolated test.

Regarding your last message, is that a proposed patch you find resolves this bug?

@starcraftman starcraftman self-assigned this Sep 8, 2015
@junegunn
Copy link
Owner

junegunn commented Sep 8, 2015

@choco Thanks, but it doesn't seem to fix the problem for me. Hmm.
By the way, is this really something "we" should fix? :PlugInstall works just fine unless we start it from the command line. EDIT: Okay, my bad. :PlugInstall after startup doesn't use python installer.

@junegunn
Copy link
Owner

junegunn commented Sep 8, 2015

@choco @starcraftman
Geez, I'm sorry man, I just realized that I had a symlink to different version of plug.vim in my ~/.nvim/autoload directory. The patch does fix the problem :)

@starcraftman
Copy link
Collaborator

@junegunn Can you see if problem reproduces outside of brew? I mean, just clone neovim & build with make. When I do that, and then ./build/bin/nvim +PlugUpdate looks fine (except for inevitable syntax path issue). I find it strange that I can't reproduce.

@choco Are you also using brew?

@junegunn
Copy link
Owner

junegunn commented Sep 8, 2015

This is the exception raised from curbuf.append(msg, lnum)

Traceback (most recent call last):
  File "<string>", line 101, in write
  File "/usr/local/lib/python2.7/site-packages/neovim/api/buffer.py", line 120, in append
    return self._session.request('buffer_insert', self, index, lines)
  File "/usr/local/lib/python2.7/site-packages/neovim/api/common.py", line 219, in request
    return walk(self._in, self._session.request(name, *args), self, name,
  File "/usr/local/lib/python2.7/site-packages/neovim/msgpack_rpc/session.py", line 81, in request
    raise self.error_wrapper(err)
NvimError: Index out of bounds
  • len(curbuf) == lnum == 3
  • :python import vim; vim.current.buffer.append("hello", len(vim.current.buffer))
    • works fine on Vim
    • but fails on Neovim, gives NvimError('Index out of bounds',)
      • Need to decrement len by one to make it work.

The behavior is inconsistent with Vim, so it definitely looks like a bug of nvim, doesn't it?

@choco
Copy link
Author

choco commented Sep 8, 2015

@starcraftman Yes I'm using brew, but I can reproduce it even by compiling by hand on a fresh vagrant image.

@junegunn I'm not sure they consider it a bug, reading the discussion on the pull request it seems to me that if we want to append text at the end of the buffer we have to use set_line_slice like this I think:

diff --git a/plug.vim b/plug.vim
index 9eb11fc..96cbce8 100644
--- a/plug.vim
+++ b/plug.vim
@@ -1121,7 +1121,11 @@ class Buffer(object):
           lnum = 3
       else:
         lnum = 3
-      curbuf.append(msg, lnum)
+
+      if lnum == len(curbuf):
+        curbuf.set_line_slice(-1, -1, False, True, msg)
+      else:
+        curbuf.append(msg, lnum)

       self.header()
     except vim.error:

But that function isn't available in vim.

@starcraftman
Copy link
Collaborator

@junegunn Ah yes, this looks familiar. I reported an inconsistency like this a long time ago to the python neovim provider. There was an off by 1 bug happening between my invocation (and what I expected vim to do) & the end result that was being received inside nvim.

Interesting note, reason I was still working was I wasn't using the python version. I made the mistake of testing nvim on my main dev box, that has a bug breaking the :py bridge. Kinda wished I'd added a debug command to just say which installer would be run.

Edit: I should probably do a fresh install, I think it is 3 upgrades old.

@choco Yes, I was afraid that was what had happened. I can see where they are coming from, but I wouldn't have broken such long standing behaviour of :py. Seems late in the game to say its technically correct.

I will test this new patch against my alternate machines nvim, seems reasonable if works.

@junegunn
Copy link
Owner

junegunn commented Sep 8, 2015

@choco :help python states

The buffer object methods are:
b.append(str) Append a line to the buffer
b.append(str, nr) Idem, below line "nr"

so I would say that it's a bug.

@junegunn junegunn added the neovim label Sep 8, 2015
@choco
Copy link
Author

choco commented Sep 8, 2015

@junegunn I'll open a issue there then and see what they think.

:help python helped me figure out that I'm just stupid, updated the possible fix without using set_slice function

diff --git a/plug.vim b/plug.vim
index 9eb11fc..96cbce8 100644
--- a/plug.vim
+++ b/plug.vim
@@ -1121,7 +1121,11 @@ class Buffer(object):
           lnum = 3
       else:
         lnum = 3
-      curbuf.append(msg, lnum)
+
+      if lnum == len(curbuf):
+        curbuf.append(msg)
+      else:
+        curbuf.append(msg, lnum)

       self.header()
     except vim.error:

@junegunn
Copy link
Owner

junegunn commented Sep 8, 2015

@choco The document does not say that it's not allowed to append below the last line, and it works just as expected in Vim. The real problem is the line number is off by one as @starcraftman pointed out. Try python import vim; vim.current.buffer.append("hello", 5) on a file that has more than 5 lines on Vim and Neovim and you'll notice the difference.

@starcraftman
Copy link
Collaborator

@choco The only stupid programmer, is the one who writes something and refuses to improve upon it. Pride cometh before the fall.

I can reproduce the bug now, but for some odd reason the patch doesn't fix the +PlugUpdate issue on my notebook. I'm willing to say it might be my messed up dev boxes though (as first time), I'm getting another vagrant box to try, first one wasn't right.

@junegunn I am disappointed that the off by 1 bug I reported back in April is still present.

@choco
Copy link
Author

choco commented Sep 8, 2015

@junegunn Oh god I didn't notice that.....

@starcraftman
I think I found where the bug is in neovim:

diff --git a/src/nvim/api/buffer.c b/src/nvim/api/buffer.c
index 12c97cf..ce3e64d 100644
--- a/src/nvim/api/buffer.c
+++ b/src/nvim/api/buffer.c
@@ -467,7 +467,7 @@ void buffer_insert(Buffer buffer,
                    ArrayOf(String) lines,
                    Error *err)
 {
-  buffer_set_line_slice(buffer, lnum, lnum, false, true, lines, err);
+  buffer_set_line_slice(buffer, lnum, lnum, true, false, lines, err);
 }

 /// Return a tuple (row,col) representing the position of the named mark

Can you guys apply this with the last patch for vim-plug up there and test if everything works?

@starcraftman
Copy link
Collaborator

@choco So I set up my clean Ubuntu 14.04 vagrant. Regarding your two different patches:

  1. For vim-plug: I can confirm your latest one does the job. However, it is missing a cosmetic space between the progress bar and the first plugin line. I imagine this is the off by one bug problem in buffer. Can fix it temporarily with:
diff --git a/plug.vim b/plug.vim
index 9eb11fc..97711a8 100644
--- a/plug.vim
+++ b/plug.vim
@@ -1073,6 +1073,8 @@ class Buffer(object):
     self.lock = lock
     self.maxy = int(vim.eval('winheight(".")'))
     self.num_plugs = num_plugs
+    if G_NVIM:
+      vim.current.buffer.append('')

   def _where(self, name):
     """ Find first line with name in current buffer. Return line num. """
@@ -1121,7 +1123,11 @@ class Buffer(object):
           lnum = 3
       else:
         lnum = 3
-      curbuf.append(msg, lnum)
+
+      if lnum == len(curbuf):
+        curbuf.append(msg)
+      else:
+        curbuf.append(msg, lnum)

       self.header()
     except vim.error:
  1. For buffer.c: with or without it, I still get an index out of bounds error using junegunn's test line:
    :python import vim; vim.current.buffer.append("hello", len(vim.current.buffer)) Unless I'm missing something.

@choco
Copy link
Author

choco commented Sep 8, 2015

@starcraftman

  1. What space is missing? here's my output, and I get the same in vim (with HEAD vim-plug)
    2015-09-08 21_54_21

  2. I think that's intended, they don't want you to append outside the bounds of the buffer. My fix only conforms to vim implementation when we try to append inside the bounds. That kinda makes sense I think.

@starcraftman
Copy link
Collaborator

@choco

  1. Weird, nevermind. Not sure what I did.

  2. Yup, inserting on an index works with that patch. Only within bounds as you say. I guess that's their new policy, but it should be put in the docs. I guess they might argue that vim's behaviour is the incorrect one though, permitting out of bounds. You should probably make a PR against neovim.

@junegunn
Copy link
Owner

junegunn commented Sep 8, 2015

@starcraftman Not sure if I want to have temporary workaround for the bug in our code even if it's trivial. And I don't think they should insist they can just change the behavior when the original behavior is universal across append(), if_python, and if_ruby (or any other language interface of vim I suppose).

@junegunn
Copy link
Owner

junegunn commented Sep 8, 2015

And note that :python import vim; vim.current.buffer.append("hello", len(vim.current.buffer) + 1) does fail on vim. So there's bound checking. It makes sense to me to be able to append something after the last line.

@starcraftman
Copy link
Collaborator

@junegunn No disagreement from me, I mainly use vim anyway. Then this ticket stays open until nvim gets a patch to conform to expected vim if_pyth behaviour.

@starcraftman starcraftman removed their assignment Sep 9, 2015
@starcraftman
Copy link
Collaborator

@choco I was wondering about state of this issue since your neovim PR seems merged. Is there more work to be done?

@choco
Copy link
Author

choco commented Oct 2, 2015

@starcraftman The PR I submitted fixed both the bugs described here, but I had to remove the fix for the append after the last line. They want me to implement that on the python side of things.
To do that I'll need to modify the python client to perform multiple atomic API requests in a single one. But I'm not a python guy and understanding the current implementation isn't super easy for me, so I'm actually having an hard time implementing a decent solution in the limited time I have...

@starcraftman
Copy link
Collaborator

@choco I see, not knowing python is definitely an impediment. As a python guy, I am not super impressed by their python-client. Seems like after putting it together quickly, it has become a non priority for the main devs.

If I get some time I may try and help with the PR. Been a bit busy on my own new python project. It might be easier for you to learn python with, I think it is laid out a bit better.

Python is pretty straightforward, many tutorials to learn from. The main gotcha most people find odd is decorators. See this tutorial for a quick rundown. They are useful, but hardly necessary and sometimes just complicate code.

@choco
Copy link
Author

choco commented Oct 6, 2015

@starcraftman Hey, thank you so much for your help :) Finally finished my exams session, so I'll be able to dive into Python in the next couple of weeks :D
I've always have a hard time finding the right resources, and having an expert advice on good material is always great! What tutorial always seem to lack is the best practises to follow, so when I understand more I'll surely check out your project to see how things should be put in place. Again, thanks for your advices :)

@starcraftman
Copy link
Collaborator

@choco Good to hear, hope you did well. Have fun with python, in my opinion one of the better languages I've used. If you have python questions, feel free to ping me from one of your repos or on freenode. We should probably keep any further comments here to the issue at hand.

@wyntau
Copy link

wyntau commented Oct 23, 2015

still have this problem in latest neovim

can anyone help me?

NVIM 0.0.0-alpha+201510191853 (compiled Oct 23 2015 10:33:00)
Commit: e38cbb93670272d0da15c60222a123b88ec55002
Build type: RelWithDebInfo
Compilation: /usr/local/Library/ENV/4.3/clang -Wconversion -O2 -g -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wvla -fstack-protector-strong -fdiagnostics-color=auto -DINCLUDE_GENERATED_DECLARATIONS -DHAVE_CONFIG_H -I/tmp/neovim20151023-59571-d912yv/build/config -I/tmp/neovim20151023-59571-d912yv/src -I/tmp/neovim20151023-59571-d912yv/deps-build/usr/include -I/tmp/neovim20151023-59571-d912yv/deps-build/usr/include -I/tmp/neovim20151023-59571-d912yv/deps-build/usr/include/luajit-2.0 -I/tmp/neovim20151023-59571-d912yv/deps-build/usr/include -I/tmp/neovim20151023-59571-d912yv/deps-build/usr/include -I/tmp/neovim20151023-59571-d912yv/deps-build/usr/include -I/tmp/neovim20151023-59571-d912yv/deps-build/usr/include -I/usr/local/opt/gettext/include -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include -I/tmp/neovim20151023-59571-d912yv/build/src/nvim/auto -I/tmp/neovim20151023-59571-d912yv/build/include
Compiled by Treri@Treri-MacBook-Air.local

Optional features included (+) or not (-): +acl   +iconv    +jemalloc
For differences from Vim, see :help vim-differences

   system vimrc file: "$VIM/nvimrc"
     user vimrc file: "~/.nvimrc"
 2nd user vimrc file: "~/.nvim/nvimrc"
      user exrc file: "~/.exrc"
  fall-back for $VIM: "/usr/local/Cellar/neovim/HEAD/share/nvim"

@starcraftman
Copy link
Collaborator

@treri Hi, for now start neovim normally and then issue PlugUpdate/PlugInstall. In order to be fixed, either the jobwait function would have to be patched in neovim to support the starting use case or the python neovim client needs to receive aforementioned patch that @choco described. The latter seems more likely, but I don't think either of us have worked on it.

@choco
Copy link
Author

choco commented Apr 1, 2016

neovim/neovim#4083 seems to have fixed this, can anyone else check so we can close this?

@wyntau
Copy link

wyntau commented Apr 2, 2016

@choco I installed the latest neovim/neovim@28d3def from homebrew, it had been fixed.
Maybe someone else should also have a try before this issue can be closed.

@starcraftman
Copy link
Collaborator

@choco @treri Thanks for notification, bit late to it. I can confirm that with latest nvim, neovim & plug.vim using the python installer via +PlugInstall works fine now. Happy to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants