Skip to content

fix: hijack_cursor with update_focused_file #2600

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

Merged
merged 1 commit into from
Dec 31, 2023

Conversation

geril2207
Copy link
Collaborator

Fix: #2599

@alex-courtis
Copy link
Member

Unfortunately this doesn't always work. It seems the issue might be with place_cursor_on_node

2599.log.patch.gz

[2023-12-26 09:51:52] [dev] hijacking {
  buf = 2,
  event = "BufEnter",
  file = "/home/alex/src/linux/NvimTree_1",
  group = 8,
  id = 14,
  match = "/home/alex/src/linux/NvimTree_1"
}
[2023-12-26 09:51:52] [dev] place_cursor_on_node
[2023-12-26 09:51:52] [dev] hijacking {
  buf = 2,
  event = "CursorMoved",
  file = "/home/alex/src/linux/NvimTree_1",
  group = 8,
  id = 14,
  match = "/home/alex/src/linux/NvimTree_1"
}
[2023-12-26 09:51:52] [dev] place_cursor_on_node
...
[2023-12-26 09:52:00] [dev] hijacking {
  buf = 2,
  event = "CursorMoved",
  file = "/home/alex/src/linux/NvimTree_1",
  group = 8,
  id = 14,
  match = "/home/alex/src/linux/NvimTree_1"
}
[2023-12-26 09:52:00] [dev] place_cursor_on_node
[2023-12-26 09:52:00] [dev] nvim_win_set_cursor(0, { 35, 6 })

@alex-courtis
Copy link
Member

It also looks like the BufEnter is redundant, as CursorMoved is documented to fire when moved to another window.

@geril2207
Copy link
Collaborator Author

geril2207 commented Dec 26, 2023

Unfortunately this doesn't always work. It seems the issue might be with place_cursor_on_node

I don't find any issues, but yeah, there are better ways to resolve, which require more code to change.
Why do we need the following?

local prev_line
function M.place_cursor_on_node()
local l = vim.api.nvim_win_get_cursor(0)[1]
if l == prev_line then
return
end
prev_line = l

For me removing this check fixes the issue. Git blame says it was search fix, can you describe more about this? How can we change it. Actually it is, without this check you can't go next in search using n but can go prev with N. Seems that we need to find better way to handle if search with hijack_cursor.

@alex-courtis
Copy link
Member

alex-courtis commented Dec 30, 2023

For me removing this check fixes the issue. Git blame says it was search fix, can you describe more about this? How can we change it.

That is very old code indeed, unfortunately I have no insight. It could be from a time when there was no root node or there was something else on the top line, perhaps the search input.

Actually it is, without this check you can't go next in search using n but can go prev with N. Seems that we need to find better way to handle if search with hijack_cursor.

Correct me if I'm wrong, but it looks like we don't have any search n or N mappings or functionality, just a one off search and find.

I don't find any issues, but yeah, there are better ways to resolve, which require more code to change.

Yes please. It's actually failing to hijack the root node itself.

This is only called in one place and we can easily test it. Do you want to just rewrite that method? It's clear what it needs to do and can be very simple.

@geril2207
Copy link
Collaborator Author

Correct me if I'm wrong, but it looks like we don't have any search n or N mappings or functionality, just a one off search and find.

I am about native neovim / or ? search where you have native n and N binds for jump next/prev. I don't think we should break it. And with removing line check, we will break n bind completely, you can't jump to the next occurence if current one not on the start.

@alex-courtis
Copy link
Member

I am about native neovim / or ? search where you have native n and N binds for jump next/prev. I don't think we should break it. And with removing line check, we will break n bind completely, you can't jump to the next occurence if current one not on the start.

Ah I see, I completely misunderstood. I was thinking about S search rather than reglar vim search. Yes, we should not break that.

This is quite tricky. The prev_line local is quite hacky and will break when switching nvim-tree windows / tabs etc.

Do you want to try writing a "clean" implementation? It seems clear what the requirements are now.

@geril2207
Copy link
Collaborator Author

This is quite tricky. The prev_line local is quite hacky and will break when switching nvim-tree windows / tabs etc.
Do you want to try writing a "clean" implementation? It seems clear what the requirements are now.

I don't think we need completely rewrite impl, just to figure out how to handle search better. Any ideas, how to detect if search, or maybe detect params of text under cursor? Currently I have no way to solve it.

@alex-courtis
Copy link
Member

Any ideas, how to detect if search, or maybe detect params of text under cursor? Currently I have no way to solve it.

Ah yes, that would do it.

Looking at lualine it's using hlsearch and seems functional: https://github.com./nvim-lualine/lualine.nvim/blob/master/lua/lualine/components/searchcount.lua

@geril2207
Copy link
Collaborator Author

My assumption is that we can get search entries through extmarks and if cursor under search extmark we skip hijacking.

@alex-courtis
Copy link
Member

My assumption is that we can get search entries through extmarks and if cursor under search extmark we skip hijacking.

Yes please... that will inform us whether we are searching inside the nvim-tree window AND we are on the match.

@geril2207
Copy link
Collaborator Author

geril2207 commented Dec 30, 2023

Seems that we can't do that with extmarks. I checked vim.fn.searchcount and seems that we can use exact_match from the result. From my tests, if cursor under search word exact_match eq to 1. In this case we can skip hijacking. I can do rewrite with vim.fn.searchcount to test if it solves, but not now.

@geril2207 geril2207 force-pushed the fix/hijack_cursor-with-update-focused branch from 6a31f24 to 5659f78 Compare December 30, 2023 09:04
@geril2207 geril2207 force-pushed the fix/hijack_cursor-with-update-focused branch from 5659f78 to 24b5a11 Compare December 30, 2023 09:06
@geril2207
Copy link
Collaborator Author

So what do you think about current solution?

@alex-courtis
Copy link
Member

So what do you think about current solution?

Well, that's rather elegant. Cursor is always correctly placed on n/N and is hijacked on j/k

  • update_focused_file T/F
  • hijack_cursor T/F
  • &wrapscan on/off
  • root_folder_label = false

Regression: user can't move horizontally on a node with h/l.
Looking at the documentation "Keeps the cursor on the first letter of the filename when moving in the tree."
I'm happy to run with this new behaviour - it matches the documentation.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Fantastic work as always @geril2207

@alex-courtis alex-courtis merged commit 02ae523 into master Dec 31, 2023
@alex-courtis alex-courtis deleted the fix/hijack_cursor-with-update-focused branch December 31, 2023 03:40
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.

hijack_cursor with update_focused_file.enable does not set cursor on focus/toggle
2 participants