Initial Telescope extension

kabouzeid commented 2 years ago

Been using this for myself. Let me know what you think :)

mickael-menu commented 2 years ago

Thanks for your contribution!

I tried to add your fork as a Vim plugin with:

Plug 'kabouzeid/zk-nvim'

But then running require("telescope").load_extension("zk") I get the following error:

Do you know what I might be missing in my config?

kabouzeid commented 2 years ago

It's currently only on the telescope branch.

Plug 'kabouzeid/zk-nvim', { 'branch': 'telescope' }

kabouzeid commented 2 years ago

Alternatively you can check out the repo locally and then use something like Plug '/my/plugin/path'

strange commented 2 years ago

Great to see some activity here! Feels like zk is gaining momentum and plugins for the most popular editors is imperative to its success.

I started looking into creating something similar, alas Christmas is our busiest time of the year, and I haven't found the time.

I'll take this for a spin during my lunch break!

This is how I designed a theoretical plugin on a napkin:

A command to open a telescope picker that can be filtered (title and tags is probably sufficient for a first version). I can press <C-n> to create a new note with the current filter as title (tags excluded). I can press <CR> to edit a selected file and I can navigate between files with <C-j> and <C-k>. <C-c> clears the filter state. If I run the command to open the picker again it remembers the last filter state (optional).
Preview-support is nice, but I often keep so many terminals open, that even on my 34" monitor, I rarely have space for more than 80 columns.
Basic LSP integration (complete tags, titles/paths when creating links etc).
A command that opens up a picker with all files that are related to the file open in the current buffer.

kabouzeid commented 2 years ago

Just a heads up, you need to build the latest (unreleased) version of zk from git for this to work, ie it doesn't work with zk 0.8.0.

Nothing needs to be done in this plugin for LSP integration, link/tag autocompletion etc already work out of the box with zk and lspconfig.

I've also included a "backlinks" picker which lists all notes that reference the currently open note.

mickael-menu commented 2 years ago

@kabouzeid Thanks, I was able to run your plugin, it's working great!

I'll wait to release the version 0.9 (after fixing https://github.com/mickael-menu/zk/issues/118#issuecomment-992864682) before merging your PR, to list a proper version requirement. I guess I'll do this this week-end.

There's also some initial work that has been done by @smithbm2316, @eric-hansen and @cljoly in some alternate branches or forks. I'm not sure what's the state of this, and if we need to merge your work together. Would you mind chiming in?

Great to see some activity here! Feels like zk is gaining momentum and plugins for the most popular editors is imperative to its success.

@strange Definitely, I think the LSP server is in a right shape to start building minimal plugins around it.

cljoly commented 2 years ago

There's also some initial work that has been done by @smithbm2316, @eric-hansen and @cljoly in some alternate branches or forks. I'm not sure what's the state of this, and if we need to merge your work together. Would you mind chiming in?

I made a telescope picker by calling the zk list command (it was before you added list to the lsp) and I think the solution of this PR is much better.

smithbm2316 commented 2 years ago

I made a telescope picker by calling the zk list command (it was before you added list to the lsp) and I think the solution of this PR is much better.

I've unfortunately been very caught up with work the last couple of weeks since I pushed some initial ideas/code to my branch a few weeks ago. We're at the end of a project lifecycle at the moment, so it's been a bit busier than I was hoping for me. @cljoly The require('zk').list() function that I wrote was meant to be an API wrapper around zk list so that people who don't use Telescope can still pump results from the zk list command into their favorite fuzzy finder in Neovim more easily. I hadn't gotten around to working on the Telescope extension that I was planning on at the time. I wanted to work on the API wrapper functions first before working on the Telescope-specific stuff.

@mickael-menu I just saw that you added the new LSP support for zk list which is so EXCITING!! I had written that require('zk').list() API wrapper before that was added, and the LSP method is most definitely the better way to go! I'm hoping to do some more work this weekend now that things have calmed down at work for me a bit to clean up that branch and get it ready for PR/review. Especially now that we have the zk list LSP command!

@kabouzeid thanks for submitting this, I can do a code review sometime this weekend if @mickael-menu or you would like me to before merging. I'll be excited to try out this branch tomorrow for myself 😄

cljoly commented 2 years ago

the require('zk').list() function that I wrote was meant to be an API wrapper around zk list

@smithbm2316 I guess the two could be complementary: on one hand, the tightly integrated LSP call to list with telescope and, on the other hand, the more free form call to zk list with a template, that might be easier to integrate with other finders.

cljoly commented 2 years ago

Thanks a lot for this plugin @kabouzeid! I’ve been able to get all the three commands to work and it’s great!

mickael-menu commented 2 years ago

Thanks for chiming in guys! Looks like we can merge this PR after addressing the tags issue and then build from there.

I found another issue for could be addressed in another PR: the preview window doesn't automatically wrap the content, so if you don't format your Markdown you can't see most of the note content.

By the way @mhanberg also did some great work in another repo. He uses fzf instead of Telescope, but I think the LSP communication layer could be shared, as well as some utilities for example related to colors.

(@mhanberg Do you have any interest in participating to this repository directly, or you prefer maintaining your custom plugin?)

I opened a new discussion to talk about the architecture and features of this plugin: https://github.com/mickael-menu/zk-nvim/discussions/6

Let me know if there are other LSP commands missing in zk. I guess it could be interesting to have access to the config layer.

mhanberg commented 2 years ago

By the way @mhanberg also did some great work in another repo.

thanks!

I think the LSP communication layer could be shared, as well as some utilities for example related to colors.

I agree

(@mhanberg Do you have any interest in participating to this repository directly, or you prefer maintaining your custom plugin?)

I would rather unify on a single plugin, so I'm open to contributing PRs and collaborating on ideas in this repository.

kabouzeid commented 2 years ago

@cljoly thanks for the review, you were right on all points :)

kabouzeid commented 2 years ago

I've added some documentation and made a whole plugin out of it.

Most notably this doesn't use lspconfig to manage the language server, and will now only launch a single instance of the zk server. The notebook path can be optionally provided so that one can run the Telescope pickers and other commands without having a note open.

I've also added API wrappers for index, new, list, and tag.list, which should make it easy to write e.g. an FZF picker.

I appreciate your feedback :)

cljoly commented 2 years ago

The notebook path can be optionally provided so that one can run the Telescope pickers and other commands without having a note open.

Just a random idea, it does not look like you use the global variable $ZK_NOTEBOOK_DIR, maybe its value could be used as a default if no notebook path is provided? (disclaimer: I’ve looked into this really quickly, I hope to test tomorrow).

kabouzeid commented 2 years ago

When no path is provided, the default is already the path of the current buffer.

I didn't know about this environment var. In this case I think a good solution would be to only use the path of the current buffer as default if it is a note in a notebook. Otherwise use $ZK_NOTEBOOK_DIR as default. It's an easy change, I'll do it when I'm back home.

kabouzeid commented 2 years ago

Actually no changes necessary. I just checked, and zk already automatically falls back to $ZK_NOTEBOOK_DIR, which is the correct behavior 🎉

mickael-menu commented 2 years ago

Thanks for tackling the Lua wrapper API! I assume you took a look at the architecture discussion?

Most notably this doesn't use lspconfig to manage the language server, and will now only launch a single instance of the zk server. The notebook path can be optionally provided so that one can run the Telescope pickers and other commands without having a note open.

That's an interesting take, is it to allow running the commands even if we are not inside a notebook?

I suggest we focus on merging your PR now to get the plugin quick-started from this great base and then build up more from there. Let me know when you're ready to merge.

All of this is very exciting!

kabouzeid commented 2 years ago

Thanks for tackling the Lua wrapper API! I assume you took a look at the architecture discussion?

Yes :) Right now the Telescope "adapter" is loosely oriented on the discussion. At least for now, I'd keep it as is. One could easily copy this code and replace the Telescope pickers with FZF pickers or vim.ui.select() calls.

That's an interesting take, is it to allow running the commands even if we are not inside a notebook?

Yes, exactly. And as a bonus, when opening multiple notebooks, a single zk server instance will be shared between them. Overall this simplified the code, since now the zk client doesn't need to be attached to the buffer, and we don't have to deal with duplicated responses because multiple servers are running.

When the current buffer is not within a notebook and the user did not explicitly specify the notebook location, it will fall back to $ZK_NOTEBOOK_DIR, just like with the zk cli. Meaning we can now use :Telescope zk notes (and every other command for that matter) from any buffer, and it'll do the right thing.

I suggest we focus on merging your PR now to get the plugin quick-started from this great base and then build up more from there. Let me know when you're ready to merge.

I agree. Once this is merged we can tackle the rest with more focused PRs, this one has already gotten out of hand 😅

As you suggested https://github.com/mickael-menu/zk-nvim/pull/7#issuecomment-998246206, I've now moved the higher level commands from zk.cmd to zk. I think this will clear up confusion on whether to use the commands in zk.api or in zk.cmd

I'm quite satisfied with the experience of the plugin now. Feel free to merge.

smithbm2316 commented 2 years ago

If you'd like to @kabouzeid, it might be nice to just grab lines 226-237 from this file so that @mickael-menu can get his jumping between note links in a file with this PR too 😄

cljoly commented 2 years ago

I’ve tested commit 8825b93 and it worked flawlessly for me. FWIW I was using a custom path to zk, like so:

require("zk").setup{
  lsp = {
    config = {
      cmd = { "/home/cj/ghq/github.com/mickael-menu/zk/zk", "lsp" },
      name = "zk",
      -- init_options = ...
      -- on_attach = ...
      -- etc, see `:h vim.lsp.start_client()`
    },
  },
}

kabouzeid commented 2 years ago

If you'd like to @kabouzeid, it might be nice to just grab lines 226-237 from this file so that @mickael-menu can get his jumping between note links in a file with this PR too smile

Hmm, I'm not quite sure about this one. I might have another language server attached which also provides diagnostics (think eg writegood or proselint via null-ls or efm). In that case this would not jump to the next link but simply to the next diagnostic hint.

Also, if we add this, then to be consistent it would also make sense to add

function preview_link()
  vim.lsp.buf.hover()
end

and so on, but it seems a bit redundant to me.

cljoly commented 2 years ago

If you'd like to @kabouzeid, it might be nice to just grab lines 226-237 from this file so that @mickael-menu can get his jumping between note links in a file with this PR too smile

Hmm, I'm not quite sure about this one. I might have another language server attached which also provides diagnostics (think eg writegood or proselint via null-ls or efm). In that case this would not jump to the next link but simply to the next diagnostic hint.

Also, if we add this, then to be consistent it would also make sense to add
function preview_link()
  vim.lsp.buf.hover()
end
and so on, but it seems a bit redundant to me.

I agree, plus at this point, it’s probably better to merge this PR and address those things in an other PR.

kabouzeid commented 2 years ago

Apparently it's possbile to filter by namespace in goto_next(), and we can get the zk namespace via h: vim.diagnostic.get_namespace() and h: vim.lsp.diagnostic.get_namespace(). The diagnostic severity then also needs to be user configurable, since it's configurable in the zk config. This should be in it's own PR though.

mickael-menu commented 2 years ago

It's actually small enough that I can bind a key directly to this for now, thanks for the idea @smithbm2316

Seems you're all happy about the current state, let's merge! Thank you @kabouzeid

smithbm2316 commented 2 years ago

If you'd like to @kabouzeid, it might be nice to just grab lines 226-237 from this file so that @mickael-menu can get his jumping between note links in a file with this PR too smile

Hmm, I'm not quite sure about this one. I might have another language server attached which also provides diagnostics (think eg writegood or proselint via null-ls or efm). In that case this would not jump to the next link but simply to the next diagnostic hint.

Also, if we add this, then to be consistent it would also make sense to add
function preview_link()
  vim.lsp.buf.hover()
end
and so on, but it seems a bit redundant to me.

Ah yeah I forgot that people might use multiple LSPs with zk-nvim. I do that for web development stuff, but just attach zk itself for my ZK notebook. I have a keybinding setup globally anytime I attach a LSP to a buffer to be able to jump between diagnostics myself. Glad it worked out for you to just copy and paste though @mickael-menu! I'm going to close #7 and we can continue discussion and further development on the excellent base you've already put together @kabouzeid 😄

zk-org / zk-nvim

Initial Telescope extension #5