debug: configurable key mappings #3035

kernel-panic96 · 2020-10-01T21:01:05Z

kernel-panic96 · 2020-10-01T21:08:14Z

autoload/go/debug.vim

+    \["nmap", "<F5>",  "<Plug>(go-debug-continue)"],
+    \["nmap", "<F6>",  "<Plug>(go-debug-print)"],
+    \["nmap", "<F9>",  "<Plug>(go-debug-breakpoint)"],
+    \["nmap", "<F10>", "<Plug>(go-debug-next)"],
+    \["nmap", "<F11>", "<Plug>(go-debug-step)"],


This slightly differs from the default. I tested it with <buffer> but when I step into some file the buffer changes and the mappings are no longer active, thus I decided to leave it without the buffer attribute

Ideally, IMO, the behavior should be that these mappings will be active only in a certain tab or split. E.g. I have the n key mapped to step over in debug mode and I usually have the n key mapped to next search result, I would like the semantics of n in other tabs to be those from my normal config, so I could search for things.

Unfortunately I don't see a way of doing this. WDYT?

kernel-panic96 · 2020-10-01T21:29:36Z

The CI fails with:

--- FAIL Test_GoTermReuse (0.155905s)
vim-go: [go run '/tmp/vim-go-test/testrun/0cd3f7c6e22668318f531ac9655e91b8e94221ce94fb1355844e31d6066c7407/src/term/term.go'] FAIL
function Test_GoTermReuse line 20: Expected 3 but got 4
function Test_GoTermReuse line 26: Expected 3 but got 4
FAIL term_test.vim 0.481729s / 3 tests

At a first glance this doesn't seem related to the changes in the PR

EDIT: Is there a way to restart the CI? It passed the first time.

bhcleek

Making the mappings local to the buffer is critical.

Thanks for contributing @kernel-panic96 🙇

Can you put the debug_mode.vim contents into debug.vim and make the public functions script-scoped functions instead?

Please don't change the location of where the mappings are applied and cleared; debugging is surprisingly finicky, and the placement of those operations has been a deliberate choice to make sure that mappings are available at the right times in the debugging lifecycle. I suspect this means that the mappings list will need converted to a dictionary where each operations' name is used as a key (e.g. {'go-debug-continue': ["nmap", "<F5>"]}, though I think that the value may be able to just be the keybindings; I can't think of a reason to use anything other than nmap).

The problems that you had with using buffer-local mappings is because the doautocmd is no longer applied. If you'll make the changes I requested in the previous paragraph, we should be able to make the changes buffer-local when the user's preferred keys are processed.

Also, please do not remove the autogroup. Instead, leverage it as it was leveraged before, just with with customizations.

Please use execute() instead of :execute in VimL.

If you'd prefer to be done with this PR, I can take over and make the changes I've requested.

bhcleek · 2020-10-01T21:29:34Z

autoload/go/debug_mode.vim

+function! go#debug_mode#Restore(...) abort
+  for [lhs, save] in items(s:mappings_save)
+    let command = s:restore_mapping(lhs, save)
+    silent! execute command


use execute() here instead of normal mode commands.

bhcleek · 2020-10-01T22:14:11Z

At a first glance this doesn't seem related to the changes in the PR
EDIT: Is there a way to restart the CI? It passed the first time.

Those failures seem unrelated to your changes so far. That failing test has been flaky recently. I thought I'd resolved it in a recent PR, though; perhaps you're work is based on an older commit? If so, that's ok, there's no need to rebase or merge master into your branch.

kernel-panic96 · 2020-10-02T17:21:50Z

I'm fine with you finishing it up. I tried and ran into some race conditions where the backing of user mappings happens twice and when you try to restore them they get restored to the debugger mappings which is not what you would want.

Currently the code is structured with the assumption that the setup & teardown would be executed only once. Making <buffer> work by executing it continually requires a violation of this assumption. I tried adding some guards to backup the user mappings if they weren't backed up before, but I ran in some race conditions. I'll try to test it before the merge

bhcleek · 2020-10-02T23:56:16Z

I'm fine with you finishing it up.

Ok, I'll take over from here and make some changes. Are you ok if I push to your branch?

kernel-panic96 · 2020-10-03T07:30:29Z

I'm fine with you finishing it up.

Ok, I'll take over from here and make some changes. Are you ok if I push to your branch?

Yes.

kernel-panic96 · 2020-10-10T08:13:01Z

I'm willing to give it another try. Do you have unpushed code?

bhcleek · 2020-10-10T15:23:10Z

I just finished up the major refactor last night. I need to do some testing locally, and then I'll push changes; I'll likely do that today or tomorrow.

Thank you for offering to test; that will be very helpful.

bhcleek · 2020-10-12T02:54:55Z

@kernel-panic96 I've created a pull request against your branch. Please don't merge it to your branch regardless of your testing results. After you test, I can push directly to your branch instead and then merge this if your testing is successful.

bhcleek · 2020-10-15T02:44:03Z

@kernel-panic96 I'm going to rebase this onto master to resolve conflicts and push to your branch before I merge.

Move the modeline to the end of the file and add expandtab. Add the usual header for vim-go.

Update docs for g:go_debug_mappings to reflect the desired state.

Refactor debug mapping to be normal mode buffer-local mappings only. Restore mappings in go#debug#Stop instead of s:stop() for consistency of purpose.

Fix unmapping of a long-standing unreported bug where the debugging mappings were not being unmapped when debugging stops.

Move debug mapping functions into debug.vim and change their names appropriately. No functional changes were made.

Remove <Plug> mapping on stop. This was mistakenly removed in a recent comment that changed the code that removes mapping so that only the key mappings were removed. Add a test to ensure <Plug> mappings are removed. It turns out that the previous commit should have added the removal of key mappings and kept the removal of the <Plug> mappings. This also fixes a bug with the previous implementation of removal of <Plug> mappings to ensure that the first <Plug> mapping is also removed.

Copy g:go_debug_mappings before extending it so that the extension merge with the user's value.

Do not reset s:mappings on each call to s:configureMappings so that opening a new file doesn't reset the value.

The FileType events are not buffer-local events, so in order to reset them, the autocmd! command must not be restricted to buffer-local events.

Set the arguments before lhs.

bhcleek · 2020-10-15T02:53:10Z

Thank you @kernel-panic96 !

kernel-panic96 · 2020-10-15T05:45:32Z

Thank you for the collaboration @bhcleek . It was awesome 👍 .

kernel-panic96 commented Oct 1, 2020

View reviewed changes

bhcleek reviewed Oct 1, 2020

View reviewed changes

bhcleek added this to the vim-go 1.25 milestone Oct 2, 2020

kernel-panic96 marked this pull request as draft October 3, 2020 07:30

bhcleek added the hacktoberfest-accepted label Oct 4, 2020

bhcleek changed the title ~~Debug mode~~ debug: configurable key mappings Oct 15, 2020

bhcleek marked this pull request as ready for review October 15, 2020 02:43

yrl and others added 14 commits October 14, 2020 19:46

Implement debug mode key mappings

13f7ea3

Reformat with sw=2 ts=2

d357283

Cleanup. Remove unnecessary testing code

bf0f9e3

lint fix. Line width

fc26226

use execute() instead of normal mode command

2702e13

Use the usual vim-go script header and footer

db399f5

Move the modeline to the end of the file and add expandtab. Add the usual header for vim-go.

doc: update docs for g:go_debug_mappings

e30ede1

Update docs for g:go_debug_mappings to reflect the desired state.

debug: refactor debug mapping handling

f52298e

Refactor debug mapping to be normal mode buffer-local mappings only. Restore mappings in go#debug#Stop instead of s:stop() for consistency of purpose.

debug: fix unmapping of debugging mappings

1d1cf95

Fix unmapping of a long-standing unreported bug where the debugging mappings were not being unmapped when debugging stops.

debug: move mapping functions into debug.vim

b9ab47a

Move debug mapping functions into debug.vim and change their names appropriately. No functional changes were made.

debug: unmap debug mappings in restoreMappings()

6d662b3

debug: fix debug mapping name

8930a38

config: make g:go_debug_mappings values dictionaries

0654d3f

bhcleek added 6 commits October 14, 2020 19:46

config: copy g:go_debug_mappings

470256c

Copy g:go_debug_mappings before extending it so that the extension merge with the user's value.

debug: do not reset s:mappings

733af48

Do not reset s:mappings on each call to s:configureMappings so that opening a new file doesn't reset the value.

debug: clear vim-go-debug correctly

1a6ba2e

The FileType events are not buffer-local events, so in order to reset them, the autocmd! command must not be restricted to buffer-local events.

debug: fix construction of mapping with arguments

2229855

Set the arguments before lhs.

doc: wrap long line

71cd31c

debug: fix tests after proper key mapping handling

7b9a982

bhcleek force-pushed the debug-mode branch from b55d5e5 to 7b9a982 Compare October 15, 2020 02:47

bhcleek merged commit 409ef4c into fatih:master Oct 15, 2020

bhcleek added a commit that referenced this pull request Oct 15, 2020

update CHANGELOG.md for #3035

c5f8e42

This was referenced Oct 15, 2020

debug: restore all buffers' mappings #3048

Merged

Debugger mappings should be configurable #1788

Closed

kernel-panic96 deleted the debug-mode branch February 15, 2023 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debug: configurable key mappings #3035

debug: configurable key mappings #3035

kernel-panic96 commented Oct 1, 2020

kernel-panic96 Oct 1, 2020 •

edited

Loading

kernel-panic96 Oct 1, 2020 •

edited

Loading

kernel-panic96 commented Oct 1, 2020 •

edited

Loading

bhcleek left a comment

bhcleek Oct 1, 2020

bhcleek commented Oct 1, 2020

kernel-panic96 commented Oct 2, 2020 •

edited

Loading

bhcleek commented Oct 2, 2020

kernel-panic96 commented Oct 3, 2020

kernel-panic96 commented Oct 10, 2020

bhcleek commented Oct 10, 2020

bhcleek commented Oct 12, 2020

bhcleek commented Oct 15, 2020 •

edited

Loading

bhcleek commented Oct 15, 2020

kernel-panic96 commented Oct 15, 2020

debug: configurable key mappings #3035

debug: configurable key mappings #3035

Conversation

kernel-panic96 commented Oct 1, 2020

kernel-panic96 Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

kernel-panic96 Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

kernel-panic96 commented Oct 1, 2020 • edited Loading

bhcleek left a comment

Choose a reason for hiding this comment

bhcleek Oct 1, 2020

Choose a reason for hiding this comment

bhcleek commented Oct 1, 2020

kernel-panic96 commented Oct 2, 2020 • edited Loading

bhcleek commented Oct 2, 2020

kernel-panic96 commented Oct 3, 2020

kernel-panic96 commented Oct 10, 2020

bhcleek commented Oct 10, 2020

bhcleek commented Oct 12, 2020

bhcleek commented Oct 15, 2020 • edited Loading

bhcleek commented Oct 15, 2020

kernel-panic96 commented Oct 15, 2020

kernel-panic96 Oct 1, 2020 •

edited

Loading

kernel-panic96 Oct 1, 2020 •

edited

Loading

kernel-panic96 commented Oct 1, 2020 •

edited

Loading

kernel-panic96 commented Oct 2, 2020 •

edited

Loading

bhcleek commented Oct 15, 2020 •

edited

Loading