Add support for appending to named registers #731
Conversation
| if (typeof existingText === 'string') { | ||
| content = existingText + (existingText.endsWith('\n') ? '' : '\n') + content; | ||
| } else { | ||
| content = existingText.join('\n') + '\n' + content; |
There was a problem hiding this comment.
Careful. The reason that existingText can be a string[] is because of yanking in visual block mode - you copy an entire block, and then paste the whole block later. (Same thing with content[].) I'm don't think that flattening down that array is the right thing to do here, because then you lose that block.
There was a problem hiding this comment.
Yes I was unsure about this. The approach I took preserves the type of the new content, so string + string[] -> string[], string[] + string -> string.
Are you saying that if either existingText or content is string[] then the result should be string[]?
|
Thanks for the PR, @dominic31! Looks good to me, just a few concerns about how we handle visual block mode copy/paste. |
|
@johnfn Thanks for the comments! I've pushed some changes, hopefully it looks better now. |
|
Ergh. After investigating registers a little more, I believe that I have actually implemented registers incorrectly in the case of visual block mode! It seems like when you copy in visual block mode, you simply copy each line separated by a newline. This is different than what we are doing, where if you copy in visual block mode, you actually copy an array of each line. I would encourage you to go ahead and fix this, but actually I am anticipating some annoying merge conflicts with a branch I have been working on for a while #587 which adds yet another register case type. If you really want to, you could first copy my register changes into here from my PR, and then work on top of them to replace the |
Where does this happen? When appending to registers in visual block mode, I concatenate the arrays, |
|
Closing this PR as there haven't been any movement in the last month. Fee free to re-open the PR and let us know when it's ready for review. |
|
@jpoon I was waiting for a reply to my last comment, else it is as far as I can see ready. |
|
@dominic31 My bad. Some of this logic changed since your last rebase, can you fix the conflicts then we can give it a quick review and merge? |
| @@ -52,6 +52,27 @@ export class Register { | |||
| clipboard.copy(content); | |||
| } | |||
|
|
|||
| // Upper-case register name denotes append. | |||
| if (/^[A-Z]$/.test(register)) { | |||
There was a problem hiding this comment.
Can you move this into it's own separate static function (isNamedRegister), similar to isValidRegister?
|
I'm really sorry I didn't respond to that comment! I'm getting spammed with notifications from Github constantly and it makes it really difficult to find the stuff that matters. :( Anywho, if you can make this work for the new register type I alluded to, we should be good here. |
Named registers should be case insensitive, with lower case denoting replace register contents, and upper case denoting append to register contents.
Vim documentation, Named Registers