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

Added Denite Support #191

Merged
merged 7 commits into from
Apr 30, 2017
Merged

Added Denite Support #191

merged 7 commits into from
Apr 30, 2017

Conversation

0phoff
Copy link
Contributor

@0phoff 0phoff commented Jan 15, 2017

Requirements (please check off with 'x')

What does this Pull Request (PR) do?

Add support for the denite plugin

Any background context you can provide?

works the same way as the unite converter, but denite requires any addons to be coded in python so that's what I did!

Screenshots (if appropriate or helpful)

screenshot

abbr is what is displayed to the denite buffer.
It's intended use is to change how the source is displayed without changing the source candidates themselves.
@ryanoasis
Copy link
Owner

@0phoff Thanks for this by the way! Sorry for the delay 😄

@0phoff
Copy link
Contributor Author

0phoff commented Feb 12, 2017

Do I need to change anything for you to merge it in?
Let me know and I'll change it!

@ryanoasis
Copy link
Owner

@0phoff At a glance it looks good so nope I don't think you need to change anything 👍

I have just been away from the project since end of last year and still trying to get my dev environment back up to 100%.

I will let you know if I have any issues with the PR.

@ryanoasis ryanoasis added this to the v0.9.0 milestone Feb 25, 2017
@sodiumjoe
Copy link

sodiumjoe commented Mar 4, 2017

Hi, I just tried this branch and I'm running into a couple of issues:

  1. Performance on Denite file_rec is noticeably slower with this turned on.
  2. when I open vim straight into nerdtree with vim . I get this error:
Error detected while processing function nerdtree#checkForBrowse[9]..184[2]..185[17]..186[7]..149[9]..147[32]..260[4]..NERDTreeWebDevIconsRefreshListener[21]..WebDevIconsGetFileType
Symbol:
line   21:
E121: Undefined variable: g:WebDevIconsUnicodeDecorateFileNodesPatternSymbols
E116: Invalid arguments for function items(g:WebDevIconsUnicodeDecorateFileNodesPatternSymbols)
E15: Invalid expression: items(g:WebDevIconsUnicodeDecorateFileNodesPatternSymbols)
Error detected while processing function nerdtree#checkForBrowse[9]..184[2]..185[17]..186[7]..149:
line    9:
E171: Missing :endif

@0phoff
Copy link
Contributor Author

0phoff commented Mar 11, 2017

Hey,

It is normal that denite gets a little slower with this plugin! It has to fetch images for all filetypes after all!
I do wonder on what kind of projects you work? I worked on project with 100+ files and didnt find it to be that slow... You could maybe try to use Ag (the_silver_searcher) as your fuzzy finder if you dont have it already! (see denite for info on how to set it up)

About the other issue, thats probably a problem of load order of the plugins, make sure you load WebDevIcons as last plugin, but the issue might be in the coding and not your fault..
I never started vim with nerdtree which is why i never bumped into this error! I'll see what I can do 👍

@ryanoasis
Copy link
Owner

@0phoff Thanks for following up.

@sodiumjoe Yeah I also think that error seems unrelated to the branch but I haven't had a chance to test that out (having nerdtree open on load)

@sodiumjoe
Copy link

@0phoff thanks for the response!

  1. I remember vim-devicons slowing down unite, too, but not this much. I'm working in a project with ~1000 files, and I'm using ripgrep for search. And to be more specific, the initial search speed is fine, it's filtering speed on entering text into the prompt that's slow. It causes actual input lag 😢

  2. the issue with opening into nerdtree with $ nvim . doesn't happen on master, only on this branch.

@0phoff
Copy link
Contributor Author

0phoff commented Mar 12, 2017

Hmm not to sure about the input lag thing.. I though it was kinda the same with unite! I implemented this the same way as @ryanoasis did for unite, so I am guessing that it has to do with how denite implemented things.. Denite is usually faster than unite though, so that seems weird... (btw in the denite docs, the author says that ripgrep is slower than Ag.. Not sure if it is true but might be worth checking out)

As for the second error, that might be my fault.. I changed some things in how this plugins gets loaded
I call the initialize function on the VimEnter autocmd. In the docs they say that this causes the function to be called after the vimconfig (& thus plugins) have been sourced!
Probably that there lies the issue.. (I taught I would fix the issue of having to source this plugin as last by doing this, but apparently i broke stuff 🤡 )

@sodiumjoe
Copy link

I just tried with ag instead of rg, no noticeable difference. Maybe I'll take it up with Shougo once this merges. Or I'll just disable it for denite if I can't deal with it.

Happy to supply any debugging info for the nerdtree issue!

@petobens
Copy link

Is it possible to also include support for file_mru? Thanks!

@ryanoasis
Copy link
Owner

@0phoff Hey so first sorry for the delay (been busy with some other things and I had to rebuild my vim8 with only python 3 support -- since Denite requires Vim8 + Python3)


Testing

I have tested your PR a bit and it does add the glyphs however it always seems to use the default. This looks like it is because the abbr is the entire line and there is further logic to perform on the values (mostly the bufnr to get the actual filename to parse).

Your changes

screenshot from 2017-03-31 19-58-05

My additional changes (see gist diff)

Here is my changeset with the icons showing for the correct types:

screenshot from 2017-03-31 20-16-03

See changes here: VimDevIcons-PR191-feedback.diff (my working changes)

There are obviously some tweaks we probably want to do (add padding before glyph, retain the buffer number, etc.)

Looks like we need to revert back to the simple init: call s:initialize() I was getting the errors with or without opening NERDTree on load.

@ryanoasis
Copy link
Owner

@0phoff I am leaning toward merging this in and then making my changes I posted above (see the diff) for the next release (maybe this weekend)

@0phoff
Copy link
Contributor Author

0phoff commented Apr 17, 2017

@ryanoasis, Do as you want! I don't have to much time to work on this either (last month before I have to turn in my thesis...), so do what you need to do!

About your problem with the abbr. In my denite setup the only thing in this string is the path to the file, so I didn't give it much thought... Your solution is thus more appropriate!

One thing I was thinking about is the fact that we are calling the getFileTypeSymbol function in vim, through the self.vim.funcs functionality. I did this so I didn't have to duplicate code in python that was already in vimscript.
But now I don't think that is a good idea. Iirc, the python code gets executed in a separate process and uses IPC to communicate with the vim process. If we would rewrite the getFileTypeSymbol function in python, the filter might be quite a bit faster because it can perform all the computations itself an then report the results to the vim process. (The entire filter would be asynchronous, in stead of having to wait for the vim process all the time...)

@sodiumjoe
Copy link

ooh, i wonder if that would help my perf issue

@ryanoasis
Copy link
Owner

@0phoff Sure thanks I understand! 😄

Yeah a lot of the logic I added was more or less the same as was done for Unite.

@0phoff , @sodiumjoe Good point on the vim function calls 👍

I feel like things are really starting to log jam (mostly my fault). So I think for now what I'll do is incorporate my changes with yours and as long as it's working I will call it 'good enough' for now 😆 . However if the performance issue is really because of this then we can enhance it and re-implement the necessary functions in Python 🤔

ryanoasis added a commit that referenced this pull request Apr 29, 2017
* fixes load errors
* fixes display of icons in Denite
@ryanoasis ryanoasis merged commit c82ea41 into ryanoasis:master Apr 30, 2017
@sodiumjoe
Copy link

should i open a new issue for the perf issues?

@ryanoasis
Copy link
Owner

@sodiumjoe Yes feel free to do that 😄

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

Successfully merging this pull request may close these issues.

4 participants