[WIP] Feature: Comment toggling by glennsl · Pull Request #188 · onivim/libvim

glennsl · 2020-03-24T15:21:21Z

This adds a new operator, gc/gcc, which will comment or uncomment the current or selected lines. It also adds a new function to the API, vimOptionSetLineComment to specify the comment string that will be used for the current buffer.

The way it works is a bit crude currently, but it does the job. The comment string will only be inserted (and checked) at the very beginning of each line, ignoring indentation, and lines are commented or uncommented individually rather than as a block. If I can get a decent workflow going I might come back to this to give it some more polish. But until then it's fair game if anyone else wants a go at it.

glennsl · 2020-03-24T15:30:41Z

+  }
+}
+
+void toggle_comment_lines(linenr_T start, linenr_T end)


These function should probably live somewhere else. I'm not sure where though.

glennsl · 2020-03-24T15:32:48Z

  int b_diff_failed; // internal diff failed for this buffer
 #endif
+
+  char_u *b_oni_line_comment;


Not sure how OK it is to add a new field to the buf_T structure. But since it's at the end it should at least have a relatively low chance of impacting FFI and such.

Thanks for calling this out. Should be safe, as we're the only consumer of the library

glennsl · 2020-03-24T15:38:09Z

I think I might have a different version of clang-format. Mine is 9.0.1

glennsl · 2020-03-24T16:39:32Z

/azp run

azure-pipelines · 2020-03-24T16:39:42Z

Azure Pipelines successfully started running 1 pipeline(s).

bryphe · 2020-03-24T18:42:52Z

+MU_TEST(test_toggle_uncommented)
+{
+  vimInput("g");
+  vimInput("c");
+  vimInput("c");
+
+  char_u *line = vimBufferGetLine(curbuf, vimCursorGetLine());
+  printf("LINE: %s\n", line);
+
+  mu_check(strcmp(line, "//This is the first line of a test file") == 0);
+}


Nice set of test cases!

bryphe · 2020-03-24T18:48:12Z

When I run this against Onivim 2 (with a resolution: "libvim": "link:../libvimsrc"), I get this error when commenting (using gcc in a Reason file):

If I then uncomment, I get a crash here:

[ERROR]    +0ms Oni2.Exception : Exception (Invalid_argument "index out of bounds"):
Raised by primitive operation at file "src/Core/BufferLine.re", line 144, characters 11-30
Called from file "src/Feature/Editor/BufferViewTokenizer.re", line 64, characters 29-75
Called from file "src/Feature/Editor/OverlaysView.re", line 75, characters 8-141
Called from file "src/Feature/Editor/EditorSurface.re", line 233, characters 4-308
Called from file "src/UI/EditorGroupView.re", line 148, characters 10-702
Called from file "src/UI/EditorLayoutView.re", line 54, characters 12-74
Called from file "list.ml", line 92, characters 20-23
Called from file "src/UI/EditorLayoutView.re", line 37, characters 2-583
Called from file "src/UI/EditorLayoutView.re", line 66, characters 17-46
Called from file "src/UI/EditorView.re", line 38, characters 17-43
Called from file "src/UI/Root.re", line 116, characters 8-28
Called from file "src/bin_editor/Oni2_editor.re", line 138, characters 19-47
Called from file "src/Core/Tick.re", line 112, characters 9-15
Called from file "src/Core/Tick.re", line 90, characters 15-32
Called from file "list.ml", line 92, characters 20-23
Called from file "list.ml", line 92, characters 32-39
Called from file "src/Core/Tick.re", line 99, characters 22-52
Called from file "src/Core/App.re", line 236, characters 4-23
Called from file "src/sdl2.re", line 648, characters 10-20
Called from file "src/bin_editor/Oni2_editor.re", line 240, characters 2-18

glennsl · 2020-03-24T20:28:02Z

Hmm, that's unfortunate. I can easily reproduce. I'll have to try to make this a failing test.

Interesting though that there's no error in visual mode. Although it still crashes when uncommenting.

bryphe · 2020-03-24T20:51:55Z

Hmm, that's unfortunate. I can easily reproduce. I'll have to try to make this a failing test.

Right, it was strange to me, because I see a test case that exercises the un-commenting. It seems to me, based on the exception, an issue with the cursor position.

We use the curwin->w_cursor.col to get the cursor position - I wonder if that could be out of sync? It might be worth adding a case to validate the cursor position is expected (calling vimCursorGetColumn will give the same byte-position that reason-libvim/oni2 use)

Interesting though that there's no error in visual mode. Although it still crashes when uncommenting.

It must be hitting this condition here:

libvim/src/ops.c

Line 159 in 3c9fe45

if (i == (int)(sizeof(opchars) / sizeof(char[3]) - 1))

Although it's not clear to me why; as the sizeof should be updated with your extra entry in the opchars table 🤔

glennsl · 2020-03-24T21:01:30Z

We use the curwin->w_cursor.col to get the cursor position - I wonder if that could be out of sync? It might be worth adding a case to validate the cursor position is expected (calling vimCursorGetColumn will give the same byte-position that reason-libvim/oni2 use)

Good idea. Will try that!

Although it's not clear to me why; as the sizeof should be updated with your extra entry in the opchars table thinking

It's being called with cc for some reason, rather than gc. So I must still be missing some wiring somewhere.

glennsl · 2020-03-24T22:06:42Z

I believe I've fixed both, but I'm not able to get esy to rebuild linked dependencies, so I'm unable to test it in Oni2 proper

bryphe · 2020-03-25T13:53:15Z

Thanks for fixing those last issues, @glennsl ! I picked up the latest and it is working great - undo, redo, motions, macros🥇

Very excited to have this in!

glennsl · 2020-03-25T14:22:32Z

Woohoo! Will you do a release so I can pick it up properly in reason-libvim and oni2 as well?

bryphe · 2020-03-25T14:25:23Z

Done! Released as libvim@8.10869.35

glennsl added 5 commits March 19, 2020 16:03

toggle single line

0b61663

toggle multiple lines at once!

04b7a6c

add vimOptionSetLineComment

7690499

use specified line comment string when toggling

d75878f

add undo tests

1871019

glennsl requested a review from bryphe March 24, 2020 15:21

formatting

2b864d3

glennsl commented Mar 24, 2020

View reviewed changes

glennsl added 2 commits March 24, 2020 16:33

remove stray printf

68b482b

unformatting

c2ef0ff

bryphe reviewed Mar 24, 2020

View reviewed changes

glennsl added 2 commits March 24, 2020 22:24

fix get_op_type error

495528d

set cursor to beginning after toggle

27cfd0b

bryphe reviewed Mar 24, 2020

View reviewed changes

Comment thread src/normal.c

bryphe reviewed Mar 24, 2020

View reviewed changes

Comment thread src/normal.c

bryphe reviewed Mar 24, 2020

View reviewed changes

Comment thread src/normal.c

glennsl added 2 commits March 25, 2020 11:36

mark dirty lines properly

bca12bf

save undo state

6ac48e6

glennsl force-pushed the feature/op/comment-toggling branch from b29204a to 6ac48e6 Compare March 25, 2020 10:44

bryphe approved these changes Mar 25, 2020

View reviewed changes

glennsl merged commit 9d3b7fb into onivim:master Mar 25, 2020

glennsl deleted the feature/op/comment-toggling branch March 25, 2020 14:22

Uh oh!

Conversation

glennsl commented Mar 24, 2020

Uh oh!

glennsl Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

glennsl Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryphe Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

glennsl commented Mar 24, 2020

Uh oh!

glennsl commented Mar 24, 2020

Uh oh!

azure-pipelines Bot commented Mar 24, 2020

Uh oh!

bryphe Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

bryphe commented Mar 24, 2020

Uh oh!

glennsl commented Mar 24, 2020

Uh oh!

bryphe commented Mar 24, 2020

Uh oh!

glennsl commented Mar 24, 2020

Uh oh!

glennsl commented Mar 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryphe commented Mar 25, 2020

Uh oh!

glennsl commented Mar 25, 2020

Uh oh!

bryphe commented Mar 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

glennsl Mar 24, 2020 •

edited

Loading