feat: modern menu

[danilshvalov]

Hello everyone I don't really like the way the selection menu is made now. Therefore, I suggest updating it.

Now I have the following developments:

[kristijanhusak]

Hey, I don't plan to change the current built in view because it looks similar to Emacs prompt. There are 2 options this can be tackled:

• Add a config option to define your own menu, and default it to current utils.menu. That way you can override the functionality in your config
• Monkey patch orgmode.utils.menu with your implementation. This is more hacky way, but I think it could work.

[danilshvalov]

I've redone the menu. Now the user can customize the menu himself. emacs and modern presets are also available. Examples:

Emacs style

require("orgmode").setup({
  ui = {
    menu = {
      preset = "emacs",
    },
  },
})

Modern style

require("orgmode").setup({
  ui = {
    menu = {
      preset = "modern",
    },
  },
})

Custom style

require("orgmode").setup({
  ui = {
    menu = {
      custom = function(title)
        vim.print("Just print title...")
        vim.print("Title is '" .. title .. "'")
      end,
    },
  },
})

[jgollenz]

Hm, I think this opens up a larger question of UI changes and where they belong. Could this (and any UI specifics) be made accessible via the API and enable @danilshvalov and everybody else to write their own UI overhaul of the whole plugin?

edit: @danilshvalov don't get me wrong, I really like your take on it :) but I think it might be worthwhile to discuss thinking big here, before we start doing UI customization in the core plugin

[danilshvalov]

@jgollenz If I understood what was written correctly, right now it is impossible to customize the UI through the API. This PR provides one of the ways of such customization (the custom option).

[kristijanhusak]

Thanks for the PR!

The general idea is fine, but I don't think there's a need to have presets part of the plugin.
We can either have the default one, or a custom one that the user provides.
Comments on code will explain what's missing.
If you want to expose the modern menu to more people, I would suggest creating a small plugin and just attach it through the config.

Edit: It would also be great to update the documentation with this new option so other users know how to set up a custom menu.

[danilshvalov]

I have redone the code, created some classes that internally check the correctness of the entered data. So now there are two types that can be used in the menu: MenuItem and MenuSeparator.

If everything is fine in the code, I am ready to add documentation.

[kristijanhusak]

Looks good, you can proceed with docs.

[danilshvalov]

I now thought a little and decided that it would be nice to pass data to the menu not through function arguments, but through a table. For example:

Menu.open({
  title = 'Some title',
  items = { ... },
  prompt = 'Prompt',
})

What do you think about it? In the future, with possible expansion, this will make refactoring easier.

[jgollenz]

pass data to the menu not through function arguments, but through a table

I second this, but please let's document that table somewhere 👍

[kristijanhusak]

I now thought a little and decided that it would be nice to pass data to the menu not through function arguments, but through a table. For example:

Menu.open({
  title = 'Some title',
  items = { ... },
  prompt = 'Prompt',
})

What do you think about it? In the future, with possible expansion, this will make refactoring easier.

I agree with this. Additionally, I would also expose option and separator methods on the menu so the menu can be built. Something like this:

local menu = Menu:new({ 
  title = 'Some title',
  items = { ... }, -- optional
  prompt = 'Prompt',
  separator = '-',
})

menu:option(label, key, action)
menu:separator(length, char_override)
menu:option(label, key, action)

return menu:open()

[danilshvalov]

@kristijanhusak This is a great idea! But maybe it would be better to name the methods add_option and add_separator instead of option and separator for more clarity? For example, separator might appear to change the value of the separator itself rather than adding it to the menu.

[danilshvalov]

And it's probably better to pass a delimiter to the Menu constructor like this:

local menu = Menu:new({
  -- ...
  separator = {
    icon = '~', -- optional, default is `-`
    length = 1, -- optional, default is 80
  },
})

[kristijanhusak]

@kristijanhusak This is a great idea! But maybe it would be better to name the methods add_option and add_separator instead of option and separator for more clarity? For example, separator might appear to change the value of the separator itself rather than adding it to the menu.

Sure, that also works.

And it's probably better to pass a delimiter to the Menu constructor like this:

local menu = Menu:new({
  -- ...
  separator = {
    icon = '~', -- optional, default is `-`
    length = 1, -- optional, default is 80
  },
})

This is also ok. add_separator method should accept both of these options as an override, length and then icon because we have situations where we have different length separators in the menu.

[danilshvalov]

I did what we discussed. Review please.

[kristijanhusak]

Looks good, just few small changes regarding the arguments.

[danilshvalov]

Ready for review

[danilshvalov]

I also did a minor refactoring in the default menu.

[kristijanhusak]

Looks great, thanks!