Skip to content

Improve and unify CopyLine, CutLine, DeleteLine, DuplicateLine actions #3335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dmaluka merged 17 commits into from
Sep 16, 2024
+152 −67
Merged

Improve and unify CopyLine, CutLine, DeleteLine, DuplicateLine actions #3335

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
19c69f9
Fix Cursor{Up,Down} after DeleteLine and CutLine
dmaluka Jun 8, 2024
df8d528
Fix Cursor{Up,Down} after CopyLine
dmaluka Jun 8, 2024
8bc6756
Fix CopyLine at the last empty line of buffer
dmaluka Jun 8, 2024
52ed431
Make lastCutTime actually work
dmaluka Jun 9, 2024
2860efb
CutLine: remove unneeded if check
dmaluka Jun 9, 2024
830768b
Reorganize Copy and CopyLine actions
dmaluka Jun 8, 2024
a317aef
Reorganize Cut and CutLine actions
dmaluka Jun 9, 2024
c1bbd7b
CutLine: cosmetic refactoring
dmaluka Jun 9, 2024
9f7bdb1
Cosmetic change: move Cut above CutLine
dmaluka Jun 9, 2024
fdacb28
CopyLine, CutLine, DeleteLine: respect selection
dmaluka Jun 9, 2024
e6825f0
CutLine: make infobar message more useful
dmaluka Jun 9, 2024
04143c7
Make Cut, Copy, CopyLine don't mess with CutLine's multi line cuts
dmaluka Jun 9, 2024
33a1bb1
CutLine: return if cliboard read failed
dmaluka Jun 9, 2024
25f71ee
DuplicateLine: move selection duplication to separate Duplicate action
dmaluka Jun 9, 2024
6f724bc
DuplicateLine: respect selections
dmaluka Jun 8, 2024
68d6f43
CutLine: remove lastCutTime feature
dmaluka Jun 12, 2024
bf65847
help/keybindings: document CutLine behavior
dmaluka Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 126 additions & 47 deletions internal/action/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1162,101 +1162,180 @@ func (h *BufPane) Redo() bool {
return true
}

func (h *BufPane) selectLines() int {
if h.Cursor.HasSelection() {
start := h.Cursor.CurSelection[0]
end := h.Cursor.CurSelection[1]
if start.GreaterThan(end) {
start, end = end, start
}
if end.X == 0 {
end = end.Move(-1, h.Buf)
}

h.Cursor.Deselect(true)
h.Cursor.SetSelectionStart(buffer.Loc{0, start.Y})
h.Cursor.SetSelectionEnd(buffer.Loc{0, end.Y + 1})
} else {
h.Cursor.SelectLine()
}
return h.Cursor.CurSelection[1].Y - h.Cursor.CurSelection[0].Y
}

// Copy the selection to the system clipboard
func (h *BufPane) Copy() bool {
if h.Cursor.HasSelection() {
h.Cursor.CopySelection(clipboard.ClipboardReg)
h.freshClip = true
InfoBar.Message("Copied selection")
if !h.Cursor.HasSelection() {
return false
}
h.Cursor.CopySelection(clipboard.ClipboardReg)
h.freshClip = false
InfoBar.Message("Copied selection")
h.Relocate()
return true
}

// CopyLine copies the current line to the clipboard
// CopyLine copies the current line to the clipboard. If there is a selection,
// CopyLine copies all the lines that are (fully or partially) in the selection.
func (h *BufPane) CopyLine() bool {
if h.Cursor.HasSelection() {
origLoc := h.Cursor.Loc
origLastVisualX := h.Cursor.LastVisualX
origSelection := h.Cursor.CurSelection

nlines := h.selectLines()
if nlines == 0 {
return false
}
origLoc := h.Cursor.Loc
h.Cursor.SelectLine()
h.Cursor.CopySelection(clipboard.ClipboardReg)
h.freshClip = true
InfoBar.Message("Copied line")
h.freshClip = false
if nlines > 1 {
InfoBar.Message(fmt.Sprintf("Copied %d lines", nlines))
} else {
InfoBar.Message("Copied line")
}

h.Cursor.Deselect(true)
h.Cursor.Loc = origLoc
h.Cursor.LastVisualX = origLastVisualX
h.Cursor.CurSelection = origSelection
h.Relocate()
return true
}

// CutLine cuts the current line to the clipboard
func (h *BufPane) CutLine() bool {
h.Cursor.SelectLine()
// Cut the selection to the system clipboard
func (h *BufPane) Cut() bool {
if !h.Cursor.HasSelection() {
return false
}
if h.freshClip {
if h.Cursor.HasSelection() {
if clip, err := clipboard.Read(clipboard.ClipboardReg); err != nil {
InfoBar.Error(err)
} else {
clipboard.WriteMulti(clip+string(h.Cursor.GetSelection()), clipboard.ClipboardReg, h.Cursor.Num, h.Buf.NumCursors())
}
h.Cursor.CopySelection(clipboard.ClipboardReg)
h.Cursor.DeleteSelection()
h.Cursor.ResetSelection()
h.freshClip = false
InfoBar.Message("Cut selection")

h.Relocate()
return true
}

// CutLine cuts the current line to the clipboard. If there is a selection,
// CutLine cuts all the lines that are (fully or partially) in the selection.
func (h *BufPane) CutLine() bool {
nlines := h.selectLines()
if nlines == 0 {
return false
}
totalLines := nlines
if h.freshClip && time.Since(h.lastCutTime) < 10*time.Second {
Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Minor thing,
Suggested change
if h.freshClip && time.Since(h.lastCutTime) < 10*time.Second {
if h.freshClip && time.Since(h.lastCutTime) < 10 * time.Second {
  1. I know this is a behavior from before but I never knew it was there. I think we should either (or both)
    a. Mention this somewhere in the documentation
    b. Maybe have an option to toggle this on or off?

Copy link
Collaborator Author

@dmaluka dmaluka Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding spaces, it is the gofmt style.

2. I know this is a behavior from before but I never knew it was there.

Neither did I. And I agree with you.

If we are talking about the append-to-clipboard feature as such, I agree we should document and/or make it optional, and I'm inclined to think that we should do both. But it seems better (more flexible) not to add an option but to add a separate action. Rename the current CutLine implementation to CutLineAppend or CutLineSmart (both names suck, but I can't think of anything better at the moment), add a new CutLine implementation providing the "classical" line cut functionality without this fancy behavior, and update the default Ctrl-x and Ctrl-k bindings accordingly, to not change their default behavior.

And if we are talking specifically about this lastCutTime sub-feature (resetting the append-to-clipboard after 10 minutes), AFAICS it was buggy and never worked from the beginning. (Someone though it was a good idea to implement it, but didn't bother to test it, lol.) And I'm not sure it's a useful feature. So, since it has never really worked anyway, I'm inclined to think we should just remove it.

Copy link
Collaborator Author

@dmaluka dmaluka Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR that originally added it: #64 + #70.

Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding spaces, it is the gofmt style.

It's weird how some places in the same file we have spaces but not this one. I don't code much in golang and it is a very minor thing so I don't mind, just pointing it out.

I'm inclined to think that we should do both. But it seems better (more flexible) not to add an option but to add a separate action. Rename the current CutLine implementation to CutLineAppend

Yeah, this seems good to me. CutLineAppend is fine I think.

So, since it has never really worked anyway, I'm inclined to think we should just remove it.

Yeah, it was never working so we can remove it.

Copy link
Collaborator Author

@dmaluka dmaluka Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird how some places in the same file we have spaces but not this one.

Apparently there is some logic in that. Like, use spaces in standalone statements but don't use them in function arguments (but only if there is more than one function argument), etc.

I'd prefer to always use spaces, but our rule is: just follow whatever style is expected by gofmt, instead of wasting time on discussions which style is better.

CutLineAppend is fine I think.

Or ClipLine, maybe?

Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ClipLine doesn't explictly describe what it is doing. If we have the same thing for copy line (where it appends to the clipboard), the word ClipLine is confusing.

I still think CutLineAppend (Or some variation of it if you don't like it) describes what it is doing best, and we can even have the same naming style for CopyLine (that appends to clipboard) as well if we want to

Copy link
Collaborator Author

@dmaluka dmaluka Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I suggested ClipLine because it sounded cool and succinct to me, but frankly I'm not really familiar with the meaning of the word "clip" (I'm not a native speaker). Doesn't it actually have two different meanings: one like "append" but the other one like "cut"?

Anyway... now I'm thinking: maybe let's not complicate things and leave the current CutLine behavior as is (just document it somewhere; and remove the 10 seconds part). If the user wants the simple behavior instead, the user can just use CopyLine,DeleteLine, right?

Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it actually have two different meanings: one like "append" but the other one like "cut"?

It means "cut" to me. I can see how it can mean "append" but wouldn't click for me unless I used it.

leave the current CutLine behavior as is

Yeah, that's fine with me as well (to be honest I don't use cutline that much 😅)

Ideally, the ability to switch off this behavior would be great but I think this behavior works fine for most scenarios. (We can add it if people really want to switch it off)

Copy link
Collaborator

@JoeKar JoeKar Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you selected the line without selecting the newline character at the end of it?

Sure it was. It was just my expectation which didn't fit. My fault...

Anyway... now I'm thinking: maybe let's not complicate things and leave the current CutLine behavior as is (just document it somewhere; and remove the 10 seconds part). If the user wants the simple behavior instead, the user can just use CopyLine,DeleteLine, right?

Yep, I'm fine with that too, since I didn't used it so far.

Copy link
Collaborator Author

@dmaluka dmaluka Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the lastCutTime feature and added documentation for the CutLine behavior.

if clip, err := clipboard.Read(clipboard.ClipboardReg); err != nil {
InfoBar.Error(err)
return false
} else {
clipboard.WriteMulti(clip+string(h.Cursor.GetSelection()), clipboard.ClipboardReg, h.Cursor.Num, h.Buf.NumCursors())
totalLines = strings.Count(clip, "\n") + nlines
}
} else if time.Since(h.lastCutTime)/time.Second > 10*time.Second || !h.freshClip {
h.Copy()
} else {
h.Cursor.CopySelection(clipboard.ClipboardReg)
}
h.freshClip = true
h.lastCutTime = time.Now()
h.Cursor.DeleteSelection()
h.Cursor.ResetSelection()
InfoBar.Message("Cut line")
h.Cursor.StoreVisualX()
if totalLines > 1 {
InfoBar.Message(fmt.Sprintf("Cut %d lines", totalLines))
} else {
InfoBar.Message("Cut line")
}
h.Relocate()
return true
}

// Cut the selection to the system clipboard
func (h *BufPane) Cut() bool {
if h.Cursor.HasSelection() {
h.Cursor.CopySelection(clipboard.ClipboardReg)
h.Cursor.DeleteSelection()
h.Cursor.ResetSelection()
h.freshClip = true
InfoBar.Message("Cut selection")

h.Relocate()
return true
// Duplicate the selection
func (h *BufPane) Duplicate() bool {
if !h.Cursor.HasSelection() {
return false
}
return h.CutLine()
h.Buf.Insert(h.Cursor.CurSelection[1], string(h.Cursor.GetSelection()))
InfoBar.Message("Duplicated selection")
h.Relocate()
return true
}

// DuplicateLine duplicates the current line or selection
// 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 {
var infoMessage = "Duplicated line"
if h.Cursor.HasSelection() {
infoMessage = "Duplicated selection"
h.Buf.Insert(h.Cursor.CurSelection[1], string(h.Cursor.GetSelection()))
origLoc := h.Cursor.Loc
origLastVisualX := h.Cursor.LastVisualX
origSelection := h.Cursor.CurSelection

start := h.Cursor.CurSelection[0]
end := h.Cursor.CurSelection[1]
if start.GreaterThan(end) {
start, end = end, start
}
if end.X == 0 {
end = end.Move(-1, h.Buf)
}

h.Cursor.Deselect(true)
h.Cursor.Loc = end
h.Cursor.End()
for y := start.Y; y <= end.Y; y++ {
h.Buf.Insert(h.Cursor.Loc, "\n"+string(h.Buf.LineBytes(y)))
}

h.Cursor.Loc = origLoc
h.Cursor.LastVisualX = origLastVisualX
h.Cursor.CurSelection = origSelection

if start.Y < end.Y {
InfoBar.Message(fmt.Sprintf("Duplicated %d lines", end.Y-start.Y+1))
} else {
InfoBar.Message("Duplicated line")
}
} else {
h.Cursor.End()
h.Buf.Insert(h.Cursor.Loc, "\n"+string(h.Buf.LineBytes(h.Cursor.Y)))
// h.Cursor.Right()
InfoBar.Message("Duplicated line")
}

InfoBar.Message(infoMessage)
h.Relocate()
return true
}

// DeleteLine deletes the current line
// DeleteLine deletes the current line. If there is a selection, DeleteLine
// deletes all the lines that are (fully or partially) in the selection.
func (h *BufPane) DeleteLine() bool {
h.Cursor.SelectLine()
if !h.Cursor.HasSelection() {
nlines := h.selectLines()
if nlines == 0 {
return false
}
h.Cursor.DeleteSelection()
h.Cursor.ResetSelection()
InfoBar.Message("Deleted line")
h.Cursor.StoreVisualX()
if nlines > 1 {
InfoBar.Message(fmt.Sprintf("Deleted %d lines", nlines))
} else {
InfoBar.Message("Deleted line")
}
h.Relocate()
return true
}
Expand Down
5 changes: 4 additions & 1 deletion internal/action/bufpane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,8 @@ type BufPane struct {
// It is used for clearing the clipboard to replace it with fresh cut lines.
lastCutTime time.Time

// freshClip returns true if the clipboard has never been pasted.
// freshClip returns true if one or more lines have been cut to the clipboard
// and have never been pasted yet.
freshClip bool

// Was the last mouse event actually a double click?
Expand Down Expand Up @@ -783,6 +784,7 @@ var BufKeyActions = map[string]BufKeyAction{
"CopyLine": (*BufPane).CopyLine,
"Cut": (*BufPane).Cut,
"CutLine": (*BufPane).CutLine,
"Duplicate": (*BufPane).Duplicate,
"DuplicateLine": (*BufPane).DuplicateLine,
"DeleteLine": (*BufPane).DeleteLine,
"MoveLinesUp": (*BufPane).MoveLinesUp,
Expand Down Expand Up @@ -909,6 +911,7 @@ var MultiActions = map[string]bool{
"Copy": true,
"Cut": true,
"CutLine": true,
"Duplicate": true,
"DuplicateLine": true,
"DeleteLine": true,
"MoveLinesUp": true,
Expand Down
10 changes: 5 additions & 5 deletions internal/action/defaults_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ var bufdefaults = map[string]string{
"Alt-]": "DiffNext|CursorEnd",
"Ctrl-z": "Undo",
"Ctrl-y": "Redo",
"Ctrl-c": "CopyLine|Copy",
"Ctrl-x": "Cut",
"Ctrl-c": "Copy|CopyLine",
"Ctrl-x": "Cut|CutLine",
"Ctrl-k": "CutLine",
"Ctrl-d": "DuplicateLine",
"Ctrl-d": "Duplicate|DuplicateLine",
"Ctrl-v": "Paste",
"Ctrl-a": "SelectAll",
"Ctrl-t": "AddTab",
Expand Down Expand Up @@ -144,8 +144,8 @@ var infodefaults = map[string]string{
"Backtab": "CycleAutocompleteBack",
"Ctrl-z": "Undo",
"Ctrl-y": "Redo",
"Ctrl-c": "CopyLine|Copy",
"Ctrl-x": "Cut",
"Ctrl-c": "Copy|CopyLine",
"Ctrl-x": "Cut|CutLine",
"Ctrl-k": "CutLine",
"Ctrl-v": "Paste",
"Home": "StartOfTextToggle",
Expand Down
10 changes: 5 additions & 5 deletions internal/action/defaults_other.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ var bufdefaults = map[string]string{
"Alt-]": "DiffNext|CursorEnd",
"Ctrl-z": "Undo",
"Ctrl-y": "Redo",
"Ctrl-c": "CopyLine|Copy",
"Ctrl-x": "Cut",
"Ctrl-c": "Copy|CopyLine",
"Ctrl-x": "Cut|CutLine",
"Ctrl-k": "CutLine",
"Ctrl-d": "DuplicateLine",
"Ctrl-d": "Duplicate|DuplicateLine",
"Ctrl-v": "Paste",
"Ctrl-a": "SelectAll",
"Ctrl-t": "AddTab",
Expand Down Expand Up @@ -147,8 +147,8 @@ var infodefaults = map[string]string{
"Backtab": "CycleAutocompleteBack",
"Ctrl-z": "Undo",
"Ctrl-y": "Redo",
"Ctrl-c": "CopyLine|Copy",
"Ctrl-x": "Cut",
"Ctrl-c": "Copy|CopyLine",
"Ctrl-x": "Cut|CutLine",
"Ctrl-k": "CutLine",
"Ctrl-v": "Paste",
"Home": "StartOfTextToggle",
Expand Down
10 changes: 5 additions & 5 deletions runtime/help/keybindings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,10 +491,10 @@ conventions for text editing defaults.
"Alt-]": "DiffNext|CursorEnd",
"Ctrl-z": "Undo",
"Ctrl-y": "Redo",
"Ctrl-c": "CopyLine|Copy",
"Ctrl-x": "Cut",
"Ctrl-c": "Copy|CopyLine",
"Ctrl-x": "Cut|CutLine",
"Ctrl-k": "CutLine",
"Ctrl-d": "DuplicateLine",
"Ctrl-d": "Duplicate|DuplicateLine",
"Ctrl-v": "Paste",
"Ctrl-a": "SelectAll",
"Ctrl-t": "AddTab",
Expand Down Expand Up @@ -615,8 +615,8 @@ are given below:
"Backtab": "CycleAutocompleteBack",
"Ctrl-z": "Undo",
"Ctrl-y": "Redo",
"Ctrl-c": "CopyLine|Copy",
"Ctrl-x": "Cut",
"Ctrl-c": "Copy|CopyLine",
"Ctrl-x": "Cut|CutLine",
"Ctrl-k": "CutLine",
"Ctrl-v": "Paste",
"Home": "StartOfTextToggle",
Expand Down