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

The mapping does not work as designed in debug mode #899

Closed
2 tasks done
YuCao16 opened this issue Jul 27, 2023 · 34 comments · Fixed by #902 or #907
Closed
2 tasks done

The mapping does not work as designed in debug mode #899

YuCao16 opened this issue Jul 27, 2023 · 34 comments · Fixed by #902 or #907
Labels
bug Something isn't working dap Debug Adapter related issues has:repro This issue contains reproducing steps keymap Keymap related issues nvim:nightly Compatibility issues caused by nightly Neovim builds

Comments

@YuCao16
Copy link
Contributor

YuCao16 commented Jul 27, 2023

Version confirmation

  • Confirm

Following prerequisites

  • Confirm

Neovim version

NVIM v0.10.0-dev-618+g4fd852b8c

Operating system/version

Pop!_OS 22.04 LTS x86_64

Terminal name/version

kitty 0.28.1

$TERM environment variable

xterm-kitty

Branch info

main (Default/Latest)

Fetch Preferences

SSH (use_ssh = true)

How to reproduce the issue

  1. Go to Debug mode
  2. Press K in code buffer

Expected behavior

K is bound to lua require("dapui").eval()

Actual behavior

K is bound to `Lspsaga hover_doc'

Additional information

In the most recent version, 'K' has been bound to lua require("dapui").eval(). However, in the main code window, this mapping has not been overwritten. There are two reasons for this:

  1. Some LSP clients, such as copilot, are attached after entering debug mode.
  2. LSP-related mappings are not overwritten. I need to specify the 'bufnr' for it to work.
   ["n|K"] = map_cmd("<Cmd>lua require('dapui').eval()<CR>")
   	:with_buffer(0)
   	:with_noremap()
   	:with_nowait()
   	:with_desc("debug: Evaluate expression under cursor"),

I'm not sure if this is a real issue or a conflict between my own configuration and the current version. Could you please test it?

@YuCao16 YuCao16 added the bug Something isn't working label Jul 27, 2023
@Jint-lzxy
Copy link
Collaborator

Cannot repro. We did test this before merging #877.

This is a global keymap, so it's impossible for a specific buffer to have a different definition (unless overridden). And theoretically no LSP activity is allowed during this period. Can you provide a minimum repro so that we can look into this issue?

@Jint-lzxy Jint-lzxy added needs:repro We need more details to reproduce this problem keymap Keymap related issues dap Debug Adapter related issues labels Jul 27, 2023
@CharlesChiuGit
Copy link
Collaborator

How about K in visual mode? does that works?

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

How about K in visual mode? does that works?

Yes, it works as expect in visual mode.

@CharlesChiuGit
Copy link
Collaborator

CharlesChiuGit commented Jul 27, 2023

I'll test this keymap again tomorrow, I dont have the env setup right now.
I think I encountered sth similar one or two times, but unable to reproduce it every time.

As u mentioned it might b/c lsp-related thingy is blocking the road, I think it might b/c some lsp somehow got reattach twice.
If u open a lua file, u can see two lua_ls status at the down-right corner. It's no big deal for most case tho, but we might still need to fix it after all.
I have observed this issue since #666
@Jint-lzxy can u confirm this part?

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

Cannot repro. We did test this before merging #877.

This is a global keymap, so it's impossible for a specific buffer to have a different definition (unless overridden). And theoretically no LSP activity is allowed during this period. Can you provide a minimum repro so that we can look into this issue?

I agree with you, I'm also very confused. But if I replace
https://github.com/ayamir/nvimdots/blob/a9610243ce33271effb6ea10db683adc59fb62bd/lua/modules/configs/tool/dap/dap-keymap.lua#L8-#L13
to

   ["n|K"] = map_cmd("<Cmd>lua require('dapui').eval()<CR>")
   	:with_buffer(0)
   	:with_noremap()
   	:with_nowait()
   	:with_desc("debug: Evaluate expression under cursor"),

it works (in the main code window of course).

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

I'm not sure if this issue is caused by the problem of having multiple LSPs attached or something similar, since specifying the bufnr (:with_buffer(0)) seems to resolve it. In addition, the restoration method may not be comprehensive. The original K mapping is linked to those buffers with LSPs attached, but the restoration method does not specify this.

@Jint-lzxy
Copy link
Collaborator

can u confirm this part?

Yeah I encountered this issue log ago, but forgot to port it upstream 🤦‍♂️ Thanks for the heads up! To be short, if the config (for lua_ls) is set in lspconfig, the surprising thing is, two lua_lss will be spawned (to identify this, sending lsp_hover will generate duplicate results), one is for that without config, and the other is with config. The solution is to set up lua_ls on a per-project basis. See: Jint-lzxy/nvimconfig/.luarc.json

@Jint-lzxy
Copy link
Collaborator

@YuCao16 Can u provide a minimum repro? I have the correct setup prepared, but somehow cannot reproduce this 😢

@Jint-lzxy
Copy link
Collaborator

demo

@CharlesChiuGit
Copy link
Collaborator

CharlesChiuGit commented Jul 27, 2023

Just guessing.

with_buffer() will make the keymap have higher priority over other non-with_buffer() keymap.

So the loading order might be:

  1. First LspAttach -> bind n|K] to Lspsaga hover_doc
  2. DAP got loaded -> bind [n|K] to lua require("dapui").eval() But since it don't have with_buffer() priority, so it can't overwrite LSP keymap.

If u give DAP keymap with_buffer() priority, then [n|K] will be decided by the loading order.

https://neovim.io/doc/user/api.html#nvim_buf_set_keymap()
I think buffer-local keymap will have higher priority then global keymap?

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

I try to create a easier way to reproduce this issue, could you please try this?

  1. add this line: ["n|<leader>pz"] = map_cr("Lazy"):with_buffer(0):with_silent():with_noremap():with_nowait():with_desc("package: Clean"), into nvim/lua/keymap/init.lua
  2. add this line ["n|<leader>pz"] = map_cmd("<Cmd>lua require('dapui').eval()<CR>"):with_silent():with_noremap():with_nowait():with_desc("package: Clean"), into nvim/lua/modules/configs/tool/dap/dap-keymap.lua line 14.

So the <leader>pz should be bound to lau require('dapui').eval() in debug mode, but in my side, it still be bound to Lazy. And in other buffer such as dapui_scopes, it has been bound to lua require('dapui').eval() correctly.

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

Just guessing.

with_buffer() will make the keymap have higher priority over other non-with_buffer() keymap.

So the loading order might be:

1. First `LspAttach` -> bind `n|K]` to `Lspsaga hover_doc`

2. DAP got loaded -> bind `[n|K]` to `lua require("dapui").eval()` But since it don't have `with_buffer()` priority, so it can't overwrite LSP keymap.

If u give DAP keymap with_buffer() priority, then [n|K] will be decided by the loading order.

https://neovim.io/doc/user/api.html#nvim_buf_set_keymap() I think buffer-local keymap will have higher priority then global keymap?

I think so ... but if so, why can't you reproduce it?

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

Shall we just use another mechanism to setup key mapping, for example, based on filetype? I can send a pr if you accept.

@Jint-lzxy
Copy link
Collaborator

but if so, why can't you reproduce it?

Yeah it's quite strange... I can reproduce #899 (comment) but the same thing doesn't apply to this issue. I'm checking the log to see if there's anything useful.

@Jint-lzxy
Copy link
Collaborator

Shall we just use another mechanism to setup key mapping, for example, based on filetype? I can send a pr if you accept.

But FWIW this keymap should be globally available in the current debug session, as the debugger could potentially switch files, and once the buffer changed, those keymaps will be invalidated.

@Jint-lzxy
Copy link
Collaborator

Jint-lzxy commented Jul 27, 2023

Oh I see where the issue is. I'm using Neovim v0.9.1 on this host, but once I switch to Neovim nighly, the problem appears:

執行 LspAttach Autocommands: "*"
autocommand <*>
autocommand <*>
...
執行 LspAttach Autocommands: "*"
autocommand <*>
...
執行 LspAttach Autocommands: "*"
autocommand <*>
...
執行 LspAttach Autocommands: "*"
autocommand <*>
...
執行 LspAttach Autocommands: "*"
autocommand <*>
......

Not sure why this event got triggered when no (new) LSP attached to that buffer...

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

Shall we just use another mechanism to setup key mapping, for example, based on filetype? I can send a pr if you accept.

But FWIW this keymap should be globally available in the current debug session, as the debugger could potentially switch files, and once the buffer changed, those keymaps will be invalidated.

What if we set keymap based on filetype, for example python and dapui_*, and use callable, define a function to pcall require('dapui').eval()? this will simply the restore function.

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

Oh I see where the issue is. I'm using Neovim v0.9.1 on this host, but once I switch to Neovim nighly, the problem appears:

執行 LspAttach Autocommands: "*"
autocommand <*>
autocommand <*>
...
執行 LspAttach Autocommands: "*"
autocommand <*>
...
執行 LspAttach Autocommands: "*"
autocommand <*>
...
執行 LspAttach Autocommands: "*"
autocommand <*>
...
執行 LspAttach Autocommands: "*"
autocommand <*>
......

Not sure why this event got triggered when no (new) LSP attached to that buffer...

V0.10.0-dev is the most bug-ridden version I've ever encountered. Almost every day I'm facing bugs that emerge from who knows where.

@Jint-lzxy
Copy link
Collaborator

What if we set keymap based on filetype, for example python and dapui_*, and use callable, define a function to pcall require('dapui').eval()? this will simply the restore function.

That thought crossed my mind as well. But the issue is, some languages may have multiple filetypes that meet the criteria (e.g., .c[pp] (c/cpp) and .h (header)). I don't think we can exhaust all those combinations, not to mention user ftplugins and bigfile.nvim.

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

What if we set keymap based on filetype, for example python and dapui_*, and use callable, define a function to pcall require('dapui').eval()? this will simply the restore function.

That thought crossed my mind as well. But the issue is, some languages may have multiple filetypes that meet the criteria (e.g., .c[pp] (c/cpp) and .h (header)). I don't think we can exhaust all those combinations, not to mention user ftplugins and bigfile.nvim.

Is require('dapui').eval() works in *.h?

@Jint-lzxy
Copy link
Collaborator

V0.10.0-dev is the most bug-ridden version I've ever encountered. Almost every day I'm facing bugs that emerge from who knows where.

TBH nightly builds have always been like this. We never know what the next "surprise" is :D

@Jint-lzxy
Copy link
Collaborator

Is require('dapui').eval() works in *.h?

If I explicitly specify *.h this works. But not the case if I only specify c/cpp.

@Jint-lzxy
Copy link
Collaborator

IMO the most effective (and "economical") way is to revert #735, but is it worth it?

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

IMO the most effective (and "economical") way is to revert #735, but is it worth it?

Just bind K to a callable function should works, we should only detect if we are in debug mode or not.

@Jint-lzxy
Copy link
Collaborator

Just bind K to a callable function should works, we should only detect if we are in debug mode or not.

In fact, this was my initial proposal (_G._debugging) but I reverted that b/c I didn't feel like introducing global variables was a good choice - that will make the base config more difficult to understand. But considering the current situation, if it takes tooooooo long for Neovim core to identify/fix this issue, this sounds appealing to me! I'll open a PR for this 😄

@Jint-lzxy Jint-lzxy removed the needs:repro We need more details to reproduce this problem label Jul 27, 2023
@Jint-lzxy Jint-lzxy added the has:repro This issue contains reproducing steps label Jul 27, 2023
@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

Just bind K to a callable function should works, we should only detect if we are in debug mode or not.

In fact, this was my initial proposal (_G._debugging) but I reverted that b/c I didn't feel like introducing global variables was a good choice - that will make the base config more difficult to understand. But considering the current situation, if it takes tooooooo long for Neovim core to identify/fix this issue, this sounds appealing to me! I'll open a PR for this smile

Maybe detect if buffer with filetype dapui_scopes exist will prevent create a global variable?

@Jint-lzxy
Copy link
Collaborator

Maybe detect if buffer with filetype dapui_scopes exist will prevent create a global variable?

Sorry I didn't get this, what's the point of detecting buffers with filetype dapui_scopes?

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

Maybe detect if buffer with filetype dapui_scopes exist will prevent create a global variable?

Sorry I didn't get this, what's the point of detecting buffers with filetype dapui_scopes?

I mean we can detect if there is a dapui's window so we know if we are in debug mode, dapui_scopes is one of them.

@Jint-lzxy
Copy link
Collaborator

I would prefer setting this up by registering callbacks during dap_initialized (post) and dap_terminated (pre) :) This would be a more generic way to achieve that.

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 27, 2023

I would prefer setting this up by registering callbacks during dap_initialized (post) and dap_terminated (pre) :) This would be a more generic way to achieve that.

Agreed, that's more decent, thanks for your effort.

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 28, 2023

@Jint-lzxy This seems happen again after merging the latest commit...

@Jint-lzxy
Copy link
Collaborator

Jint-lzxy commented Jul 28, 2023

@YuCao16 Ah yes many thanks for reminding me. This is now a specific issue for Neovim nightly, as these keymaps are set by repeated LspAttach event even after they get cleared. lemme think a bit about how to fix this.

@Jint-lzxy Jint-lzxy reopened this Jul 28, 2023
@Jint-lzxy Jint-lzxy added the nvim:nightly Compatibility issues caused by nightly Neovim builds label Jul 28, 2023
Jint-lzxy added a commit that referenced this issue Jul 28, 2023
@Jint-lzxy
Copy link
Collaborator

@YuCao16 Does #907 fix this?

@YuCao16
Copy link
Contributor Author

YuCao16 commented Jul 28, 2023

@YuCao16 Does #907 fix this?

Yes, it works!

Jint-lzxy added a commit that referenced this issue Jul 28, 2023
singlemancombat pushed a commit to singlemancombat/nvim-config that referenced this issue Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dap Debug Adapter related issues has:repro This issue contains reproducing steps keymap Keymap related issues nvim:nightly Compatibility issues caused by nightly Neovim builds
Projects
None yet
3 participants