fix(popup)!: moved="any", enter default, callback=fn

[errael]

(I'm looking at adding mousemoved='any', close='click', and popup_close() to popups; came across a few things to fix first.)

To reviewers
This is my first neovim related PR and my first lua code. Suggestions about style... welcome. Not sure how this repo handles reviews, so I'm tagging some names that have done some popup work (sorry for the noise). There's some TODO I scattered which I'll remove before taking out of draft, looking for feedback. Note: haven't yet looked at tests; wonder how to run locally. There's a probable bug in tbl.apply_defaults, noted in popup/utils.lua.

@l-kershaw @fdschmidt93 @AlejandroSuero @xactlyblue @jesseleite
@Conni2461 @tjdevries

• Fix moved = 'any' option.
  Before this PR, the popup option "moved = 'any'," gets an exception because of changes made to vim.lsp.util.close_preview_autocmd:

  • refactor(lua): use api to replace vim.cmd neovim/neovim#19283 changed the behavior,
  • fix(ui): close floating window on BufLeave event neovim/neovim#16557 changed the accessibility,

  Those changes looks like a fix. The now private code from "...lsp.util" is copied in here and the the buffer numbers are added as needed for the new code.

• Default the new option enter to false
  Changed the default to false, so the behavior matches vim with no options; which is to leave the cursor where it was and not change the focus to the popup. Is there a place to note breaking changes? Wonder the reason for "enter" option?

• Fix callback
  I couldn't get it to work in a simple case. Converted to an autocmd. Comments on potential timing issues? Accessing buffer in callback does work. There's some preliminary work to support "popup_close(win_id, result)" and mouse stuff for a later PR.

• Update POPUP.md

Could also do ...
There's dict_default(). Would using tbl.apply_defaults make sense? Seems cleaner.
Should tbl.apply_defaults do a deepcopy of any table from defaults?

There was a comment around the callback setup.

Giving win_id is pointless here because it's closed right afterwards
but it might make more sense once hidden is implemented

I've removed the comment. The called back function may want to know the win_id; for example,
there could be a data structure associated with the win_id that can be freed.

PS: For the stuff I'm looking at adding, I could open a discussion or just do the PR and discuss there; recommendation?

[AlejandroSuero]

@errael I am not too familiar with the codebase itself but I can solve some of the questions

To run test:

• make test see Makefile if you want to know what it does.
• Reference to plenary.test_harness.

To make test:

• Reference to lua testing with busted style(it's the style that plenary uses).
• Check the tests/plenary/popup_spec.lua to see some examples (it's what I did).

To check code style:

• Check the files you touched and make a similar style.
• Check the linter passes when using make lint (same Makefile as before).

The next thing I am not so sure about, but I think this PR will "break" things in comparison with the master branch so it would be nice to use the conventional commits syntax to display it, i.e. fix(popup)!: .... Also commenting it in the PR itself often helps to see it before checking the code (at least for me).

[errael]

@AlejandroSuero Thanks so much; very useful. luacheck was yet another thing to install. The commit syntax you mean is the ! in the message; cool.

I was going to ask about running just popup tests. But got around to doing the following on the command line

nvim --headless --noplugin -u scripts/minimal.vim -c "PlenaryBustedDirectory 
    tests/plenary/popup_spec.lua {minimal_init = 'tests/minimal_init.vim', sequential = true}"

which seemed to work.

It's been so many years since I've messed with makefiles... Wanting to use the Makefile to run only popup_spec.lua, this seems to work

... "PlenaryBustedDirectory tests/plenary/$(TEST1) {mi...

and doing

TEST1=popup_spec.lua make test

Is there something else that makes sense? I'll include this change in the PR, can always take it out.

One test, out of all the tests, is failing. Is this something to file an issue about?

[AlejandroSuero]

@errael to test single files you can change or create a target on the Makefile, the PlenaryBustedDirectory with PlenaryTestFile as indicated in the reference plenary.test_harness that I linked before.

I will stick to the testing the whole suite in case some other modules make use of popup. Also is what is used on CI. But this something that @Conni2461 and @tjdevries should know better to explain why there is no target for test-file passing the file as arguments.

Regarding the test failure, right now Im on my mobile so its a pain to check it tbh, but no ones resolves your question, tomorrow when I wake I see what could be the point of failure 😊

[errael]

I've added some tests. I've run into a problem. In particular async(), from busted, is not found.

Here's the use case

1. move the cursor with vim.fn.cursor(2, 1)
2. let the cursor move events and autocommands propagate
3. check that things happened as expected

The code can be seen in popup_spec.lua around line 195 in the PR after

    it("with moved", function()
      async()

/src/tools/neovim/plenary.nvim/tests/plenary/popup_spec.lua:196: attempt to call global 'async' (a nil value)

[errael]

Regarding the test failure, right now Im on my mobile so its a pain to check it tbh, but no ones resolves your question, tomorrow when I wake I see what could be the point of failure 😊

What I'm seeing

Testing: 	/junk/repo/neovim/plenary.nvim/tests/plenary/scandir_spec.lua	
Success	||	scandir can list all files recursive with cwd	
...
Success	||	scandir ls works for cwd	
Success	||	scandir ls works for another directory	
Fail	||	scandir ls works with opts.hidden for cwd	
            .../repo/neovim/plenary.nvim/tests/plenary/scandir_spec.lua:246: Expected objects to be the same.
            Passed in:
            (boolean) false
            Expected:
            (boolean) true
            
            stack traceback:
            	.../repo/neovim/plenary.nvim/tests/plenary/scandir_spec.lua:246: in function <.../repo/neovim/plenary.nvim/tests/plenary/scandir_spec.lua:240>
            	
	
Success: 	21	
Failed : 	1	
Errors : 	0	

[errael]

Make it ready for review.

Test callback option OK. Test enter option OK.

Test moved option has problems; it's commented out. Pretty sure the test is flawed. I'll address it with the next PR.

[AlejandroSuero]

@errael The principal issue I see is that async() is not defined (it doesn't exists), so does timer and done.

In my case, scandir_spec.lua doesn't fail.

Here is the error that async throws in my end.

Fail || plenary.popup moved option without moved
     .../aome/personal/plenary.nvim/tests/plenary/popup_spec.lua:179: attempt to call global 'async' (a nil value)
            
     stack traceback:
       .../path/to/plenary.nvim/tests/plenary/popup_spec.lua:179: in function <.../path/to/plenary.nvim/tests/plenary/popup_spec.lua:178>
            
Fail || plenary.popup moved option with moved
     .../path/to/plenary.nvim/tests/plenary/popup_spec.lua:195: attempt to call global 'async' (a nil value)
            
      stack traceback:
        .../path/to/plenary.nvim/tests/plenary/popup_spec.lua:195: in function <.../path/to/plenary.nvim/tests/plenary/popup_spec.lua:194>

[AlejandroSuero]

You can use either vim.defer_fn or vim.schedule_wrap or require "plenary.job" and make use of Job:new.

Some examples in async_testing_spec.

Also in tests/plenary/async/async_spec.lua you can see how to use "plenary.async".tests.add_to_env() to make the it async.

[AlejandroSuero]

@errael I made the following changes, in case you want me to commit it to your branch.

-- at the top
require("plenary.async").tests.add_to_env() -- this lets you use the `a` variable to make it async
-- ...
  a.describe("moved option", function()
    local function populate()
      local wnr = vim.fn.winnr()
      local bnr = vim.fn.winbufnr(wnr)
      vim.fn.setbufline(bnr, 1, {"one", "two", "three", "four"})
      vim.fn.cursor(1, 1)
    end
    local callback_result
    local function callback(wid, result)
      callback_result = result
    end

    a.it("without moved", function()
      -- removed aync()
      callback_result = nil
      populate()
      local win_id = popup.create("hello there", {
        callback = callback,
      })
      -- move the cursor, should not do callback
      vim.fn.cursor(2, 1)
      local timer = vim.uv.new_timer() -- declared timer
      timer:start(10, 0, function()
        eq(nil, callback_result)
        vim.api.nvim_win_close(win_id, true)
      end)
      -- removed done()
    end)

    a.it("with moved", function()
      -- removed async()
      callback_result = nil
      populate()
      local win_id = popup.create("hello there", {
        moved = "any",
        callback = callback,
      })
      -- move the cursor, window closes and callback invoked
      vim.fn.cursor(2, 1)
      local timer = vim.uv.new_timer() -- declared timer
      timer:start(10, 0, function()
        eq(-1, callback_result)
        if -1 ~= callback_result then
          -- window wasn't closed
          vim.api.nvim_win_close(win_id, true)
        end
        -- removed done()
      end)
    end)
  end)

[errael]

You can use either vim.defer_fn or vim.schedule_wrap or require "plenary.job" and make use of Job:new.

Cool. vim.defer_fn looks convenient. I'll play around with those.

Some examples in async_testing_spec.

Also in tests/plenary/async/async_spec.lua you can see how to use "plenary.async".tests.add_to_env() to make the it async.

Thanks for the notes on using async. I'll take a look at the references you provided; appreciated. So the busted async isn't available I guess. I'll have to think about why the done wouldn't be needed.

No need to do the commit, thanks for taking a look. So much new stuff ...