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

feat: add option to set last active window as target for file actions #3008

Closed
wants to merge 2 commits into from
Closed

feat: add option to set last active window as target for file actions #3008

wants to merge 2 commits into from

Conversation

devxpain
Copy link
Contributor

  • Add new option set_last_win_as_target to opts.actions
  • Update documentation to reflect new option
  • Set default value of set_last_win_as_target to true

- Add new option `set_last_win_as_target` to `opts.actions`
- Update documentation to reflect new option
- Set default value of `set_last_win_as_target` to `true`
@alex-courtis
Copy link
Member

Does api.node.open.no_window_picker() achieve the functionality you are after?

The actions.open_file.window_picker apply even when the window_picker.enable is false, as does the above API. This needs to be properly documented, see similar #3005

@@ -570,6 +570,7 @@ Following is the default configuration. See |nvim-tree-opts| for details. >lua
},
actions = {
use_system_clipboard = true,
set_last_win_as_target = true,
Copy link
Member

Choose a reason for hiding this comment

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

This would be a change to existing behaviour; we'll need to default to false.

@@ -232,6 +232,17 @@ local function setup_autocommands(opts)
end,
})
end

if opts.actions.set_last_win_as_target then
create_nvim_tree_autocmd("BufLeave", {
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid autocommands; this should be done in open-file.lua

create_nvim_tree_autocmd("BufLeave", {
callback = function()
local buf_name = vim.api.nvim_buf_get_name(0)
if not string.find(buf_name, "NvimTree_") then
Copy link
Member

Choose a reason for hiding this comment

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

utils.is_nvim_tree_buf should be used.

if opts.actions.set_last_win_as_target then
create_nvim_tree_autocmd("BufLeave", {
callback = function()
local buf_name = vim.api.nvim_buf_get_name(0)
Copy link
Member

Choose a reason for hiding this comment

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

bufnr / bufname should come from the ev argument passed. See :help nvim_create_autocmd()

@alex-courtis
Copy link
Member

Looking at similar issue #2262 vim.fn.winnr("#") may be a more reliable approach.

- Changed the default value of `set_last_win_as_target` back to `false` to maintain original behavior.
- Moved the autocmd for `set_last_win_as_target` to `open-file.lua` for better organization.
@devxpain
Copy link
Contributor Author

I’ve updated the code as requested and reviewed the behavior of api.node.open.no_window_picker(). I didn’t encounter any issues.

Looking at similar issue #2262, I see vim.fn.winnr("#") mentioned as a more reliable approach. I’m not sure how it applies to this pull request. Could you clarify or provide an example?

Let me know if there’s anything else that needs adjustment!

@alex-courtis
Copy link
Member

alex-courtis commented Nov 22, 2024

Looking at similar issue #2262, I see vim.fn.winnr("#") mentioned as a more reliable approach. I’m not sure how it applies to this pull request. Could you clarify or provide an example?

The idea is to use the actual previous window, according to nvim, rather than the one tracked via lib.target_winid.

It could be something like:

local function get_target_winid(mode)
  local target_winid
  if not M.window_picker.enable or mode == "edit_no_picker" or mode == "preview_no_picker" then
    target_winnr = vim.fn.winnr("#")

We would likely need to check that against usable_win_ids and fall back to first.

Looking through other issues it seems that this is not the best approach: #2262 (comment)

This whole area of functionality is a mess and needs a rewrite. Let's stick with tracking for now; it will be rewritten as part of #2255

if opts.actions.set_last_win_as_target then
vim.api.nvim_create_autocmd("BufLeave", {
callback = function()
if utils.is_nvim_tree_buf then
Copy link
Member

@alex-courtis alex-courtis Nov 23, 2024

Choose a reason for hiding this comment

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

This always evaluates to true, as it's not actually calling the function.

This will always set target_winid on leaving a buffer, and will set it to special buffers such as help. That will violate existing selection criteria like actions.open_file.window_picker.exclude.

@alex-courtis
Copy link
Member

I'm sorry, this is not a change we should make:

I think the best option for you is to write a actions.open_file.window_picker.picker function that exactly meets your use case.
You could perform your own window tracking and return your desired choice from that function.
You can even invoke a window picker in situations you choose e.g. s1n7ax/nvim-window-picker

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.

2 participants