feat: auto load popups and auto populate `help` popup #601

ten3roberts · 2023-07-15T11:37:08Z

This allows less locations to edit when adding a new popup

This ensures that when new popups are added they are also visible in the help buffer and not forgotten, as well as working the same when accessed

ten3roberts · 2023-07-15T12:30:36Z

Figured when working on #599 that I'd make it such that the Help popup always reflects the popups available in the status buffer, and that they all work the same way.

This way it is easier to contribute new ones, as there were previously 3 different files oyu had to change to add one, now it is only one.

This is almost ready, just have to fix the lints.

CKolkey · 2023-07-15T13:43:37Z

Yeah, consolidating that is a great move.

ten3roberts · 2023-07-16T08:38:48Z

Yes, the help buffer is a lot more useful now and is guaranteed to work the same (rather than error out as a popup required an argument when starting, but was supplied nil in the help buffer). A common example of this is the stash help which requires special feeding

ten3roberts · 2023-07-16T08:46:18Z

It all works on my side, and the tests.

Though it is quite a large change as it touches the status buffer, so let me know if you find anything

CKolkey · 2023-07-16T08:43:34Z

lua/neogit/popups/echo/actions.lua

Is this just for testing?

Yes, it is for a unit test inside M.test

CKolkey · 2023-07-16T08:45:03Z

lua/neogit/popups/init.lua

+---@param f nil|fun(create: fun(...)): any
+--- Creates a curried function which will open the popup with the given name when called
+function M.open(name, f)
+  f = f or function(f)


Can you explain how this works and why? 😅

Yes absolutely.

It is a function which given a popup name will return a function (which takes 0 arguments) which will when called load the popup neogit.popups/{name} module, and then pass its create function to the second argument f.

f can then call the create function with any arguments it likes, or whenever it wants.

I originally had it be plain argument, and create was called inside the open returned function instead, but this allows async and sleeping, as well as aborting the popup using the supplied function.

Essentially, this function will create a keymapping ready function, which will supply f with the means to open the correct popup.

the f = f or function(create) create() end will simply call the create function directly with no arguments if no second argument is supplied. Most popups don't need any extra arguments, but some do, such as stashing.

The normal flow is

open loads popup module
uses f to help calling the popup.create method.

In most cases, if the popup.create requires no special loading or arguments,

popup.open("my_awesome_popup") -> function() which opens popup is enough

This is usually called lifting and creating a function which "fills in" some arguments and returns a function, eg function(a,b) -> function(b) is called currying (partial application)

CKolkey · 2023-07-16T08:47:12Z

lua/neogit/popups/init.lua

+      "Help",
+      M.open("help", function(f)
+        f {
+          use_magit_keybindings = config.values.use_magit_keybindings,


I'm a little torn on this pattern of reaching into the status buffer for stuff. On the one hand, its a little cleaner than needing to pass the line into the constructor, but on the other hand it makes this function dependent on external state instead of passing that state in as an argument.

Not saying to change this, just voicing my thoughts

Yes, I am as well.

I usually go about coding with a domain driven approach. Some code lives in a certain domain/space, and can enter a smaller contained domain. Code usually becomes messier when a code enters a larger domain than which is was called from. Think of a graph which has to remain acyclic, like a tree.

The status buffer would need a bit of splitting like you did for the repository.

Perhaps splitting it as well into an init/view and actions and then using actions in other places.

Splitting the status buffer up is a longer-term goal of mine, but it clearly will take some doing. It also pre-dates the buffer lib, so doesn't really take advantage of the library that all the popups/buffers do. Eventually I'd like the status buffer to not have a special place like it currently does, and just be one more view (albeit the "main" view), but given the size/complexity of it, thats not a simple undertaking. So, small bits here and there.

It may be a good middle-step to use the init/view/actions pattern - I'm not against that.

lua/neogit/status.lua

CKolkey · 2023-07-16T08:52:25Z

Generally I really like this - hopefully I'll make some time later to look in more detail. Left some comments, I can definitely tell you're more at home with functional style stuff than I am - I dig that. Nice to learn new stuff

CKolkey · 2023-07-16T20:03:51Z

Everything looks great, except hitting ? pops up a notice saying there's no help popup :P

All the other popups seems to work though :)

ten3roberts · 2023-07-17T07:24:35Z

That is strange. I am not getting that error, though it makes the popup close itself, likely a "race" between closing the previous popup and opening a new popup with the same name

ten3roberts · 2023-07-17T07:25:09Z

This behavior can be seen on master as well

CKolkey · 2023-07-17T07:43:47Z

It can? The help popup comes up fine for me.

(also, this is why we need better test coverage)

ten3roberts · 2023-07-17T07:49:36Z

Yes. What happens if you do ? and then a second later ?

For me that opens and then closes the help popup.

There are definitely a lot of the older parts of the neogit that need to be fixed and are quite buggy and magic.

ten3roberts · 2023-07-17T15:10:38Z

Do you think the explaination of open is clear enough?

It is essentially to avoid doing repeatedly

["Foo"] = function() require("neogit.popups.foo").create() end, -- Most common usage
["Bar"] = function() require("neogit.popups.bar").create({ msg = input.read_message() }) end,

and allows

["Foo"] = popups.open("foo"), -- Most common usage
["Bar"] = popus.open("bar", function(c) c({ msg = input.read_message() }) end),

The invoked callback can also do something async and open the popup later as well.

ten3roberts · 2023-07-18T08:09:11Z

How about we merge this for now, and then fix any outlying errors afterwards?

The contributing PR depends on this one, and having the contributing guide merged sooner than later would be preferred

CKolkey · 2023-07-18T09:06:02Z

Lemme just verify the help popup is working first

CKolkey · 2023-07-18T09:15:58Z

Ok, the problem was on my end: I had Toggle bound to both h and l (to work more like a file tree), and the help popup didn't like that. Unfortunately the error saying "popup not found" wasn't the most helpful, and I discovered this by adding error(value) to popups/init.lua.

So, everything looks good, but maybe we can improve the error handling?

ten3roberts · 2023-07-18T09:18:42Z

Oh, great. I'll fix that then.

What did error(value) show, or in other words how does it fail to do pcall(require, "mod")?

lua/neogit/popups/echo/init.lua

lua/neogit/popups/echo/actions.lua

CKolkey · 2023-07-18T09:19:09Z

lua/neogit/popups/init.lua

+        "nv",
+        M.open(
+          "cherry_pick",
+          async.void(function(f)


Can you verify that this needs to be async.void? I'm not sure that it's needed, especially considering the revert popup isn't, and... they're real similar.

I don't think it needs to either, it was how it was at present.

The difference is that async.void may run on the next microtick, though not exactly sure. It should be immediate unless it needs that microframe delay for getting the visual selection. Will remove and tset

Yea, pretty sure I added that because I had no idea how async stuff worked when I did.

I tested it and it works without.

There is an error on master when doing AA on a visual selection. More testing (again) would catch it

Error executing vim.schedule lua callback: ...share/nvim/lazy/plenary.nvim/lua/plenary/async/async.lua:18: The coroutine failed with this message: ...rs/teiroberts/dev/nvim/neogit/lua/neogit/lib/git/cli.lua:749: invalid value (table) at index 8 in table for 'concat' stack traceback: [C]: in function 'error' ...share/nvim/lazy/plenary.nvim/lua/plenary/async/async.lua:18: in function 'callback_or_next' ...share/nvim/lazy/plenary.nvim/lua/plenary/async/async.lua:45: in function <...share/nvim/lazy/plenary.nvim/lua/plenary/async/async.lua:44>

CKolkey · 2023-07-18T09:22:38Z

Oh, great. I'll fix that then.

What did error(value) show, or in other words how does it fail to do pcall(require, "mod")?

with error(value) on line 19 of popups/init.lua

If I add this to the setup(), the following error occurs

    mappings = {
      status = {
        ["l"] = "Toggle",
        ["h"] = "Toggle",
      },
    },

E5108: Error executing lua /Users/cameron/code/neogit/lua/neogit/popups/init.lua:20: vim/shared.lua:436: 
The reverse lookup found an existing value for "Toggle" while processing key "<tab>"
stack traceback:
        [C]: in function 'error'
        /Users/cameron/code/neogit/lua/neogit/popups/init.lua:20: in function 'mapping'
        .../cameron/code/neogit/lua/neogit/lib/mappings_manager.lua:22: in function 'invoke'

        [string ":lua"]:1: in main chunk

Without that error(value) line, I just get a notification saying "the help popup cannot be found"

ten3roberts · 2023-07-18T09:38:07Z

Oh that's interesting, so the require fails because setup code is run when the module is loaded, and that fails.

I thought that require would only fail if not found, but I suppose it failing if anything errors makes sense, as lua allows generating modules entirely programmatically when loading instead of from a file (if you are crazy enough to do that and throw debuggabability out of the window)

CKolkey

Great work :D

The error popup is a nice touch

ten3roberts · 2023-07-18T13:48:46Z

vim.keymap.set('n', '<leader>gc', neogit.popups.open("commit"))

Would be enough if you do not want to pass extra data to the popup create function.

ten3roberts added 2 commits July 15, 2023 13:41

feat: auto load popups

37991e3

This allows less locations to edit when adding a new popup

chore: move popup mappings to popups module

2973302

ten3roberts force-pushed the move-cmd-map branch from 1b4c1a2 to 35d39ac Compare July 15, 2023 12:16

feat: make help use same popups as status buffer

f687001

This ensures that when new popups are added they are also visible in the help buffer and not forgotten, as well as working the same when accessed

ten3roberts force-pushed the move-cmd-map branch from 35d39ac to f687001 Compare July 15, 2023 12:16

ten3roberts added 3 commits July 16, 2023 10:29

feat: async popup opener

8732bf5

fix: ensure help ordering is the same as before

9ec9c91

fix: lint

26526cf

ten3roberts marked this pull request as ready for review July 16, 2023 08:37

chore: remove superfluous stash pass through

6662b04

ten3roberts requested a review from CKolkey July 16, 2023 08:45

CKolkey reviewed Jul 16, 2023

View reviewed changes

ten3roberts added a commit that referenced this pull request Jul 16, 2023

chore: add popups section following #601

9695afd

ten3roberts added 2 commits July 16, 2023 11:00

chore: export get_selected_commits

4119fb9

fix: argument name

0d673ba

CKolkey mentioned this pull request Jul 16, 2023

Modify the hint according to user keymappings #564

Closed

ten3roberts requested a review from CKolkey July 16, 2023 18:56

CKolkey requested changes Jul 18, 2023

View reviewed changes

ten3roberts requested a review from CKolkey July 18, 2023 11:55

fix: popup error message

d0e15b5

ten3roberts force-pushed the move-cmd-map branch from a31b775 to d0e15b5 Compare July 18, 2023 11:56

CKolkey approved these changes Jul 18, 2023

View reviewed changes

ten3roberts merged commit f765604 into master Jul 18, 2023

ten3roberts deleted the move-cmd-map branch July 18, 2023 12:11

CKolkey mentioned this pull request Jul 18, 2023

Unable to open popups from outside of Neogit #637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auto load popups and auto populate `help` popup #601

feat: auto load popups and auto populate `help` popup #601

ten3roberts commented Jul 15, 2023

ten3roberts commented Jul 15, 2023

CKolkey commented Jul 15, 2023

ten3roberts commented Jul 16, 2023

ten3roberts commented Jul 16, 2023

CKolkey Jul 16, 2023

ten3roberts Jul 16, 2023

CKolkey Jul 16, 2023

ten3roberts Jul 16, 2023

CKolkey Jul 16, 2023

ten3roberts Jul 16, 2023

ten3roberts Jul 16, 2023

CKolkey Jul 16, 2023

CKolkey commented Jul 16, 2023

CKolkey commented Jul 16, 2023

ten3roberts commented Jul 17, 2023

ten3roberts commented Jul 17, 2023

CKolkey commented Jul 17, 2023

ten3roberts commented Jul 17, 2023 •

edited

Loading

ten3roberts commented Jul 17, 2023 •

edited

Loading

ten3roberts commented Jul 18, 2023

CKolkey commented Jul 18, 2023

CKolkey commented Jul 18, 2023

ten3roberts commented Jul 18, 2023

CKolkey Jul 18, 2023

ten3roberts Jul 18, 2023

CKolkey Jul 18, 2023

ten3roberts Jul 18, 2023

CKolkey commented Jul 18, 2023 •

edited

Loading

ten3roberts commented Jul 18, 2023

CKolkey left a comment •

edited

Loading

ten3roberts commented Jul 18, 2023

feat: auto load popups and auto populate help popup #601

feat: auto load popups and auto populate help popup #601

Conversation

ten3roberts commented Jul 15, 2023

ten3roberts commented Jul 15, 2023

CKolkey commented Jul 15, 2023

ten3roberts commented Jul 16, 2023

ten3roberts commented Jul 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CKolkey commented Jul 16, 2023

CKolkey commented Jul 16, 2023

ten3roberts commented Jul 17, 2023

ten3roberts commented Jul 17, 2023

CKolkey commented Jul 17, 2023

ten3roberts commented Jul 17, 2023 • edited Loading

ten3roberts commented Jul 17, 2023 • edited Loading

ten3roberts commented Jul 18, 2023

CKolkey commented Jul 18, 2023

CKolkey commented Jul 18, 2023

ten3roberts commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CKolkey commented Jul 18, 2023 • edited Loading

ten3roberts commented Jul 18, 2023

CKolkey left a comment • edited Loading

Choose a reason for hiding this comment

ten3roberts commented Jul 18, 2023

feat: auto load popups and auto populate `help` popup #601

feat: auto load popups and auto populate `help` popup #601

ten3roberts commented Jul 17, 2023 •

edited

Loading

ten3roberts commented Jul 17, 2023 •

edited

Loading

CKolkey commented Jul 18, 2023 •

edited

Loading

CKolkey left a comment •

edited

Loading