fix: 2 bugs in the `Alt-)` and `Alt-(` commands

helix-term/src/commands.rs

@@ -5311,46 +5311,71 @@ fn rotate_selections_last(cx: &mut Context) {
     doc.set_selection(view.id, selection);
 }
 
+#[derive(Debug)]
 enum ReorderStrategy {
     RotateForward,
     RotateBackward,
     Reverse,
 }
 
+/// Like `usize::wrapping_sub`, but provide a custom range from `0` to `range_length`
+fn wrapping_sub_in_range(index: usize, range_length: usize, delta: usize) -> usize {
+    (index + range_length - delta) % range_length
+}
+
+/// Like `usize::wrapping_add`, but provide a custom range from `0` to `range_length`
+fn wrapping_add_in_range(index: usize, range_length: usize, delta: usize) -> usize {
+    (index + range_length + delta) % range_length
+}
+
 fn reorder_selection_contents(cx: &mut Context, strategy: ReorderStrategy) {
     let count = cx.count;
     let (view, doc) = current!(cx.editor);
     let text = doc.text().slice(..);
 
     let selection = doc.selection(view.id);
-    let mut fragments: Vec<_> = selection
+
+    let mut ranges: Vec<_> = selection
         .slices(text)
         .map(|fragment| fragment.chunks().collect())
         .collect();
 
-    let group = count
-        .map(|count| count.get())
-        .unwrap_or(fragments.len()) // default to rotating everything as one group
-        .min(fragments.len());
+    let rotate_by = count.map_or(1, |count| count.get().min(ranges.len()));
 
-    for chunk in fragments.chunks_mut(group) {
-        // TODO: also modify main index
-        match strategy {
-            ReorderStrategy::RotateForward => chunk.rotate_right(1),
-            ReorderStrategy::RotateBackward => chunk.rotate_left(1),
-            ReorderStrategy::Reverse => chunk.reverse(),
-        };
+    let primary_index = match strategy {
+        ReorderStrategy::RotateForward => {
+            ranges.rotate_right(rotate_by);
+            wrapping_add_in_range(selection.primary_index(), ranges.len(), rotate_by)
         }
+        ReorderStrategy::RotateBackward => {
+            ranges.rotate_left(rotate_by);
+            wrapping_sub_in_range(selection.primary_index(), ranges.len(), rotate_by)
+        }
+        ReorderStrategy::Reverse => {
+            if rotate_by % 2 == 0 {
+                // nothing changed, if we reverse something an even
+                // amount of times, the output will be the same
+                return;
+            }
+            ranges.reverse();
+            // -1 to turn 1-based len into 0-based index
+            (ranges.len() - 1) - selection.primary_index()
+        }
+    };
 
     let transaction = Transaction::change(
         doc.text(),
         selection
             .ranges()
             .iter()
-            .zip(fragments)
+            .zip(ranges)
             .map(|(range, fragment)| (range.from(), range.to(), Some(fragment))),
     );
 
+    doc.set_selection(
+        view.id,
+        Selection::new(selection.ranges().into(), primary_index),
+    );
     doc.apply(&transaction, view.id);
 }
 

helix-term/tests/test/commands.rs

@@ -4,6 +4,8 @@ use super::*;
 
 mod insert;
 mod movement;
+mod reverse_selection_contents;
+mod rotate_selection_contents;
 mod write;
 
 #[tokio::test(flavor = "multi_thread")]

helix-term/tests/test/commands/reverse_selection_contents.rs

@@ -0,0 +1,49 @@
+use super::*;
+
+const A: &str = indoc! {"
+    #(a|)#
+    #(b|)#
+    #(c|)#
+    #[d|]#
+    #(e|)#"
+};
+const A_REV: &str = indoc! {"
+    #(e|)#
+    #[d|]#
+    #(c|)#
+    #(b|)#
+    #(a|)#"
+};
+const B: &str = indoc! {"
+    #(a|)#
+    #(b|)#
+    #[c|]#
+    #(d|)#
+    #(e|)#"
+};
+const B_REV: &str = indoc! {"
+    #(e|)#
+    #(d|)#
+    #[c|]#
+    #(b|)#
+    #(a|)#"
+};
+
+const CMD: &str = "<space>?reverse_selection_contents<ret>";
+
+#[tokio::test(flavor = "multi_thread")]
+async fn reverse_selection_contents() -> anyhow::Result<()> {
+    test((A, CMD, A_REV)).await?;
+    test((B, CMD, B_REV)).await?;
+
+    Ok(())
+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn reverse_selection_contents_with_count() -> anyhow::Result<()> {
+    test((B, format!("2{CMD}"), B)).await?;
+    test((B, format!("3{CMD}"), B_REV)).await?;
+    test((B, format!("4{CMD}"), B)).await?;
+
+    Ok(())
+}

helix-term/tests/test/commands/rotate_selection_contents.rs

@@ -0,0 +1,71 @@
+use super::*;
+
+// Progression: A -> B -> C -> D
+//              as we press `A-)`
+const A: &str = indoc! {"
+    #(a|)#
+    #(b|)#
+    #(c|)#
+    #[d|]#
+    #(e|)#"
+};
+
+const B: &str = indoc! {"
+    #(e|)#
+    #(a|)#
+    #(b|)#
+    #(c|)#
+    #[d|]#"
+};
+
+const C: &str = indoc! {"
+    #[d|]#
+    #(e|)#
+    #(a|)#
+    #(b|)#
+    #(c|)#"
+};
+
+const D: &str = indoc! {"
+    #(c|)#
+    #[d|]#
+    #(e|)#
+    #(a|)#
+    #(b|)#"
+};
+
+#[tokio::test(flavor = "multi_thread")]
+async fn rotate_selection_contents_forward_repeated() -> anyhow::Result<()> {
+    test((A, "<A-)>", B)).await?;
+    test((B, "<A-)>", C)).await?;
+    test((C, "<A-)>", D)).await?;
+
+    Ok(())
+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn rotate_selection_contents_forward_with_count() -> anyhow::Result<()> {
+    test((A, "2<A-)>", C)).await?;
+    test((A, "3<A-)>", D)).await?;
+    test((B, "2<A-)>", D)).await?;
+
+    Ok(())
+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn rotate_selection_contents_backward_repeated() -> anyhow::Result<()> {
+    test((D, "<A-(>", C)).await?;
+    test((C, "<A-(>", B)).await?;
+    test((B, "<A-(>", A)).await?;
+
+    Ok(())
+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn rotate_selection_contents_backward_with_count() -> anyhow::Result<()> {
+    test((D, "2<A-(>", B)).await?;
+    test((D, "3<A-(>", A)).await?;
+    test((C, "2<A-(>", A)).await?;
+
+    Ok(())
+}