Continue single comment #10996

TornaxO7 · 2024-06-20T01:33:32Z

Closes #1730

Continues #8664

This is my attempt to apply the changes of #4718 to the PR.

… token

…n1991/helix into feature/continue-comment

…lix into continue-single-comment

TornaxO7 · 2024-06-20T01:40:36Z

One thing which bothers me is how we want to "continue" the comment from block comments like for C of this style:

/*
 * | <-- let's say here's the cursor, how should we "recognize" this `*`? Adding another entry in `language.toml` per language which should indicate which "multiblock-continuity" there are?
 */

Should this be tackled in this PR or in another PR? Should this be tackled at all?

TornaxO7 · 2024-08-07T09:52:36Z

I don't see it as a must for this PR but definitly would be nice to have

I think, I'd like to create a followup PR for block comments after I'm done with this.

…single-comment

This reverts commit da2a444.

This reverts commit 1eb963d.

This reverts commit da2a444.

…ce a comment

the-mikedavis · 2024-10-03T17:50:43Z

helix-core/src/comment.rs

+/// Returns the matching comment token of the given line (if it exists).
+pub fn get_comment_token<'a>(
+ text: &Rope,
+ tokens: Option<&'a Vec<String>>,


I don't think we should be trying to continue comments if a language doesn't configure comment token(s). Similarly I'm not sure we should be doing anything when using C-c on plaintext or a language that doesn't define comments. I think it's fine if this feature doesn't work on plaintext.

So instead this parameter can be &'a [String]

the-mikedavis · 2024-10-03T17:51:07Z

helix-core/src/comment.rs

+
+/// Returns the matching comment token of the given line (if it exists).
+pub fn get_comment_token<'a>(
+ text: &Rope,


This can take a RopeSlice directly rather than a &Rope

the-mikedavis · 2024-10-03T17:57:05Z

helix-core/src/comment.rs

+ .unwrap_or(DEFAULT_COMMENT_TOKEN);
+
+ for token in tokens {
+ let (is_commented, _, _, _) = find_line_comment(token, text, [line_num]);


find_line_comment does a fair amount of work that isn't necessary here (discarded with the _s). I think it's fine to not reuse that function and have the relevant parts of the implementation inline:

/// Returns the longest matching comment token of the given line (if it exists). pub fn get_comment_token<'a>( text: RopeSlice, tokens: &'a [String], line_num: usize, ) -> Option<&'a str> { let mut result = None; let line = text.line(line_num); let start = line.char_to_byte(line.first_non_whitespace_char()?); for token in tokens { let end = std::cmp::min(start + token.len(), line.len_bytes()); let fragment = Cow::from(line.byte_slice(start..end)); if fragment == *token { result = Some(token.as_str()); } } result }

the-mikedavis · 2024-10-03T18:00:36Z

helix-term/src/commands.rs

@@ -3391,16 +3391,17 @@ fn open(cx: &mut Context, open: Open) {
 enter_insert_mode(cx);
 let (view, doc) = current!(cx.editor);

- let text = doc.text().slice(..);
+ let config = doc.config.load();
+ let doc_text = doc.text().slice(..);


The rename here isn't necessary, can be reset

the-mikedavis · 2024-10-03T18:06:27Z

helix-view/src/editor.rs

@@ -342,6 +346,9 @@ pub struct Config {
 deserialize_with = "deserialize_alphabet"
 )]
 pub jump_label_alphabet: Vec<char>,
+ /// Whether to prepend a comment token onto a new line that follows a commented line. (default: false)
+ #[serde(default = "default_continue_comments")]


Suggested change

#[serde(default = "default_continue_comments")]

This doesn't need a default directive for serde since Default for Config is implemented manually.

Also the doc here is outdated that the default is false. This should also be added to the book

I wonder if this needs to be configurable at all? I'm not sure it's configurable in Kakoune for example

the-mikedavis · 2024-10-03T18:17:07Z

helix-term/src/commands.rs

+ let comment_token = {
+ let tokens = doc
+ .language_config()
+ .and_then(|config| config.comment_tokens.as_ref());
+
+ get_comment_token(doc.text(), tokens, cursor_line)
+ };
+ let new_line_will_be_comment = config.continue_comments && comment_token.is_some();


We can clean this up a bit to both signal that we will be continuing the comment and carry the comment token:

let continue_comment_token = if config.continue_comments { doc.language_config() .and_then(|config| config.comment_tokens.as_ref()) .and_then(|tokens| comment::get_comment_token(text, tokens, cursor_line)) } else { None };

then in the match below we check if continue_comment_token.is_some() instead

the-mikedavis · 2024-10-03T18:17:43Z

helix-term/src/commands.rs

+ let indent = {
+ let line = doc_text.line(cursor_line);


style nit: we try to avoid using blocks for bindings like this. It's fine for line to escape into the outer scope

the-mikedavis · 2024-10-03T18:28:22Z

helix-term/src/commands.rs

@@ -3459,6 +3483,9 @@ fn open(cx: &mut Context, open: Open) {
 transaction = transaction.with_selection(Selection::new(ranges, selection.primary_index()));

 doc.apply(&transaction, view.id);
+
+ // Since we might have added a comment token, move to the end of the line.
+ goto_line_end_newline(cx);


Rather than jumping to the end of the line we should update the selection ranges based on how much we're inserting in the block above with the "calculate new selection ranges" comment. I think the final formula there should be pos + (i * (1 + indent_len + comment_len)) + indent_len + comment_len where comment_len is

let comment_len = continue_comment_token.map(|token| token.len() + 1).unwrap_or_default();

(+ 1 for the extra space added).

the-mikedavis · 2024-10-03T18:29:09Z

helix-term/src/commands.rs

@@ -3886,6 +3914,14 @@ pub mod insert {

 let mut new_text = String::new();

+ let comment_token = {


Similar here

let continue_comment_token = if config.continue_comments { doc.language_config() .and_then(|config| config.comment_tokens.as_ref()) .and_then(|tokens| comment::get_comment_token(text, tokens, current_line)) } else { None };

the-mikedavis · 2024-10-03T18:31:34Z

helix-term/src/commands.rs

@@ -3916,18 +3958,30 @@ pub mod insert {
 .map_or(false, |pair| pair.open == prev && pair.close == curr);

 let local_offs = if on_auto_pair {


We can add an extra block to special case comment continuing here:

let local_offs = if let Some(token) = continue_comment_token { // insert the line ending, then indent then token then ' ' } else if on_auto_pair() { // unchanged } else { // unchanged }

and switch on_auto_pair to be a closure that takes no arguments - that way we avoid computing it if we're continuing a comment

the-mikedavis · 2024-10-03T18:34:40Z

I'm not sure how much extra work it would be (maybe belongs in a follow-up PR) but it would be nice to have a way to not continue a comment. You might want to add an uncommented line after a commented line and with this change you need to go to the next line and use O rather than using o on the commented line. I think it would be handy if the first Enter (or o) continued the comment and then a second Enter removed the comment token and trailing ' '.

pascalkuthe · 2024-10-04T00:10:00Z

I'm not sure how much extra work it would be (maybe belongs in a follow-up PR) but it would be nice to have a way to not continue a comment. You might want to add an uncommented line after a commented line and with this change you need to go to the next line and use O rather than using o on the commented line. I think it would be handy if the first Enter (or o) continued the comment and then a second Enter removed the comment token and trailing ' '.

Maybe a dedicated command/jeybinding (kaklune tends to use alt based bindings like a-o for stuff like this) you work.

This requires either adding state or doing this for every commented line which would be surprising. Also I often have empty lines in doc comments (like writing code inside markdown doc comments) so that would get annoying there

seanchen1991 and others added 24 commits October 20, 2023 10:07

Add continue_comment function

3844eb7

Add test for get_comment_token fn

3d31e66

Fix incorrect assertion

129ecae

Wire up continue comment functionality

2449622

Add comment-tokens field to languages.toml

036e2bf

Add additional comment tokens for Rust

adda266

Match all comment-tokens fields with comment-token fields

be9224e

Refactor continue comment function to also return position of comment…

e77d15d

… token

Update calls to find_first_non_whitespace_char

751d15d

Continue single comments with indentation

4315906

Implement count_whitespace_after fn

0d224e5

Get tests for continue comment logic passing

ee9c015

Don't count newlines when counting whitespace

e44d55a

Rename a variable

d816982

Add continue-comment config parameter

e6a492b

Contiue comments in insert mode based on config

c3a6190

Merge branch 'helix-editor:master' into feature/continue-comment

d25c959

Use pre-existing indentation functions to indent comment lines

cbd05dc

Merge branch 'feature/continue-comment' of https://github.com/seanche…

d73cd87

…n1991/helix into feature/continue-comment

Clean up for PR submission

e170d40

Fix merge conflict in languages.toml

d52dae9

Remove changes in indent.rs

b03f870

Remove incorrect unit test

57261e5

Merge branch 'feature/continue-comment' of github.com:seanchen1991/he…

e51c288

…lix into continue-single-comment

TornaxO7 force-pushed the continue-single-comment branch from 57b449c to 0fccc92 Compare June 20, 2024 01:34

add has_line_comment method and fix starts_with for RopeSlice

8fa3cc9

TornaxO7 force-pushed the continue-single-comment branch from 0fccc92 to 8fa3cc9 Compare June 20, 2024 01:35

make clippy happy

c70acf7

kirawi added the A-core Area: Helix core improvements label Jun 21, 2024

TornaxO7 added 2 commits August 7, 2024 11:42

make clippy happy

dc92500

config: fix tests

1e18f1b

TornaxO7 added 9 commits August 7, 2024 11:55

continue-comments: add some docs

f68cf64

continue-comment: remove unwraps

4cc5148

Merge branch 'master' of github.com:helix-editor/helix into continue-…

830f148

…single-comment

continue-comment: removing indent_heuristic function

da2a444

Revert "continue-comment: removing indent_heuristic function"

1eb963d

This reverts commit da2a444.

Reapply "continue-comment: removing indent_heuristic function"

9c09141

This reverts commit 1eb963d.

Revert "continue-comment: removing indent_heuristic function"

d4b1dfb

This reverts commit da2a444.

continue-comment: set default value for continue comments to false

f53ec12

continue-comment: enable by default and use hybrid indentation

372550d

TornaxO7 force-pushed the continue-single-comment branch from 4a5a54a to 372550d Compare August 9, 2024 21:55

TornaxO7 marked this pull request as ready for review August 9, 2024 22:11

TornaxO7 requested a review from pascalkuthe August 9, 2024 22:12

TornaxO7 marked this pull request as draft August 9, 2024 22:15

TornaxO7 added 2 commits August 10, 2024 09:43

continue-comments: fix indent-logic if continue-comments wants to pla…

83093f8

…ce a comment

continue-comment: improve code readability

b842fdb

TornaxO7 marked this pull request as ready for review August 10, 2024 08:08

TornaxO7 marked this pull request as draft August 10, 2024 08:12

continue-comment: add some comments and remove unused code

b5003e9

TornaxO7 marked this pull request as ready for review August 10, 2024 08:29

the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 11, 2024

djugei mentioned this pull request Aug 24, 2024

feat: continue single-line comments #8664

Closed

the-mikedavis reviewed Oct 3, 2024

View reviewed changes

the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continue single comment #10996

Continue single comment #10996

TornaxO7 commented Jun 20, 2024

TornaxO7 commented Jun 20, 2024 •

edited

Loading

TornaxO7 commented Aug 7, 2024 •

edited

Loading

the-mikedavis Oct 3, 2024

the-mikedavis Oct 3, 2024

the-mikedavis Oct 3, 2024

the-mikedavis Oct 3, 2024

the-mikedavis Oct 3, 2024

the-mikedavis Oct 3, 2024

the-mikedavis Oct 3, 2024

the-mikedavis Oct 3, 2024

the-mikedavis Oct 3, 2024

the-mikedavis Oct 3, 2024

the-mikedavis Oct 3, 2024

the-mikedavis commented Oct 3, 2024

pascalkuthe commented Oct 4, 2024

		@@ -3886,6 +3914,14 @@ pub mod insert {

		let mut new_text = String::new();

		let comment_token = {

		@@ -3916,18 +3958,30 @@ pub mod insert {
		.map_or(false, \|pair\| pair.open == prev && pair.close == curr);

		let local_offs = if on_auto_pair {

Continue single comment #10996

Are you sure you want to change the base?

Continue single comment #10996

Conversation

TornaxO7 commented Jun 20, 2024

TornaxO7 commented Jun 20, 2024 • edited Loading

TornaxO7 commented Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-mikedavis commented Oct 3, 2024

pascalkuthe commented Oct 4, 2024

TornaxO7 commented Jun 20, 2024 •

edited

Loading

TornaxO7 commented Aug 7, 2024 •

edited

Loading