Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(toggleterm)!: simplified keybinds with removed abstraction #3812

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

opalmay
Copy link
Contributor

@opalmay opalmay commented Feb 4, 2023

  1. Users can now add keybinds more easily to lvim.builtin.terminal.terminals
    eg:
local terminals = {
  { keymap = "<leader>u", cmd = "sudo dmesg | less", desc = "Show Kernel Logs", size = 1 },
  { keymap = "<leader>h", cmd = "htop", desc = "Interactive System Monitor", size = 0.8 },
  {
    keymap = "<leader>os",
    direction = "vertical",
    cmd = "docker ps",
    desc = "List Docker Containers",
    close_on_exit = false,
    persist_mode = false,
    size = 0.3,
  },
  { keymap = "<M-4>", cmd = "neofetch", close_on_exit = false, direction = "horizontal", size = 0.6 },
}
vim.list_extend(lvim.builtin.terminal.terminals, terminals)
  1. lvim.builtin.terminal.terminals_defaults is a table containing defaults for direction, horizontal_size, vertical_size that can be modified by the user.
  2. lvim.builtin.terminal.execs is handled with soft deprecation and link to the docs docs: terminal keybinds lunarvim.org#367
  3. There's no abstraction, all parameters are passed as is to toggleterm:new directly. One exception is size which is calculated as a percentage of the buffer size in the terminal direction (same as it was) and size = 1 being a special case for a full screen terminal (like lazygit). desc is being used for the keybind description, can be explicitly provided, otherwise it is based on the keybind cmd.
  4. Refactored lazygit to use this instead of a custom function.
  5. Changed lvim.builtin.terminal from bracket to dot notation for intellisense.

Depends on #3811

@opalmay opalmay changed the title Refactor toggleterm exec refactor(toggleterm)!: execs better syntax Feb 4, 2023
@opalmay
Copy link
Contributor Author

opalmay commented Feb 5, 2023

@LostNeophyte How do you think we should handle deprecation here?
Probably the easiest would be to call this something different than execs, so it can be addressed in _deprecated.lua by the old table name

Copy link
Collaborator

@kylo252 kylo252 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to add a breaking change to tweak this wrapper, as we mostly use this table as only to alter some common defaults.

At the very least, we can soft-deprecate the original table without doing a breaking change.

Alternatively, we can leave this as is, and suggest using the full API from toggle-term instead.

@opalmay opalmay marked this pull request as ready for review February 7, 2023 19:25
@opalmay opalmay changed the title refactor(toggleterm)!: execs better syntax refactor(toggleterm)!: better keybinds Feb 7, 2023
@opalmay opalmay changed the title refactor(toggleterm)!: better keybinds refactor(toggleterm)!: simplified keybinds configuration with removed abstraction Feb 7, 2023
@opalmay opalmay changed the title refactor(toggleterm)!: simplified keybinds configuration with removed abstraction refactor(toggleterm)!: simplified keybinds with removed abstraction Feb 7, 2023
lua/lvim/core/which-key.lua Show resolved Hide resolved
lua/lvim/core/terminal.lua Outdated Show resolved Hide resolved
lua/lvim/core/terminal.lua Outdated Show resolved Hide resolved
@opalmay opalmay requested a review from kylo252 February 16, 2023 19:41
lua/lvim/core/terminal.lua Outdated Show resolved Hide resolved
lua/lvim/core/terminal.lua Outdated Show resolved Hide resolved
lua/lvim/core/terminal.lua Outdated Show resolved Hide resolved
lua/lvim/core/terminal.lua Show resolved Hide resolved
term_opts.direction = term_opts.direction or lvim.builtin.terminal.terminals_defaults.direction
term_opts.size = term_opts.size or lvim.builtin.terminal.terminals_defaults[term_opts.direction .. "_size"]
-- size is calculated dynamically as a percentage of the current buffer
term_opts.size = get_dynamic_terminal_size(term_opts.direction, term_opts.size)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be a callback

Suggested change
term_opts.size = get_dynamic_terminal_size(term_opts.direction, term_opts.size)
term_opts.size = function()
get_dynamic_terminal_size(term_opts.direction, term_opts.size)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? It doesn't work and it works without a callback

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically, it won't be dynamically calculated if it's not a function.

lua/lvim/core/terminal.lua Outdated Show resolved Hide resolved
lua/lvim/core/terminal.lua Outdated Show resolved Hide resolved
lua/lvim/core/terminal.lua Show resolved Hide resolved
@opalmay opalmay force-pushed the refactor-toggleterm-exec branch 3 times, most recently from 745e227 to b0fc939 Compare March 6, 2023 18:07
@opalmay opalmay requested a review from kylo252 March 6, 2023 18:10
opalmay and others added 10 commits April 12, 2023 13:56
Co-authored-by: kylo252 <59826753+kylo252@users.noreply.github.com>
Co-authored-by: kylo252 <59826753+kylo252@users.noreply.github.com>
Co-authored-by: kylo252 <59826753+kylo252@users.noreply.github.com>
Co-authored-by: kylo252 <59826753+kylo252@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants