Improve and unify CopyLine, CutLine, DeleteLine, DuplicateLine actions
#3335
Conversation
After executing CutLine or DeleteLine action, the cursor is at the beginning of a line (as expected) but then moving the cursor up or down moves it to an unexpected location in the middle of the next or previous line. Fix this by updating the cursor's LastVisualX.
After executing the CopyLine action, moving cursor up or down unexpectedly moves cursor to the beginning of the line, since its LastVisualX value is lost in the selection/deselection manipulations. Fix this by restoring the original LastVisualX.
When the cursor is at the last line of buffer and it is an empty line, CopyLine does not copy this line, which is correct, but it shows a bogus "Copied line" message. Fix this by adding a check for that, same as in CutLine and DeleteLine.
The CutLine action has a feature: if we execute it multiple times to cut multiple lines, new cut lines are added to the previously cut lines in the clipboard instead of replacing the clipboard, unless those previously cut lines have been already pasted or the last cut was more than 10 seconds ago. This last bit doesn't really work: newly cut lines are appended to the clipboard regardless of when was the last cut. So fix it.
Make Copy return false if there is no selection, and change the default binding for Ctrl-c from CopyLine|Copy to Copy|CopyLine accordingly, to make the semantics more meaningful: copying selection always fails if there is no selection.
Change behavior of the Cut action: don't implicitly call CutLine if there is no selection. Instead, make it return false in this case and change the default Ctrl-x binding to Cut|CutLine, to make it clear, explicit and in line with Copy and CopyLine actions.
When there is a selection containing multiple lines, CutLine, DeleteLine and CopyLine actions currently cut/delete/copy just the "current" line, as usual. This behavior is at least confusing, since when there is a selection, the cursor is not displayed, so the user doesn't know which line is the current one. So change the behavior. When there is a multi-line selection, cut/delete/copy all lines covered by the selection, not just the current line. Note that it will cut/delete/copy whole lines, not just the selection itself, i.e. if the first and/or the last line of the selection is only partially within the selection, we will cut/delete/copy the entire first and last lines nonetheless.
Since CutLine may add lines to the clipboard instead of replacing the clipboard, improve its info message to show how many lines are in the clipboard in total, not just how many lines were added to it last time.
Weird behavior is observed e.g. if we cut some lines with CutLine, then copy some selection with Copy, then cut some other lines with CutLine, and then paste. The pasted cliboard contains not just the lines that were cut at the last step, but also the selection that was copied before that. Fix that by resetting the CutLine's repeated line cuts whenever we copy anything to the clipboard via any other action (Cut, Copy or CopyLine).
If we ever encounter this clipboard.Read() failure, return false immediately. Otherwise, InfoBar.Error(err) will have no effect (it will be immediately overwritten by InfoBar.Message()) so we won't even know that there was an error.
- Add a new Duplicate action which just duplicates the selection (and returns false if there is no selection). - Change the behavior of the DuplicateLine action to only duplicate the current line, not the selection. - Change the default action bound to Ctrl-d from DuplicateLine to Duplicate|DuplicateLine, so that the default behavior doesn't change. This allows the user to rebind keybindings in a more flexible way, i.e. to choose whether a key should duplicate just lines, or just selections, or both, - in a similar fashion to Copy, Cut, Delete actions.
Similarly to CutLine, DeleteLine and CopyLine actions, if there is a selection, duplicate not just the current line but all the lines covered (fully or partially) by the selection.
dmaluka
force-pushed
the
line-actions-cleanup
branch
from
d79a331
to
6f724bc
Compare
internal/action/actions.go
Outdated
| return false | ||
| } | ||
| totalLines := nlines | ||
| if h.freshClip && time.Since(h.lastCutTime) < 10*time.Second { |
There was a problem hiding this comment.
- Minor thing,
| if h.freshClip && time.Since(h.lastCutTime) < 10*time.Second { | |
| if h.freshClip && time.Since(h.lastCutTime) < 10 * time.Second { |
- 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
CutLineimplementation toCutLineAppend
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.
There was a problem hiding this comment.
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.
CutLineAppendis fine I think.
Or ClipLine, maybe?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
CutLinebehavior 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)
There was a problem hiding this comment.
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
CutLinebehavior as is (just document it somewhere; and remove the 10 seconds part). If the user wants the simple behavior instead, the user can just useCopyLine,DeleteLine, right?
Yep, I'm fine with that too, since I didn't used it so far.
There was a problem hiding this comment.
Removed the lastCutTime feature and added documentation for the CutLine behavior.
The lastCutTime feature (reset the clipboard instead of appending to the clipboard if the last CutLine was more than 10 seconds ago) was implemented 8 years ago but was always buggy and never really worked, until we have accidentally found and fixed the bug just now. No one ever complained or noticed that, which means it is not a very useful feature. Fixing it changes the existing behavior (essentially adds a new feature which did not really exist before) and there is no reason to assume that this new behavior will be welcome by users. So it's better to remove this feature.
|
Hi I have had some thoughts about Kind Regards Gavin Holt |
CopyandCutcopy or cut only the selection (and return false if there is no selection), whileCopyLineandCutLinecopy or cut the current line.Duplicateaction which duplicates the selection only, and similarly clean up the actions behavior:Duplicateduplicates the selection (and returns false if there is no selection), whileDuplicateLineduplicates the current line. See [Feature request]: Duplicate lazy line selection #3110current ones:
changed to:
so the default behavior of these keys doesn't change.
Make
CopyLine,CutLine,DeleteLineandDuplicateLinerespect the selection (see DeleteLine does not delete selected lines #3328). When there is a multi-line selection, these actions should not just copy/cut/delete/duplicate the "current" line, as usual. This behavior is at least confusing, since when there is a selection, the cursor is not displayed, so the user doesn't know which line is the current one.So change the behavior of these actions: copy/cut/delete/duplicate all lines covered by the selection, not just the current line. Note that we cut/delete/copy/duplicate not just the selection itself (for that we have other actions:
Copy,Cutetc) but whole lines, i.e. if the first and/or the last line of the selection is only partially within the selection, we cut/delete/copy the entire first and last lines nonetheless.Fixes #3328
Fixes #3110