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.

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

Jump to bottom

Denite support #15

Open
DancingQuanta opened this issue Nov 14, 2017 · 42 comments
Open

Denite support #15

DancingQuanta opened this issue Nov 14, 2017 · 42 comments

Comments

@DancingQuanta
Copy link
Contributor

Add support for denite.nvim which supersedes unite.vim.

I am looking at how denite.nvim works and see if I can do a pull request.
However since citation.vim is a source for unite.vim it may need a rewrite for denite.nvim.
Should citation.vim-like source for denite.nvim be a separate plugin?
@rafaqz
Copy link
Owner

rafaqz commented Nov 14, 2017 •
edited
Loading

It should be relatively simple to support both as its already in python - it can hook in directly skipping the vimscript component, if I understand the new plugin system correctly.

The bulk of code is in the bibtex/zotero database loading and parsing and it would be pretty wasteful to split that over two plugins (and have to update eg. the new betterbibtex format twice). I'm pretty sure only citation.py and loader.py have any knowledge of vim at all, it just a question of formatting the returned list of items in builder.py for consumption by denite.

It's been on my list but the documentation just wasnt there last time I looked, + I'm very busy, so a pull request would really be appreciated.

@DancingQuanta
Copy link
Contributor Author

DancingQuanta commented Nov 15, 2017 via email

@rafaqz
Copy link
Owner

rafaqz commented Nov 15, 2017

Yep that is a question. Hopefully a vim.eval("g:citation_vim_bibtex_file") kind of thing is possible, then we don't have to change much. Other issues will include updating color highlighting and hooking in commands for file/url opening and showing combined info.

I'm imagining we split out unite and denite frontends from builder.py and everything deeper than that can stay as is.

@DancingQuanta
Copy link
Contributor Author

Two options for configuration

  • Old works for unite
vim.eval("g:citation_vim_bibtex_file")
  • New
vim.vars.get('citation_vim_bibtex_file', 'path'),

which requires the following configuration in vimrc

call denite#custom#var('citation.vim', 'citation_vim_bibtex_file', 'path')

I have not yet found any dual unite/denite sources so with my limited understanding of unite/denite, I am unsure what to do in order to share configuration between unite and denite.
I actually prefered functions to variables in vimscript as I don't really understand g: stuff..

@DancingQuanta
Copy link
Contributor Author

Actually, denite works this way regarding configuration.
In source

self.vars = {
    'mode': 'zotero',
    'zotero_version': 4,
    'zotero_path': '~/Zotero'
    ...
}

only require configuration in vimrc for example

call denite#custom#var('citation.vim', 'mode', 'zotero',)
call denite#custom#var('citation.vim', 'zotero_version', 4)
call denite#custom#var('citation.vim', 'zotero_path', '~/Zotero')

@rafaqz
Copy link
Owner

rafaqz commented Nov 17, 2017

That looks good - if understand you we wont have to communicate with vim at all - just access self.vars directly in python?

It does mean everyone will have to convert their variable configuration to that format to use denite, but its only five or so lines.

@rafaqz
Copy link
Owner

rafaqz commented Nov 17, 2017 •
edited
Loading

The x:var_name situation defines variable in a particular scope (as far as I understand it) g: is just a globally scoped variable, as v: is an argument variable, s: is script level? etc.

Finding another plugin conversions to follow was an issue for me too, this should be pretty formulaic and there are a lot of plugins needing conversion...

@DancingQuanta
Copy link
Contributor Author

DancingQuanta commented Nov 17, 2017 •
edited
Loading

plot of plugins needing conversion

Please explain that part? Are you saying that there is a number of unite plugins needing conversion?

@DancingQuanta
Copy link
Contributor Author

I found a useful example of highlight and syntax here.
This is a source for grep by Shougo.
You need to use two functions define_syntax and highlight to control the syntax of output.
In grep source's case, the functions appears to execute vimscript so it may be trivial to copy the syntax region and syntax match stuff to denite source of citation.vim.

@DancingQuanta
Copy link
Contributor Author

DancingQuanta commented Nov 17, 2017 •
edited
Loading

Thinking about global variables. The current unite implementation declare the global variables with defaults.
For denite, two options similar to configuration above

  1. Should a vimscript file declare these global variables and the python source executes vim.vars.get(variable) to get these variables to work with?
  2. Or store defaults in python source?

The first option allows the unite and denite to share the defaults.

@DancingQuanta
Copy link
Contributor Author

Sorry, I missed one of your comments regarding converting configuration to denite. This will only happen to users who use denite and so should make sense to them.

My idea is to change Context() class to be BaseContext() and copy Loader to UniteLoader. UniteLoader will inherit BaseContext() and add functions to extract relevant information through vim calls. BaseContext() will be used by denite sources.

@rafaqz
Copy link
Owner

rafaqz commented Nov 17, 2017 •
edited
Loading

Sorry I meant there are a lot of plugins that need to go through this same unite -> denite conversion process we are discussing so there should be a few examples around documenting the process.

It would be nice to go with option 1. and pull the vars in from vim globals so you can swap unite/denite smoothly, unless option 2 is vastly cleaner and simpler.

I was vaguely hoping there would be a syntax highlighting method without regex... we're building the description string from known separate strings then using regex to split it up again! for any other application it would be an insane strategy. But if the conversion is easy that's fine too.

The Context/UniteLoader idea sounds right. Context could have pre-defined fields too, its purpose is quite opaque right now. But you might need to split out some of Buillder() as well depending on the format denite wants data in. build_source() currently returns a list of arrays of length 4 built by item_to_array() from Item() objects, for consumption in vimscript. Another process might make more sense for denite.

@DancingQuanta
Copy link
Contributor Author

DancingQuanta commented Nov 17, 2017 via email

@DancingQuanta
Copy link
Contributor Author

Found dual unite/denote sources

@DancingQuanta
Copy link
Contributor Author

https://github.com/lervag/vimtex

@rafaqz
Copy link
Owner

rafaqz commented Nov 18, 2017

Great, looks fairly simple.

@DancingQuanta
Copy link
Contributor Author

I suggest creating a dev branch for denite support and so I can push to it if I have time.

@rafaqz
Copy link
Owner

rafaqz commented Dec 11, 2017

Can you work in a branch in your own repository? I shouldn't need to manage a dev branch here. I will pull it to master when it's working, but feel free to put in a pull request before that for feedback.

@NikosAlexandris
Copy link

What is the status regarding Denite?

@rafaqz
Copy link
Owner

rafaqz commented Jul 17, 2018

@DancingQuanta ?

I haven't worked on this because unite is still working fine, and I'm flat out with modelling projects and other work.

I'm keen for a pull request if anyone writes one. It shouldn't be major. See the discussions above, although I largely forget the details now. Otherwise I'll eventually do it when unite dies from neglect, or I have some spare time for this, but the first is more likely to be honest.

@DancingQuanta
Copy link
Contributor Author

Totally forgot about this. I like to try to do a pull request soon.

@NikosAlexandris
Copy link

Thank you both for responding.

@DancingQuanta
Copy link
Contributor Author

From what I can tell, denite.nvim loads new sources via importlib's SourceFileLoader.load_module(). This means one source per file.
This gives me two options;

  • either I write a source for each of the bibliographic fields with a name like citation/[field] args, or
  • use arguments like this citation [field] args. The builder will check that the field is valid.

@rafaqz
Copy link
Owner

rafaqz commented Jul 25, 2018 •
edited
Loading

Yeah lets not write them all out. I used code generation in vimscript to avoid that kind of duplication originally, really the only reason there are so many sources - adding them only takes a few lines.

It seems like a poor design choice to have sources allocated at the file level instead of as some kind of hooks that you can trigger multiple times programatically within one specific file, but we will have to work with that. The args seem to be a good idea if it the syntax will be clean and easy.

Edit: Summarise - the second option seems best

@DancingQuanta
Copy link
Contributor Author

I am inaccurate about the second option. The relevent denite.nvim documentation for the Denite command is

COMMANDS 						*denite-commands*

:Denite [{options}] {sources}				*:Denite*
		Creates a new Denite buffer.
		Denite can be invoked with one or more sources. This can be
		done by specifying the list on the command line, separated by
		spaces. The list of candidates (the matches found in the
		source by your filter string) will be ordered in the same
		order that you specify the {sources}.

		For example:

		:Denite file/rec line

		Will first list the files, then lines of the current buffer.

		See also |denite-sources| the available sources.

		Denite can accept a list of strings, separated with ":", after
		the name of sources.  You must escape ":" and "\" with "\"
		in parameters themselves, or surround the parameter with quotes.

		Examples:
		"file/rec:foo:bar": the parameters of source file are
		                    ["foo", "bar"].
		"file/rec:foo\:bar": the parameter of source file is
		                     ["foo:bar"].
		"file/rec:'foo:bar'": the parameter of source file is
		                     ["foo:bar"].
		"file/rec:foo::bar": the parameters of source file are
		                     ["foo", "", "bar"].

		You can use evaluation cmdline by ``.
		Note: In the evaluation, The special characters(spaces,  "\"
		and ":") are escaped automatically.
>

Arguments are separated by : for each source.

@rafaqz
Copy link
Owner

rafaqz commented Jul 27, 2018

That still looks good to me. One thing to keep in mind is that arguments like that are already used for zotero database search. But they can just be tacked on after the source argument. Also citation_source will probably need to be in a separate source file, as its a different mechanism.

Just to be clear the option is that we otherwise have 20 something separate files, one for each source?
I do think the argument is better!

@DancingQuanta
Copy link
Contributor Author

DancingQuanta commented Jul 27, 2018 •
edited
Loading

I am thinking about replacing Context() with a dict because denite uses a lot of dicts and I do not want to map from a dict to Context(). This would mean rewriting the codebase.
The denite source allows you to specify a dict of variables with default and which a user can set a value from their vimrc. Also denite use a dict to store context information.

@rafaqz
Copy link
Owner

rafaqz commented Jul 28, 2018

Just keep in mind that I still use unite and am reasonably happy with my set of plugins, and probably a few hundred other people as well going on download stats... so still working in unite with no config changes is the most important thing. Other than that do what needs to be done!

@DancingQuanta
Copy link
Contributor Author

DancingQuanta commented Jul 28, 2018 •
edited
Loading

Yes, I will make to keep the unite source's functionality the same.

My plan is then to create two pull requests:

  • Change Context() to dict (I worked out a vim search and replace regex to do this should be quick).
  • Add denite source

This way I can make sure that each pull request is tested and ensure that unite source behaves as normally.

@rafaqz
Copy link
Owner

rafaqz commented Jul 28, 2018 •
edited
Loading

Thats a good idea. I have a testing suite to hammer it with, but it needs a zotero install with lots of specific data, bibtex files and does things like open pdfs so I never bothered to set up Travis. But I'll run it on your branch before merging each pull request.

@DancingQuanta
Copy link
Contributor Author

DancingQuanta commented Jul 28, 2018 •
edited
Loading

I don't think Travis-ci citation.vim needs zotero installation. It only needs the databases?

@DancingQuanta
Copy link
Contributor Author

I have done the first PR. Please review on your end.

@rafaqz
Copy link
Owner

rafaqz commented Jul 28, 2018

It also uses the better-bibtex database, and probably neither of those should be in this repo. Mostly I couldn't see the return benefits of setting it up properly vs just running a script locally. Setting up Travis would probably take a day or two. And then the better bibtex format can change unexpectedly, so you want to be actually updating that and testing the latest...

PR is merged.

@DancingQuanta
Copy link
Contributor Author

Yeah, the better bibtex is unstable because the Zotero 5 is badly designed which makes using keys difficult. The better bibtex database is a temporary solution. Oh well.

@rafaqz
Copy link
Owner

rafaqz commented Jul 29, 2018

Ok wow. I have tried my best not to look into any of the reasons for that weirdness, but I'm glad you understand it. This plugin is exposed to way too much of that, with unite, bibtex, vimscript, zotero, better-bibtex, python unicode hell. now denite, etc etc. I'm constantly surprised at how stable it is.

@DancingQuanta
Copy link
Contributor Author

DancingQuanta commented Jul 29, 2018 via email

@DancingQuanta
Copy link
Contributor Author

Looking at citation_collection source, are these collections organised as in flat structure or nested? I seems to get flat structure in my invocation of Unite citation_collection. I have been wondering about action__path and action__directory for citation_collection. What are they supposed to do?

@rafaqz
Copy link
Owner

rafaqz commented Jul 29, 2018

They are nested in zotero, but are listed flat in unite. Selecting one will get you everything from there down in the tree. I think the database query gets all parent collections for every field, so any of them will work.

I can't remember what action__path or action__directory do there. I remember that set of commands not necessarily being limited to doing what you would think they do, but its a few years ago now. It really should have had a comment. This was the first vim plugin I wrote and I'm sure there is some cruft, especially in the vimscript...

@rafaqz
Copy link
Owner

rafaqz commented Jul 29, 2018 •
edited
Loading

action__path is connected to the start action which is mostly for opening pdfs or urls. So that's not needed at all. action__text is for yank etc which is vaguely useful, but action__directory is probably also completely useless. You may as well delete them.

I just realised how little I documented things in there, hope it's generally legible enough...

@DancingQuanta
Copy link
Contributor Author

Haha, I have mostly figured things out.

I will not be able to continue working on this until 10th of August.

@DancingQuanta
Copy link
Contributor Author

DancingQuanta commented Aug 10, 2018 •
edited
Loading

I have made a working PR #19, I am going to test it more rigorously soon.

EDIT: Still buggy with regards to actions. I need to see variety of environments of other users to understand the behavour of my code. I was trying to open files etc.

@ScheiklP
Copy link

ScheiklP commented Jul 2, 2020

Are you still working on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@rafaqz @NikosAlexandris @DancingQuanta @ScheiklP