[WIP] (mini.clue) expose function to start a query

[hbiel]

Based on our discussion in #430 I came up with the following solution for my use case.

This was my proposal:

The plugin could provide an API to show clues for a prefix of key sequences (or none).

This allows users to manually or programmatically start the key query process for a given set of keys (without actually pressing them).
The prefix can be left empty so that the first set of available keys is shown.

My Solution:

1. I exposed the rhs-function for the trigger mappings via MiniClue.start_query() and use this function in the mappings.
   The trigger is now expected as a parameter. This allows one to setup triggers other than key mappings, for example this one:

vim.api.nvim_create_autocmd(
    "modeChanged",
    {
        pattern= "n:no",
        callback = function()
            require('mini.clue').start_query({mode="o", keys=""})
        end
    }
)

3. When the starting query is empty, there are no mappings to be unmapped and mapped again afterwards. So I added a check for that.
4. I added H.keys.exit to the tweaks for the d and y operator. Otherwise in my use case we would stay in operator pending mode until we hit enter.

I have not added tests or documentation yet, but would be willing to try where needed.
All existing tests where successful on the following builds of Neovim:

• 0.7.2
• 0.8.3
• 0.9.1
• 0.10.0-dev-811+g58a1ef8e6

Just let me know what you think about this solution in general or any changes you would like to see.

• I have read CONTRIBUTING.md
• I have read CODE_OF_CONDUCT.md

[echasnovski]

Thanks for the PR!

I can't exactly verbalize why I am not fond of the idea of exporting this function directly. The best guess is probably because I have a gut feeling that it will bring a lot of unexpected issues when used not in the way we anticipated. Like, for example, I am not sure what is the proper way to handle the case when this function is called but 'mini.clue' already called getcharstr() and waiting for user input.

Having to update operator tweaking to me is another argument against this.

I would better support { mode = 'o', keys = '' } type of trigger, but I can't see a way to do that efficiently. Meaning (at least) that it won't lead to frequent recomputation of clues and that MiniClue.config.triggers can get updated during runtime while still work later in new buffer.

I'll give it a closer look.

[hbiel]

I can't exactly verbalize why I am not fond of the idea of exporting this function directly. The best guess is probably because I have a gut feeling that it will bring a lot of unexpected issues when used not in the way we anticipated. Like, for example, I am not sure what is the proper way to handle the case when this function is called but 'mini.clue' already called getcharstr() and waiting for user input.

Yeah, I can see this.

Having to update operator tweaking to me is another argument against this.

I don't exactly know why this is needed, but it was the only solution I found that worked in all cases. I played around with various modes of feedkeys and setting recursive to true. But this broke the tests.

I think it has to do with having keys = '' as starting point.

I would better support { mode = 'o', keys = '' } type of trigger, but I can't see a way to do that efficiently. Meaning (at least) that it won't lead to frequent recomputation of clues and that MiniClue.config.triggers can get updated during runtime while still work later in new buffer.

What ways do you see to approach this? (efficiently or not, just to get some ideas)
One way I see is to let the plugin setup autocommands for these types of triggers (when trigger.keys is empty).
But then we would need a mapping of full modes to the mode used in the keymaps.
I could try to implement this.

I'll give it a closer look.

Thanks!

[echasnovski]

What ways do you see to approach this? (efficiently or not, just to get some ideas)

To be perfectly honest, my initial approach is to not support this at all. 'mini.clue' has somewhat fragile implementation due having to deal with a lot of Neovim internals. Adding more is troublesome.

But at the same time I do see the appeal of having this functionality. Just not sure if it is worth it over adding a lot of new code.

One way I see is to let the plugin setup autocommands for these types of triggers (when trigger.keys is empty).
But then we would need a mapping of full modes to the mode used in the keymaps.

Yes, this is the first idea that came to mind. One problem here is that triggers are set up per buffer and having new autocommand created per buffer might lead to a lot of autocommands lying around. This is ok for mappings because they are removed with buffer (as far as I understand), but not autocommands. Which means extra code for cleanup, etc.

I could try to implement this.

I would advise against this, as updating 'mini.clue' code is indeed quite time consuming and complicated. I'll look into it myself.

[hbiel]

Yes, this is the first idea that came to mind. One problem here is that triggers are set up per buffer and having new autocommand created per buffer might lead to a lot of autocommands lying around. This is ok for mappings because they are removed with buffer (as far as I understand), but not autocommands. Which means extra code for cleanup, etc.

Ah, I see. I didn't really think about the need for them to be created per buffer. This makes it more complicated for sure.

I would advise against this, as updating 'mini.clue' code is indeed quite time consuming and complicated. I'll look into it myself.

Alright, thanks again! Let me know when I can help in any way.

[hbiel]

This is ok for mappings because they are removed with buffer (as far as I understand), but not autocommands. Which means extra code for cleanup, etc.

I just looked this up and found this part in the documentation. So maybe this makes it a little bit easier:

When a buffer is wiped out its buffer-local autocommands are also gone, of course. Note that when deleting a buffer, e.g., with ":bdel", it is only
unlisted, the autocommands are still present.

See: https://neovim.io/doc/user/autocmd.html#autocmd-buflocal

[echasnovski]

Another obstacle for going forward with this is the fact that buffer-local autocommands are not allowed to use pattern. So either don't use pattern (leading to all "mode-wide" triggers to be executed on any ModeChanged; which is far more frequent than it seems) or buffer (leading to necessity of making some triggers in global autocommands which complicates code).

Not going to lie, chances for adding this continue to go down. I'll revisit it later, hopefully with new ideas.

[hbiel]

Are there any things that must be checked / done when using global autocommands for this? Like things that wouldn't be needed if the autocommands were buffer local?

[echasnovski]

Are there any things that must be checked / done when using global autocommands for this? Like things that wouldn't be needed if the autocommands were buffer local?

Not sure if there are (right now, at least). One thing I don't like about them being global is that it introduces the need to track them. This is needed to allow updating MiniClue.config.triggers interactively, run enable_all_triggers(), and have them all valid. If not tracked, it will result into duplicating.

---

As a side note, I've tried "exporting start_query()" approach from this PR. At least one thing it does better than allowing keys = '' in triggers is that it shouldn't introduce too much new code. But it does need a revamp of operators tweaks for them to work in Operator-pending autocommand. For example, try g~iw or >ip with this PR and autocommand on ModeChanged *:no*.

[hbiel]

Not sure if there are (right now, at least). One thing I don't like about them being global is that it introduces the need to track them. This is needed to allow updating MiniClue.config.triggers interactively, run enable_all_triggers(), and have them all valid. If not tracked, it will result into duplicating.

That makes sense.

As a side note, I've tried "exporting start_query()" approach from this PR. At least one thing it does better than allowing keys = '' in triggers is that it shouldn't introduce too much new code. But it does need a revamp of operators tweaks for them to work in Operator-pending autocommand. For example, try g~iw or >ip with this PR and autocommand on ModeChanged *:no*.

I just pushed a fix for those operators. They should now work as intended.
Unfortunately this is a case where headless mode for testing seems to behave differently from 'regular' usage.
Nevermind... This probably slipped by me because i haven't written any tests for this exact case yet. Hadn't thought that trough...

I actually quite like the versatility of this approach. Another possible use case is setting up a mapping like this:
vim.keymap.set('n', 'help', "<cmd>lua require('mini.clue').start_query({mode='n', keys=''})<CR>", {desc="show clues"})
This would allow the user to show the clues for immediate keys in normal mode when she/he needs a hint rather than always triggering on mode change.

I get your concerns though.

[hbiel]

I actually just stumbled on such a problem.
I had both my autocommand and those triggers in my config:

{ mode = "o", keys = "i" },
{ mode = "o", keys = "a" },

This triggered mini.clue on entering operartor-pending mode and again when pressing i which led to strange behaviour.

This at least could be prevented by removing all mini.clue related mappings in the current buffer directly when entering the query process and re-enabling them afterwards.

Edit: Pushed a fix for it. Actually the trigger mapping hit when the first sequence was being executed. I now just disable all buffer local mappings before execution.

[hbiel]

I have pushed another iteration as a prototype.

Notes on this one:

• For simplified testing I create the autocmd alongside the other autocommands
• I modified and added a few checks when entering start_query()
  ** When H.state.disable_autocmd_triggers is true it simply returns. This was just a quick implementation to test this functionality
  ** When a mapping is currently being executed it also returns. This check is done via vim.fn.state() and unfortunately requires nvim 0.10 as this is available for just a few weeks. I don't really know if this could be done otherwise.
  ** I modified your is_in_exec check because i had issues when both the autocommand and another trigger like { mode = 'o', keys = 'i' } were set. This fixed it but doesn't seem to be needed anymore.
• During execution i disable all triggers instead of only the current one. This is an additional safety measure to disable all triggers that might be in the chain to be executed besides the current trigger. See above
• And (in my opinion) the best part: When using the autocmd as trigger no operator tweaks are needed as just the keys relevant to operator-pending mode need to be executed.

As far as i can tell everything seems to be working although some tests are still failing. I think that's because the autocommand is currently hardcoded and the tests probably need to be adjusted to the final implementation (if this should happen).

It would be awesome if you could take another look when you got the time.

[echasnovski]

I have pushed another iteration as a prototype.

Notes on this one:

* For simplified testing I create the autocmd alongside the other autocommands

* I modified and added a few checks when entering start_query()
  ** When H.state.disable_autocmd_triggers is true it simply returns. This was just a quick implementation to test this functionality
  ** When a mapping is currently being executed it also returns. This check is done via vim.fn.state() and unfortunately requires nvim 0.10 as this is available for just a few weeks. I don't really know if this could be done otherwise.
  ** I modified your is_in_exec check because i had issues when both the autocommand and another trigger like { mode = 'o', keys = 'i' } were set. This fixed it but doesn't seem to be needed anymore.

* During execution i disable all triggers instead of only the current one. This is an additional safety measure to disable all triggers that might be in the chain to be executed besides the current trigger. See above

* And (in my opinion) the best part: When using the autocmd as trigger no operator tweaks are needed as just the keys relevant to operator-pending mode need to be executed.

As far as i can tell everything seems to be working although some tests are still failing. I think that's because the autocommand is currently hardcoded and the tests probably need to be adjusted to the final implementation (if this should happen).

It would be awesome if you could take another look when you got the time.

Thanks for not loosing interest in this feature!

I have looked at implementation and here are some thoughts:

• Strictly depending on Nightly in exported functionality is not really welcome. Admittedly, it can be negotiated on a case by case basis, but I don't feel this is the one.
• Temporarily disabling all triggers when every trigger is run is something that will never get merged. I was really reluctant even to disable/enable single trigger in single buffer because it feels like an unnecessary overhead, but couldn't find a way around it.

To make it as explicit as possible, here are the requirements for this to be considered as a potential merge:

• Introduces a single new exported function start_query_process() which works for your use cases and refactored code passes all current tests on all supported Neovim versions (0.7.2, 0.8.3, 0.9.1, nightly). So no temporarily hard coded autocommands in source: tell which autocommands you've tested it with so I can double test it. No need to add tests for it just yet, but having tests parametrized over how it enables triggers was a solid choice.
• Doesn't make worse the "temporarily disable target trigger in current buffer" hack.
• Preferably, doesn't depend on Nightly features in exported function.

I know, this might be a bit strict, but 'mini.clue' did prove to be a very troublesome module with a lot of hidden edge cases. So better be overly cautious here.

[hbiel]

Thanks for your feedback.

Just to clarify: I never intended this to be merged as is. This is why i referred to it as a prototype.
I just wanted to share my current progress as i think this might be getting somewhere.

I thought of it this way: The features using 0.10 would be completely optional. So when using nvim 0.9 or below a warning could be thrown instead of setting up the autocommands (like ''-type trigger need nvim 0.10 to work as inteded).
I actually intended to improve on it by making the triggers configurable like e.g. { mode = 'o', keys = '' } an then configure an autocommand for that.

Temporarily disabling all triggers was an additional safety measure to ensure there aren't any triggers down the line when executing. As long as the user hasn't setup multiple triggers in a possible query we should be fine without it.

Nonetheless i have reduced the changes to a minimum. The required code to use it in autocommands can be done outside like this:

vim.api.nvim_create_autocmd('RecordingEnter', {
    pattern = '*',
    callback = function ()
        vim.b.miniclue_disable_autocmd_triggers = true
    end
})
vim.api.nvim_create_autocmd('RecordingLeave', {
    pattern = '*',
    callback = function ()
        vim.b.miniclue_disable_autocmd_triggers = false
    end
})
vim.api.nvim_create_autocmd('ModeChanged', {
    pattern = '*:no*',
    callback = function ()
        if vim.fn.has('nvim-0.10') == 1 and vim.fn.state('m') ~= '' then
            return
        end
        if vim.b.miniclue_disable_autocmd_triggers then
            return
        end

        vim.b.miniclue_disable_autocmd_triggers = true
        require('mini.clue').start_query_process({ mode = 'o', keys = '' })
        vim.schedule(function() vim.b.miniclue_disable_autocmd_triggers = false end)
    end
})

One thing i noticed: Using undo after e.g. >ip makes the cursor jump down a line.
This doesn't seem to be right but also happens without my modifications.

All existing tests still pass using nvim 0.7.2, 0.8.3, 0.9.1 and nightly.

[echasnovski]

I thought of it this way: The features using 0.10 would be completely optional. So when using nvim 0.9 or below a warning could be thrown instead of setting up the autocommands (like ''-type trigger need nvim 0.10 to work as inteded).

I don't really understand. If assumed most common use case for start_query_process() (in ModeChanged autocommand) needs feature from 0.10 for proper usage, then it arguably depends on being used only in 0.10.

One thing i noticed: Using undo after e.g. >ip makes the cursor jump down a line.
This doesn't seem to be right but also happens without my modifications.

I can't really reproduce this.

Would you mind also posting the 'mini.clue' config? Its config.triggers relevant to testing start_query_process() in particular?

[hbiel]

I don't really understand. If assumed most common use case for start_query_process() (in ModeChanged autocommand) needs feature from 0.10 for proper usage, then it arguably depends on being used only in 0.10.

Okay, i get what you mean now. I thought of it as an additional feature you could make use of when you got 0.10. Otherwise the plugin would still work the same without it.

Maybe vim.fn.state() isn't even needed here though. I just tested it without it and it seems to still work the same.
I implemented it before i got the operator-tweak section right, so i thought it was crucial but maybe not...

I can't really reproduce this.

Interesting. I never noticed this with which-key but maybe i changed something else in my config. Need to check this later.

Would you mind also posting the 'mini.clue' config? Its config.triggers relevant to testing start_query_process() in particular?

Sure thing

{
  triggers = {
      -- Leader triggers
      { mode = "n", keys = "<Leader>" },
      { mode = "x", keys = "<Leader>" },

      -- Built-in completion
      { mode = "i", keys = "<C-x>" },

      -- `g` key
      { mode = "n", keys = "g" },
      { mode = "x", keys = "g" },

      -- Marks
      { mode = "n", keys = "'" },
      { mode = "n", keys = "`" },
      { mode = "x", keys = "'" },
      { mode = "x", keys = "`" },

      -- Registers
      { mode = "n", keys = '"' },
      { mode = "x", keys = '"' },
      { mode = "i", keys = "<C-r>" },
      { mode = "c", keys = "<C-r>" },

      -- Window commands
      { mode = "n", keys = "<C-w>" },

      -- `z` key
      { mode = "n", keys = "z" },
      { mode = "x", keys = "z" },
  },
  clues = {
      -- Enhance this by adding descriptions for <Leader> mapping groups
      miniclue.gen_clues.builtin_completion(),
      miniclue.gen_clues.g(),
      miniclue.gen_clues.marks(),
      miniclue.gen_clues.registers({ show_contents = true }),
      miniclue.gen_clues.windows(),
      miniclue.gen_clues.z(),
      { mode = "n", keys = "<leader>c", desc = "+commands" },
      { mode = "n", keys = "<leader>ca", desc = "+ansible" },
      { mode = "n", keys = "<leader>f", desc = "+file/find" },
      { mode = "n", keys = "<leader>g", desc = "+git" },
      { mode = "n", keys = "<leader>gh", desc = "+hunks" },
      { mode = "n", keys = "<leader>n", desc = "+neo-tree" },
      { mode = "n", keys = "<leader>p", desc = "+plugins" },
      { mode = "n", keys = "<leader>sn", desc = "+noice" },
  },
  window = {
      -- Show window immediately
      delay = 0,
      config = {
          anchor='NW',
          -- Compute window width automatically
          width = "auto",
          col = 'auto',
          -- Use double-line border
          border = "rounded",
      },
  },
}

[hbiel]

A few more things i noticed / thought about:

• The use of vim.fn.state() seems to be help in the tests:
  • I have added tests on https://github.com/hbiel/mini.nvim/tree/feature/expose_start_query_function_with_tests and two tests fail when the vim.fn.state() condition is removed.
  • I have not noticed any issues in real use though, so it might be a timing related issue
• Closing the clue popup (via or ) doesn't end operator-pending mode. If these autocmd triggers should ever be fully implemented in the plugin this is probably something to think about.

One thing i noticed: Using undo after e.g. >ip makes the cursor jump down a line.
This doesn't seem to be right but also happens without my modifications.

I narrowed this down to being an issue with noice.nvim. Disabling it fixes this.

[echasnovski]

After several times of getting back to this, I couldn't reach the state when I felt that this change worth the effort.

Sorry for your time spent on this PR. Closing as not planned.

[hbiel]

Well, I had my fair share of fun exploring your plugin and discovering new options in neovim.

I'll probably keep my fork around, so feel free to take a look if / when you'd like to try again.

Thanks for taking the time to think about it!

[echasnovski]

Thanks for the understanding! 🙏