mini.nvim icon indicating copy to clipboard operation
mini.nvim copied to clipboard

Beta-testing 'mini.bracketed'

Open echasnovski opened this issue 2 years ago • 44 comments

Please leave your feedback about new mini.bracketed module here. Feel free to either add new comment or positively upvote existing one.

Some things I am interested to find out (obviously, besides bugs):

  • Are configuration and function names intuitive enough?
  • Are default values of settings convenient for you?
  • Is documentation and examples clear enough?

Thanks!

echasnovski avatar Feb 21 '23 14:02 echasnovski

Long-time user of unimpaired, with bracketed I miss unimpaired-toggling . Also, c for comments collides with gitsigns advised shortcut for moving between hunks (mnemotechnic is change), not sure how to deal with that (besides overriding the shortcut or my muscle memory).

leiserfg avatar Feb 21 '23 20:02 leiserfg

  • Are configuration and function names intuitive enough?

Yes, function names and chosen suffixes align well with expectations and config is easy/intuitive; however, I expected bracketed to have tabs as t (which I already use), so I remapped treesitter to n, for now.

  • Are documentation and examples clear enough?

Yes, but ~~maybe add an explicit callout to the options and/or link each function in the table to the doc line.~~ I overlooked it at first (Update: and second glance, sigh. I'm referring to the README. The docs are good.

Oh, I was confused that ]i (indent forward) didn't go 1 level deeper (bigger indent) and instead took me 1 shallower (smaller indent). ~~Is this intentional? It's not clear from the docs.~~ Update: after reading more carefully and trying out change_types, it's clear.

  • Most excited by these targets:

comment, indent, treesitter, quickfix

Most importantly, echo'ing from reddit:

Always a pleasure to see a new mini module pop up. "Oh yeah, this should be good, gonna try this"

+1 to this sentiment 🍻

alexpw avatar Feb 21 '23 20:02 alexpw

  • As said above, ]c conflict with gitsign's "next code Change", even if I move gitsigns's to other key (]g "next Git change" for example), ]c still conflicts with the default ]c in vim's diff mode, which is used by other plugins like Diffview.nvim.
  • ]i is used by mini.indentscope by default.
  • May I suggest ]<TAB> for switching tab. I have been using that since forever. The default for tab gt, gT is not very ergonomic, and is often overrided by lsp go to type.

Thanks for your hard work.

lkhphuc avatar Feb 21 '23 22:02 lkhphuc

Long-time user of unimpaired, with bracketed I miss unimpaired-toggling .

@leiserfg you might want to checkout mini.basics, the toggling functions are in there.

lkhphuc avatar Feb 21 '23 22:02 lkhphuc

Not sure if I should leave feature requests here, but is there a way to jump between marks?

BlackEdder avatar Feb 22 '23 07:02 BlackEdder

Not sure if I should leave feature requests here, but is there a way to jump between marks?

]` is native of vim

leiserfg avatar Feb 22 '23 07:02 leiserfg

Long-time user of unimpaired, with bracketed I miss unimpaired-toggling .

@leiserfg, yes, option toggling is a vital part of workflow once you get used to it. As said above, this is a part of 'mini.basics' under mappings.option_toggle_prefix option. By default it creates mappings like \s for spell, etc., but prefix can be changed to be yo to match 'vim-unimpaired'.

To be honest, those kind of mappings are quite easy to replicate and customize manually in own config. Here is all the code from 'mini.basics'.

echasnovski avatar Feb 22 '23 08:02 echasnovski

Also, c for comments collides with gitsigns advised shortcut for moving between hunks (mnemotechnic is change), not sure how to deal with that (besides overriding the shortcut or my muscle memory).

As said above, ]c conflict with gitsign's "next code Change", even if I move gitsigns's to other key (]g "next Git change" for example), ]c still conflicts with the default ]c in vim's diff mode, which is used by other plugins like Diffview.nvim.

@leiserfg, @lkhphuc, prior to writing 'mini.bracketed' I assumed 'gitsigns.nvim' suggested ]h for "hunk" (and I sure it did at some point), so didn't touch h suffix. Unfortunately, this is a hard to solve problem of words starting with same letter.

I am coming from the view that comment target should be definitely a part of 'mini.bracketed'. As it seems to deserve more frequent usage than conflict, it gets the c suffix. Using c in 'gitsigns.nvim' as "code change" is a bit confusing in my opinion. Mostly because hunks is more descriptive and because there is also a built-in "change list" source (which I also contemplated to add as "go to next/previous change", but opted out because of suffix collision).

That said, suffixes are configurable for exactly this same reason so that people can tailor mappings to their liking.

echasnovski avatar Feb 22 '23 08:02 echasnovski

Yes, but maybe add an explicit callout to the options and/or link each function in the table to the doc line. I overlooked it at first.

Not sure what you mean. If you mean in README's default config, then it says look at :h MiniBracketed.config (where it describes options option and points to specific function; a bit convoluted, but yeah...). If you mean table in the start of README, it says "... see help for corresponding Lua function...". Directly linking is hard with vimhelp to the point of being almost impossible at the moment.

Oh, I was confused that ]i (indent forward) didn't go 1 level deeper (bigger indent) and instead took me 1 shallower (smaller indent). Is this intentional? It's not clear from the docs.

Yes, this is intentional. I trial-ran your suggestion a bit when developing it, but didn't find it extremely useful.

Documentation says to go to next/previous line with different indent. Which type of indent change is used can be configured with change_type option. Maybe using "diff" for "different indent" will be more useful to you?

echasnovski avatar Feb 22 '23 08:02 echasnovski

  • ]i is used by mini.indentscope by default.

@lkhphuc, yes it is. I still decided to include it in 'mini.bracketed', as it allowed for a slightly different behavior. See the differences. You can disable it in either one.

  • May I suggest ]<TAB> for switching tab. I have been using that since forever. The default for tab gt, gT is not very ergonomic, and is often overrided by lsp go to type.

This change seems trivial enough that it can be a part of user's config explicitly. But using gt/gT seems to be more ergonomic to me as they are very close on standard keyboard (while ] and <Tab> are quite literally the opposite :) ). So probably, won't be a part of 'mini.bracketed'.

echasnovski avatar Feb 22 '23 08:02 echasnovski

@leiserfg, @lkhphuc, prior to writing 'mini.bracketed' I assumed 'gitsigns.nvim' suggested ]h for "hunk" (and I sure it did at some point), so didn't touch h suffix. Unfortunately, this is a hard to solve problem of words starting with same letter.

They did it (I assume) to match ]c in diff-mode. What do you think of using ]k for comments instead?

leiserfg avatar Feb 22 '23 08:02 leiserfg

@leiserfg, @lkhphuc, prior to writing 'mini.bracketed' I assumed 'gitsigns.nvim' suggested ]h for "hunk" (and I sure it did at some point), so didn't touch h suffix. Unfortunately, this is a hard to solve problem of words starting with same letter.

They did it (I assume) to match ]c in diff-mode. What do you think of using ]k for comments instead?

Ah, I see. It does make sense.

Don't know why I didn't even think about using k for comments... Might have been a good idea.

Probably, won't change now, as I still think that ]c is not the best choice for any Git/SCM/patch related target. Besides, suffixes are made configurable for a reason.

echasnovski avatar Feb 22 '23 08:02 echasnovski

I see. Since ]c stills override vim's default diff mode even if I change gitsigns mapping, I settle for opts = { comment = { suffix = "gc" } },. This way it consistent with dgc and gcgc mappings from mini.comment. Just want to put this here if someone has the same concern.

lkhphuc avatar Feb 22 '23 13:02 lkhphuc

I see. Since ]c stills override vim's default diff mode even if I change gitsigns mapping, I settle for opts = { comment = { suffix = "gc" } },. This way it consistent with dgc and gcgc mappings from mini.comment. Just want to put this here if someone has the same concern.

Nice idea. Be sure to not use any ]g mappings, because they will have delay before executing.

echasnovski avatar Feb 22 '23 13:02 echasnovski

Thank you for this great plugin! I wonder if it is possible to add options to Diagnostic so that I can enable borders for the popup menu. The result may be similar to the following code

vim.keymap.set(
	"n",
	"]d",
	"<cmd>lua vim.diagnostic.goto_next({float={border = 'rounded'}})<cr>",
	{ noremap = true, silent = true, desc = "Diagnostic forward" }
)
vim.keymap.set(
	"n",
	"[d",
	"<cmd> lua vim.diagnostic.goto_prev({float={border = 'rounded'}})<cr>",
	{ noremap = true, silent = true, desc = "Diagnostic backward" }
)


Isrothy avatar Feb 22 '23 14:02 Isrothy

lua vim.diagnostic.goto_next({float={border = 'rounded'}})

I would suggest using vim.diagnostic.config(). So add something like this to your config:

vim.diagnostic.config({ float = { border = 'rounded' } })

echasnovski avatar Feb 22 '23 15:02 echasnovski

Really liking this new module, but I had two questions / comments:

Is it possible/desirable to have the bracket movements defined by this plugin added to the jumplist?

Also, I think I get the use of the first and last motions in the treesitter context, but it strikes me that thinking of [T and ]T as "parent" (which equates to n_times of 2) is more useful and possibly more intuitive.

oncomouse avatar Feb 22 '23 16:02 oncomouse

Is it possible/desirable to have the bracket movements defined by this plugin added to the jumplist?

Hmmm... Don't have strong opinion about this right now. As not adding is simpler, I did go with that. I'd ponder on it for some time. In the meantime, if it is a huge deal breaker for you, you can remap all needed once with prepended m` before calling target function.

By the way, do you feel like all targets which move cursor in current buffer should add to jumplist? Or maybe only some of them?

Also, I think I get the use of the first and last motions in the treesitter context, but it strikes me that thinking of [T and ]T as "parent" (which equates to n_times of 2) is more useful and possibly more intuitive.

Well, if user wants to go to parent, using 2 is not much of an overhead. At least compared to having to type unknowingly big number in order to jump completely out of current "context".

echasnovski avatar Feb 22 '23 16:02 echasnovski

By the way, do you feel like all targets which move cursor in current buffer should add to jumplist? Or maybe only some of them?

I'm fine with leaving them off entirely, but for the ones that move inside the file, maybe the first and last motions are more important. The question about jumplist first occurred when I was testing [T and ]T, got moved a bunch of lines up and tried to <C-O> back to where I was but ended up in a different file entirely and got quite lost. I don't think that's a problem that's likely to occur all that often, but if others express interest it might be nice.

Well, if user wants to go to parent, using 2 is not much of an overhead. At least compared to having to type unknowingly big number in order to jump completely out of current "context".

That makes sense as is. I'll start thinking about those as "get out of the context" motions instead of "move up" motions. I think I misread your initial post on Reddit and thought [T and ]T (and [I and ]I for that matter) were intended as "parent" motions rather than the way they actually work.

oncomouse avatar Feb 22 '23 16:02 oncomouse

I tried this option but nothing change indent = { opts = { change_type = "more or diff" } },? Screenshot 2023-02-22 at 16 46 20 I tried 'more' and 'diff' and expecting to jump to line comment = { ...} but I always end up in the last line of the current indent scope, 7 lines below.

Is this a bug or did I misunderstand this option?

lkhphuc avatar Feb 22 '23 16:02 lkhphuc

The question about jumplist first occurred when I was testing [T and ]T, got moved a bunch of lines up and tried to <C-O> back to where I was but ended up in a different file entirely and got quite lost. I don't think that's a problem that's likely to occur all that often, but if others express interest it might be nice.

This is the primary reason why jump target exists. I find it really annoying when <C-O> changes buffer when I don't want to. But your point is quite valid one. I'll think it through.

That makes sense as is. I'll start thinking about those as "get out of the context" motions instead of "move up" motions. I think I misread your initial post on Reddit and thought [T and ]T (and [I and ]I for that matter) were intended as "parent" motions rather than the way they actually work.

Both indent and treesitter targets are somewhat special in how they treat "first" and "last" directions. Due to performance reasons it can't directly jump to first/last target and go from there (as other targets). So here they are quite literally the "backward"/"forward" with a huge n_times.

echasnovski avatar Feb 22 '23 16:02 echasnovski

I tried this option but nothing change indent = { opts = { change_type = "more or diff" } },?

Try indent = { options = { change_type = "more or diff" } },. It doesn't seem like a good idea to use short form of words in configuration, so it is options. While using opts as function argument is now a common convention.

echasnovski avatar Feb 22 '23 16:02 echasnovski

so it is options

Thank you. Its work as expected. I was looking at the function document indeed.

lkhphuc avatar Feb 22 '23 16:02 lkhphuc

A vote for MiniBracketed.search.

and MiniBracketed.last_action which repeats the action(both next or back) by MiniBracketed. say if ]] and [[ are bound to MiniBracketed.last_action If last action is ]d, then ]] will act like ]d and [[ will act like [d

so that I can bind n and N for everything.

maan2003 avatar Feb 23 '23 07:02 maan2003

A vote for MiniBracketed.search.

I believe that n and N (possibly remapped to always follow same absolute direction) is enough.

and MiniBracketed.last_action which repeats the action(both next or back) by MiniBracketed. say if ]] and [[ are bound to MiniBracketed.last_action If last action is ]d, then ]] will act like ]d and [[ will act like [d

This is interesting. I'll think about that, but at the moment I am more inclined to leave that kind of functionality to a future which-key/hydra-like module.

echasnovski avatar Feb 23 '23 08:02 echasnovski

Noticed a deprecation warning using NVIM v0.9.0-dev-1028+gd422fc827

   Warn  13:42:33 notify.warn vim.treesitter.get_node_at_pos() is deprecated, use vim.treesitter.get_node() instead. See :h deprecated
This function will be removed in Nvim version 0.10
   Warn  13:42:33 notify.warn stack traceback:
	...tly/nvim-macos/share/nvim/runtime/lua/vim/treesitter.lua:345: in function <...tly/nvim-macos/share/nvim/runtime/lua/vim/treesitter.lua:344>
	[C]: in function 'pcall'
	...al/share/nvim/lazy/mini.bracketed/lua/mini/bracketed.lua:870: in function 'treesitter'
	[string ":lua"]:1: in main chunk

cbochs avatar Feb 23 '23 20:02 cbochs

I've been testing this out a bit over the past couple of days. Here are my comments so far:

Indent

The default change_type as less is pretty awkward. I found that diff feels like the more "natural" default (imo), where you're moving "around" indentation scopes

Treesitter

The current treesitter implementation doesn't feel very useful in the forward direction, and moving backward when at a "top-level" node. I could be using it incorrectly, but here are some scenarios that broke my assumptions:

Scenario 1: sibling nodes

local function foo() end
^ CURSOR HERE

local function bar() end
  • Action: press ]t twice.
  • Result: moves to the end of foo(), but goes no further
  • Expect: move to the sibling node (local function bar() end)

Scenario 2: function parameters

local function foo(a, b) end
^ CURSOR HERE
  • Action: press ]t
  • Result: moves to the end of foo()
  • Expect: go to the first parameter a

Scenario 3: child nodes

local function foo()
  if true then
    print("hi")
    ^ CURSOR HERE
  end
end
  • Action: press [t twice, then ]t twice
  • Result: moves to if true then, then local function, then end
  • Expect: moves to if true then, then local function, then back to if true then, and finally the original location of print("hi")

cbochs avatar Feb 24 '23 00:02 cbochs

Noticed a deprecation warning using NVIM v0.9.0-dev-1028+gd422fc827

   Warn  13:42:33 notify.warn vim.treesitter.get_node_at_pos() is deprecated, use vim.treesitter.get_node() instead. See :h deprecated
This function will be removed in Nvim version 0.10
   Warn  13:42:33 notify.warn stack traceback:
	...tly/nvim-macos/share/nvim/runtime/lua/vim/treesitter.lua:345: in function <...tly/nvim-macos/share/nvim/runtime/lua/vim/treesitter.lua:344>
	[C]: in function 'pcall'
	...al/share/nvim/lazy/mini.bracketed/lua/mini/bracketed.lua:870: in function 'treesitter'
	[string ":lua"]:1: in main chunk

Should be fixed on latest main.

echasnovski avatar Feb 24 '23 09:02 echasnovski

I've been testing this out a bit over the past couple of days. Here are my comments so far:

Thanks for detailed feedback!

The default change_type as less is pretty awkward. I found that diff feels like the more "natural" default (imo), where you're moving "around" indentation scopes

I kind of agree with you but only if taking into account "forward" and "backward" directions. However, change_type = 'diff' is not really useful for "first" and "last" directions. So decided to use "less" as default. Plus, it is more like the one from 'mini.indentscope' mappings from which it overshadows.

The current treesitter implementation doesn't feel very useful in the forward direction, and moving backward when at a "top-level" node. I could be using it incorrectly, but here are some scenarios that broke my assumptions:

The current design of treesitter target is mostly based on these ideas:

  • Be "query agnostic", i.e. should decide target location based on the node under cursor and tree structure. So no "move to next function, when cursor is not on the function", etc.
  • Don't be "too granular" or otherwise it will mostly move small distances which is not very efficient.
  • Make "first" and "last" directions useful.

Scenario 1. It doesn't go to sibling node because it breaks "first" and "last" directions. Due to performance reasons and implementation difficulty (same as in indent), "first"/"last" directions are simply "backward"/"forward" with very big n_times. This prompted the "current node and its parents" and "don't go into root node" choices. Here second ]t would go to root node and move to the end of buffer, which is not really useful.

Scenario 2. Going to function arguments is not query-agnostic. If anything, it would go first to start of foo, then on first (, and only then on a, because those are starts of different nodes in parsed tree.

Scenario 3. Quite valid assumption, but again not really granular if being query-agnostic. When moving forward there are also nodes which start at foo, then its (), then if, then true, and only then on print.

To gain intuition behind what tree-sitter parsed nodes look like, I'd suggest installing nvim-treesitter/playground. Then open buffer with attached tree-sitter and execute :TSPlaygroundToggle. It will open a split with node tree. You can jump into that split, move cursor, and see which regions of text represent separate nodes.

One of my three initial ideas about treesitter target was basically mimicking that tree from playground: "forward"/"backward" would essentially move down and up lines from that buffer until cursor change. After play-testing it, it proved to be too granular and did not have useful "first"/"last" direction. Plus, not that intuitive implementation.

echasnovski avatar Feb 24 '23 10:02 echasnovski

However, change_type = 'diff' is not really useful for "first" and "last" directions.

I'm wondering, would you consider being able to specify the change_type individually for first/last and forward/backward mappings? E.g. less for first/last and diff for forward/backward


The current design of treesitter target is mostly based on these ideas:

  • Be "query agnostic", i.e. should decide target location based on the node under cursor and tree structure. So no "move to next function, when cursor is not on the function", etc.
  • Don't be "too granular" or otherwise it will mostly move small distances which is not very efficient.
  • Make "first" and "last" directions useful.

I think these are fantastic principles to work off of 🙂

Due to performance reasons and implementation difficulty (same as in indent), "first"/"last" directions are simply "backward"/"forward" with very big n_times.

I do still think the backward and forward are a little mismatched. My knowledge of treesitter is rudimentary at best, but within the context of MiniBracketed.treesitter() what are the limitations of toggling "traverse siblings" when the direction is not first or last?

On another note, I took your advice and dove into the treesitter playground for a bit. After a bit of reading (:h treesitter-node) and testing some queries, here are a couple more of my thoughts!

  1. I like the first and last movements, they appear to map nicely with what I would expect them to do
  2. Regarding the forward movement and on the topic of "granularity", vim.treesitter supports traversing to either the child or a named_child. I'm wondering if maybe a middle-ground would be to jump only to named children when moving forward?

cbochs avatar Feb 24 '23 16:02 cbochs