actions: CutLineAppend added (#3477) #3583
Conversation
It was confusing for people that CutLine will occasionally accumulate cut lines if executed multiple times before the clipboard's content is pasted. Shortcut Ctrl-k is reserved for CutLineAppend, since CutLine already has Ctrl-x binded as well.
| @@ -513,7 +513,7 @@ conventions for text editing defaults. | |||
| "Ctrl-y": "Redo", | |||
| "Ctrl-c": "Copy|CopyLine", | |||
| "Ctrl-x": "Cut|CutLine", | |||
| "Ctrl-k": "CutLine", | |||
| "Ctrl-k": "CutLineAppend", | |||
There was a problem hiding this comment.
Please also add CutLineAppend to the list of all actions, after line 219 in keybindings.md, and remove this outdated paragraph at line 289: "The CutLine action cuts the current line and adds it [...]"
| @@ -1394,11 +1394,37 @@ func (h *BufPane) Cut() bool { | |||
|
|
|||
| // 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. | |||
| // CutLine will replace the clipboard's current buffer. | |||
There was a problem hiding this comment.
Make it clearer: "Unlike CutLineAppend, CutLine replaces the clipboard's current contents." ?
| func (h *BufPane) CutLine() bool { | ||
| totalLines := h.selectLines() |
There was a problem hiding this comment.
I think it's better to name it nlines, for consistency with CopyLine() and DeleteLine().
There was a problem hiding this comment.
I thought about that. But when I first encountered the code, nLines name was quite confusing. It could mean newLines, nextLines, numberLines, numberOfLines etc. Only after looking at the context of the code I could understand the meaning of the nLines name. Which is a bad sign. I actually wanted to name the variable totalLinesSelected.
I would rather suggest to rename nLines to something else for the sake of clarity =)
There was a problem hiding this comment.
n conventionally means "number of something". Names like totalLinesSelected are IMO excessively verbose (as you may notice, we don't tend to give local variables such verbose names without good reason in micro's code, or in Go code in general; it is generally considered that such verbosity does not improve readability but quite the opposite), and furthermore, it is rather misleadingly overemphasizing the fact that those lines are selected. nlines means the number of lines that we need to cut (copy, delete, ...), the fact that we implement that by selecting those lines first and cutting the selection is accidental, not essential.
| } | ||
|
|
||
| h.Cursor.CopySelection(clipboard.ClipboardReg) | ||
| h.freshClip = true |
There was a problem hiding this comment.
This means that CutLineAppend will append not only to the lines previously cut via CutLineAppend, but also to the (single) line previously cut via CutLine, if there was one. Maybe that's what we actually want, but maybe we don't, i.e. we want to reset h.freshClip here to false, not true (consistently with Cut, CopyLine etc). I'm not sure which option is better.
There was a problem hiding this comment.
I'm not sure as well. But it seems to me that any command that appends data to a buffer should stick to "append-only" policy and not erase the buffer on the first call. Also it will keep the logic simpler.
|
|
||
| // CutLineAppend cuts the current line to the clipboard. If there is a selection, | ||
| // CutLineAppend cuts all the lines that are (fully or partially) in the selection. | ||
| // CutLineAppend will append the selection to the clipboard's buffer. |
There was a problem hiding this comment.
I think it's better to clarify it already in the first sentence, e.g.: "CutLineAppend cuts the current line to the clipboard, appending it to the previously cut lines in the clipboard since the last paste (in contrast to CutLine which always rewrites the clipboard)."
| if totalLines > 1 { | ||
| InfoBar.Message(fmt.Sprintf("Cut %d lines", totalLines)) | ||
| if nlines > 1 { | ||
| InfoBar.Message(fmt.Sprintf("Cut %d lines. Total lines in buffer: %d.", nlines, totalLines)) |
There was a problem hiding this comment.
The term "buffer" is commonly used in micro with a very different meaning: it means the contents of the file that the user is editing. So this message may sound confusing to the user: it sounds like this is the total number of lines in the entire file.
So, make it clear and unambiguous: "Total lines in clipboard: " ?
Also IMHO it's better to remove the dot at the end.
| @@ -788,6 +788,7 @@ var BufKeyActions = map[string]BufKeyAction{ | |||
| "CopyLine": (*BufPane).CopyLine, | |||
| "Cut": (*BufPane).Cut, | |||
| "CutLine": (*BufPane).CutLine, | |||
| "CutLineAppend": (*BufPane).CutLineAppend, | |||
There was a problem hiding this comment.
How about adding it to MultiActions, i.e. making it work with multicursors, like other actions do?
Although if we just do that, it won't work with multicursors quite correctly (the first cursor's line will be appended to the clipboard twice), but that seems easy to fix, e.g.:
--- a/internal/action/actions.go
+++ b/internal/action/actions.go
@@ -1431,7 +1431,12 @@ func (h *BufPane) CutLineAppend() bool {
InfoBar.Error(err)
return false
} else {
- clipboard.WriteMulti(clip+string(h.Cursor.GetSelection()), clipboard.ClipboardReg, h.Cursor.Num, h.Buf.NumCursors())
+ sel := string(h.Cursor.GetSelection())
+ if h.Cursor.Num == 0 {
+ clipboard.WriteMulti(clip+sel, clipboard.ClipboardReg, h.Cursor.Num, h.Buf.NumCursors())
+ } else {
+ clipboard.WriteMulti(sel, clipboard.ClipboardReg, h.Cursor.Num, h.Buf.NumCursors())
+ }
totalLines = strings.Count(clip, "\n") + nlines
}
} else {There was a problem hiding this comment.
Seems reasonable, I will look into it
It was confusing for people that CutLine will occasionally accumulate cut lines if executed multiple times before the clipboard's content is pasted. Shortcut Ctrl-k is reserved for CutLineAppend, since CutLine already has Ctrl-x binded as well.
It seems like agreement on this solution was achieved during #3320