replace DuplicateLine with directional alternatives and adjust cursor positioning

[notwithering]

• replaced DuplicateLine with DuplicateLineUp and DuplicateLineDown
• added Shift-Alt-Ctrl-Up and Shift-Alt-Ctrl-Down keybindings set to use DuplicateLineUp and DuplicateLineDown respectively while keeping original Ctrl-d keybinding
• updated docs in md files
• modifications to DuplicateLine functions
  • DuplicateLineUp: cursor will stay still instead of moving to the end of the line
  • DuplicateLineDown: cursor will move with the duplicated line down at the same X position instead of moving to the end of the line

2025-01-25.17-14-28-00.00.46.643-00.01.02.517.mp4

(works with selections aswell, forgot to show in the video)

[notwithering]

2025-01-26.02-44-23-00.06.37.366-00.07.20.971.mp4

[dmaluka]

• Fixing the cursor position after duplicate is a good improvement, thanks for that.
• Regarding DuplicateLineUp: it seems better to change the behavior of Duplicate to the DuplicateLineUp behavior you've implemented, without renaming it. First because the DuplicateLineUp name doesn't seem quite intuitive (it doesn't really move anything up), second because it's better to avoid breaking compatibility (users are not gonna be happy when their custom bindings for DuplicateLine in their bindings.json stop working).
• Regarding DuplicateLineDown: its implementation doesn't look bad, but I wonder how useful it is to add this new action. In the case when there is no selection, the same behavior could be achieved with DuplicateLine,CursorDown. So I'm curious: do you actually care about the case when there is a selection (and actually want the selection to be not de-selected but moved to the newly created lines below, and thus you want a new dedicated action for that), or perhaps DuplicateLine,CursorDown would work for you as well?

Given the above, the entire PR could be reduced to a change like this?

--- a/internal/action/actions.go
+++ b/internal/action/actions.go
@@ -1438,10 +1438,11 @@ func (h *BufPane) Duplicate() bool {
 // DuplicateLine duplicates the current line. If there is a selection, DuplicateLine
 // duplicates all the lines that are (fully or partially) in the selection.
 func (h *BufPane) DuplicateLine() bool {
+	origLoc := h.Cursor.Loc
+	origLastVisualX := h.Cursor.LastVisualX
+	origLastWrappedVisualX := h.Cursor.LastWrappedVisualX
+
 	if h.Cursor.HasSelection() {
-		origLoc := h.Cursor.Loc
-		origLastVisualX := h.Cursor.LastVisualX
-		origLastWrappedVisualX := h.Cursor.LastWrappedVisualX
 		origSelection := h.Cursor.CurSelection
 
 		start := h.Cursor.CurSelection[0]
@@ -1460,9 +1461,6 @@ func (h *BufPane) DuplicateLine() bool {
 			h.Buf.Insert(h.Cursor.Loc, "\n"+string(h.Buf.LineBytes(y)))
 		}
 
-		h.Cursor.Loc = origLoc
-		h.Cursor.LastVisualX = origLastVisualX
-		h.Cursor.LastWrappedVisualX = origLastWrappedVisualX
 		h.Cursor.CurSelection = origSelection
 
 		if start.Y < end.Y {
@@ -1475,6 +1473,11 @@ func (h *BufPane) DuplicateLine() bool {
 		h.Buf.Insert(h.Cursor.Loc, "\n"+string(h.Buf.LineBytes(h.Cursor.Y)))
 		InfoBar.Message("Duplicated line")
 	}
+
+	h.Cursor.Loc = origLoc
+	h.Cursor.LastVisualX = origLastVisualX
+	h.Cursor.LastWrappedVisualX = origLastWrappedVisualX
+
 	h.Relocate()
 	return true
 }

[notwithering]

@dmaluka

Regarding DuplicateLineUp: it seems better to change the behavior of Duplicate to the DuplicateLineUp behavior you've implemented, without renaming it

with selection, the Duplicate action takes only the selected text and adds it to the buffer and moves the selection to re-select that duplicated text (#3635 (comment)), the DuplicateLine action duplicates entire lines that contain a selection, whether partially or fully selected

---

DuplicateLineUp name doesn't seem quite intuitive (it doesn't really move anything up)

the DuplicateLineUp name is chosen to match other text editors that for the same action

  {
    "context": "Editor",
    "bindings": {
...
      "ctrl-alt-shift-up": "editor::DuplicateLineUp",
      "ctrl-alt-shift-down": "editor::DuplicateLineDown",
...
  }

(zed default-linux.json)

(vscode-docs keybindings.md)

---

it's better to avoid breaking compatibility (users are not gonna be happy when their custom bindings for DuplicateLine in their bindings.json stop working).

i made sure to keep the original binding for DuplicateLine which just calls upon the DuplicateLineDown function in 48eb320, this must've been hard to notice since it was in the giant diff of reformatted white space

---

I wonder how useful it is to add this new action. In the case when there is no selection, the same behavior could be achieved with DuplicateLine,CursorDown.

if you were to bind DuplicateLine,CursorDown it would not work properly, especially with selections

2025-01-29.16-26-25.mp4

---

do you actually care about the case when there is a selection (and actually want the selection to be not de-selected but moved to the newly created lines below, and thus you want a new dedicated action for that), or perhaps DuplicateLine,CursorDown would work for you as well?

keeping the selection is important to align with other editors that handle selections properly. as already established in the above section, DuplicateLine,CursorDown would not work (unless i read something wrong—entirely possible)

---

hope this clears things up!

[dmaluka]

with selection, the Duplicate action takes only the selected text and adds it to the buffer and moves the selection to re-select that duplicated text

Sorry, I meant DuplicateLine, not Duplicate.

i made sure to keep the original binding for DuplicateLine which just calls upon the DuplicateLineDown function in 48eb320, this must've been hard to notice since it was in the giant diff of reformatted white space

Ah, ok. Yes, it was hard to notice, especially since you haven't mentioned it in the PR description or in the commit message but quite the opposite, you said you removed DuplicateLine (it is not obvious that it is not about the action but only about the function).

if you were to bind DuplicateLine,CursorDown it would not work properly, especially with selections

Obviously I didn't mean DuplicateLine in its current form, I meant DuplicateLine with improved behavior: cursor staying still instead of moving to the end of line (i.e. what you've implemented in DuplicateLineUp, or what is implemented in the diff in my last comment). As long as there is no selection, DuplicateLineUp,CursorDown is not different from DuplicateLineDown, right?

keeping the selection is important to align with other editors that handle selections properly

Aligning with other editors is not a goal in itself. Are you actually using DuplicateLineDown (with selection) yourself? If so, I'm curious why do you find it useful (i.e. why do you want not just to duplicate the selected lines but also move the selection).

[notwithering]

Obviously I didn't mean DuplicateLine in its current form, I meant DuplicateLine with improved behavior: cursor staying still instead of moving to the end of line (i.e. what you've implemented in DuplicateLineUp, or what is implemented in the diff in my last comment). As long as there is no selection, DuplicateLineUp,CursorDown is not different from DuplicateLineDown, right?

oh i see now, i apologize for not realizing thats what you meant, this is probably a better option. this would give the user more choice in the specific behavior of it

in addition, we would need a new action for moving the entire selection (or cursor if no selection) since im not aware of any action that does this

is it a big backwards compatability issue to change the functionality of the DuplicateLine action to begin working like DuplicateLineUp? since i'm pretty sure changing it to DuplicateLineDown wasnt much of an issue since they worked almost the same, or would we need a new action name for the new DuplicateLineUp functionality? just curious so i can see what can be changed

Aligning with other editors is not a goal in itself. Are you actually using DuplicateLineDown (with selection) yourself? If so, I'm curious why do you find it useful (i.e. why do you want not just to duplicate the selected lines but also move the selection).

yes i do use this, i need to duplicate a code section multiple times on top of eachother sometimes. it's just something to help with my workflow

[usfbih8u]

Keeping the cursor position is a nice fix. Maybe other parts of this PR might be better suited as a plugin or Lua function rather than being part of the editor itself.

[dmaluka]

is it a big backwards compatability issue to change the functionality of the DuplicateLine action to begin working like DuplicateLineUp?

It changes the behavior of DuplicateLine, yes, but I doubt there are users that would find this change undesirable, rather quite the opposite.

So I'm still inclined to think we should just change the existing DuplicateLine action, without introducing new DuplicateLineUp which does the same. And it still seems to me that DuplicateLineUp is not a very intuitive name for it. Nothing really moves up: the cursor stays still, and the buffer view also stays still (doesn't scroll).

yes i do use this, i need to duplicate a code section multiple times on top of eachother sometimes. it's just something to help with my workflow

Ok, we can probably add DuplicateLineDown... Or maybe we don't need to: it seems a rather niche use case, and it indeed can be easily implemented in Lua, e.g.:

function duplicateLineDown(bp)
    bp:DuplicateLine()

    if bp.Cursor:HasSelection() then
        local lines = bp.Cursor.CurSelection[2].Y - bp.Cursor.CurSelection[1].Y
        if bp.Cursor.CurSelection[2].X > 0 then
            lines = lines + 1
        end
        bp.Cursor.Loc.Y = bp.Cursor.Loc.Y + lines
        bp.Cursor.CurSelection[1].Y = bp.Cursor.CurSelection[1].Y + lines
        bp.Cursor.CurSelection[2].Y = bp.Cursor.CurSelection[2].Y + lines
        bp.Cursor.OrigSelection[1].Y = bp.Cursor.OrigSelection[1].Y + lines
        bp.Cursor.OrigSelection[2].Y = bp.Cursor.OrigSelection[2].Y + lines
    else
        bp.Cursor.Loc.Y = bp.Cursor.Loc.Y + 1
    end
end

[dmaluka]

BTW I've noticed a couple of issues with this PR (irregardless of the above discussions):

1. If we DuplicateLineDown with selection and then press Shift-Right or Shift-Left (to increase or reduce the selection), the resulting behavior is completely broken. I think to fix it you need to update not just h.Cursor.CurSelection but also h.Cursor.OrigSelection,
2. If we DuplicateLineUp or DuplicateLineDown and then undo, the cursor is unexpectedly put at the end of the current line. This incorrect undo behavior is actually present with DuplicateLine without this PR as well, but with this PR (namely with the fix for the cursor position after duplicate) this wrong behavior after undo becomes even more noticeable.

[notwithering]

thanks for all the suggestions, I'll get to fixing this PR in around 3 or 4 hours