Determine whether to use a margin of 0 or 1 when uncommenting (#476)

* Implement `margin` calculation for uncommenting

* Move `margin` calculation to `find_line_comment`

* Fix comment bug with multiple selections on a line

* Fix `find_line_comment` test for new return type

* Generate a single vec of lines for comment toggle

`toggle_line_comments` collects the lines covered by all selections into
a `Vec`, skipping duplicates. `find_line_comment` now returns the lines
to operate on, instead of returning the lines to skip.

* Fix test for `find_line_comment`

* Reserve length of `to_change` instead of `lines`

The length of `lines` includes blank lines which will be skipped, and as
such do not need space for a change reserved for them. `to_change`
includes only the lines which will be changed.

* Use `token.chars().count()` for token char length

* Create `changes` with capacity instead of reserving

* Remove unnecessary clones in `test_find_line_comment`

* Add test case for 0 margin comments

* Add comments explaining `find_line_comment`

helix-core/src/comment.rs

@@ -1,17 +1,27 @@
 use crate::{
     find_first_non_whitespace_char, Change, Rope, RopeSlice, Selection, Tendril, Transaction,
 };
-use core::ops::Range;
 use std::borrow::Cow;
 
+/// Given text, a comment token, and a set of line indices, returns the following:
+/// - Whether the given lines should be considered commented
+///     - If any of the lines are uncommented, all lines are considered as such.
+/// - The lines to change for toggling comments
+///     - This is all provided lines excluding blanks lines.
+/// - The column of the comment tokens
+///     - Column of existing tokens, if the lines are commented; column to place tokens at otherwise.
+/// - The margin to the right of the comment tokens
+///     - Defaults to `1`. If any existing comment token is not followed by a space, changes to `0`.
 fn find_line_comment(
     token: &str,
     text: RopeSlice,
-    lines: Range<usize>,
+    lines: impl IntoIterator<Item = usize>,
-) -> (bool, Vec<usize>, usize) {
+) -> (bool, Vec<usize>, usize, usize) {
     let mut commented = true;
-    let mut skipped = Vec::new();
+    let mut to_change = Vec::new();
     let mut min = usize::MAX; // minimum col for find_first_non_whitespace_char
+    let mut margin = 1;
+    let token_len = token.chars().count();
     for line in lines {
         let line_slice = text.line(line);
         if let Some(pos) = find_first_non_whitespace_char(line_slice) {
@@ -29,47 +39,53 @@ fn find_line_comment(
                 // considered uncommented.
                 commented = false;
             }
-        } else {
+
-            // blank line
+            // determine margin of 0 or 1 for uncommenting; if any comment token is not followed by a space,
-            skipped.push(line);
+            // a margin of 0 is used for all lines.
+            if matches!(line_slice.get_char(pos + token_len), Some(c) if c != ' ') {
+                margin = 0;
+            }
+
+            // blank lines don't get pushed.
+            to_change.push(line);
         }
     }
-    (commented, skipped, min)
+    (commented, to_change, min, margin)
 }
 
 #[must_use]
 pub fn toggle_line_comments(doc: &Rope, selection: &Selection, token: Option<&str>) -> Transaction {
     let text = doc.slice(..);
-    let mut changes: Vec<Change> = Vec::new();
 
     let token = token.unwrap_or("//");
     let comment = Tendril::from(format!("{} ", token));
 
+    let mut lines: Vec<usize> = Vec::new();
+
+    let mut min_next_line = 0;
     for selection in selection {
-        let start = text.char_to_line(selection.from());
+        let start = text.char_to_line(selection.from()).max(min_next_line);
-        let end = text.char_to_line(selection.to());
+        let end = text.char_to_line(selection.to()) + 1;
-        let lines = start..end + 1;
+        lines.extend(start..end);
-        let (commented, skipped, min) = find_line_comment(&token, text, lines.clone());
+        min_next_line = end + 1;
+    }
 
-        changes.reserve((end - start).saturating_sub(skipped.len()));
+    let (commented, to_change, min, margin) = find_line_comment(&token, text, lines);
 
-        for line in lines {
+    let mut changes: Vec<Change> = Vec::with_capacity(to_change.len());
-            if skipped.contains(&line) {
-                continue;
-            }
 
-            let pos = text.line_to_char(line) + min;
+    for line in to_change {
+        let pos = text.line_to_char(line) + min;
 
-            if !commented {
+        if !commented {
-                // comment line
+            // comment line
-                changes.push((pos, pos, Some(comment.clone())))
+            changes.push((pos, pos, Some(comment.clone())));
-            } else {
+        } else {
-                // uncomment line
+            // uncomment line
-                let margin = 1; // TODO: margin is hardcoded 1 but could easily be 0
+            changes.push((pos, pos + token.len() + margin, None));
-                changes.push((pos, pos + token.len() + margin, None))
-            }
         }
     }
+
     Transaction::change(doc, changes.into_iter())
 }
 
@@ -91,23 +107,32 @@ mod test {
         let text = state.doc.slice(..);
 
         let res = find_line_comment("//", text, 0..3);
-        // (commented = true, skipped = [line 1], min = col 2)
+        // (commented = true, to_change = [line 0, line 2], min = col 2, margin = 1)
-        assert_eq!(res, (false, vec![1], 2));
+        assert_eq!(res, (false, vec![0, 2], 2, 1));
 
         // comment
         let transaction = toggle_line_comments(&state.doc, &state.selection, None);
         transaction.apply(&mut state.doc);
-        state.selection = state.selection.clone().map(transaction.changes());
+        state.selection = state.selection.map(transaction.changes());
 
         assert_eq!(state.doc, "  // 1\n\n  // 2\n  // 3");
 
         // uncomment
         let transaction = toggle_line_comments(&state.doc, &state.selection, None);
         transaction.apply(&mut state.doc);
-        state.selection = state.selection.clone().map(transaction.changes());
+        state.selection = state.selection.map(transaction.changes());
+        assert_eq!(state.doc, "  1\n\n  2\n  3");
+
+        // 0 margin comments
+        state.doc = Rope::from("  //1\n\n  //2\n  //3");
+        // reset the selection.
+        state.selection = Selection::single(0, state.doc.len_chars() - 1);
+
+        let transaction = toggle_line_comments(&state.doc, &state.selection, None);
+        transaction.apply(&mut state.doc);
+        state.selection = state.selection.map(transaction.changes());
         assert_eq!(state.doc, "  1\n\n  2\n  3");
 
-        // TODO: account for no margin after comment
         // TODO: account for uncommenting with uneven comment indentation
     }
 }