Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Ctrl-X: Cut line on no selection #7212

Closed
wants to merge 3 commits into from
Closed

Ctrl-X: Cut line on no selection #7212

wants to merge 3 commits into from

Conversation

wnr
Copy link

@wnr wnr commented Mar 17, 2014

As discussed is issue #2017 and at trello https://trello.com/c/XVyzky6d

This enables the use of Ctrl-X to cut lines when no text is selected, just like in Sublime, IntelliJ and other modern editors.

Since cutting is done by the shell, this fix simply selects the whole line of each cursor that doesn't have a selection, before letting the shell do it's thing. This results in the desired behaviour.

So to sum up:

  • If you have a cursor at a line (with no selection), hitting Ctrl-X will cut the whole line.
  • If you have a text selection, hitting Ctrl-X will work normal as before.
  • Any combinations with multiple cursors is supported.

@njx
Copy link
Contributor

njx commented Mar 17, 2014

Ah, interesting solution! Since you're cutting anyway, changing the selection first is mostly harmless. A couple of thoughts though:

  • You see the selection flash briefly before the cut. But I actually think that might be a feature :) - it actually acts as a bit of feedback on what it's doing, and doesn't feel bad to me.
  • The selection change shows up if you undo, which feels a bit strange. There probably isn't a way to avoid this with this kind of implementation - we would need some support in CodeMirror to avoid it.
  • This also doesn't generalize to Copy (since you really wouldn't want to change the user's selection in that case). Does it make sense to implement this for Cut and not Copy?

I think we were assuming that the right way to do this would be to try implement it in CodeMirror, without literally changing the actual CodeMirror selection, but rather by setting the selection in the hidden input field that CM uses to manage cut/copy (so when the selection is a cursor, automatically select the containing line in the underlying input field). It might be worth investigating that, because that would avoid having the changed selection actually show up in the UI or in the undo history. I'm not sure if messing with the hidden input field would break something else though - it might be worth starting a conversation with Marijn on this idea.

@wnr
Copy link
Author

wnr commented Mar 17, 2014

Thanks for the feedback :)

Yes, ultimately we would like to make Ctrl-C work in the same way as the extended Ctrl-X. Implementing this functionality in CM is probably the best way. However, since ctrl-x on empty selections is such a wanted feature I thought it made sense to solve it this way until a cleaner solution is achieved. The fact that this has been requested 1 year ago justifies this quick solution, according to me. I believe that the extended Ctrl-C is not as wanted (or critical) as Ctrl-X, since the extended Ctrl-C can be achieved by using Ctrl-X followed by an undo (I use this for editors that do not support extended ctrl-c but does support extended ctrl-x).

The only problem that I see with this serving as a quick solution until we do it the right way is your second point (which I failed to spot myself). I think you are right with that this is nothing we can go around with this approach. The question is if it brings the experience down more than up, which I think it doesn't. The reason is that for users that use Ctrl-X the normal (current) way will never experience this problem. Only the users that actually uses the added feature of Ctrl-X will experience that when they undo the line it would be selected. So my view of it is that users unaware (or not using) of the extended Ctrl-X will experience no difference. Only the users wanting to use extended Ctrl-X will experience a minor flaw, which I think they will choose over not having the extended Ctrl-X at all.

Summed up: I think we should merge this despite the flaws, and then try to create a clean solution in CM in the mean time.

@njx
Copy link
Contributor

njx commented Mar 17, 2014

That's a good argument. @peterflynn - what do you think? (since you were one of the folks who want this feature)

@njx
Copy link
Contributor

njx commented Mar 17, 2014

FYI, I filed codemirror/codemirror5#2382 to request this feature in CM.

@njx
Copy link
Contributor

njx commented Mar 18, 2014

@wnr Looks like this got assigned to me for review - I'm actually out of town at the moment but should get a chance to look at this in more detail when I get back next week. At first glance the code looks good though - nice functional style and unit tests.

@njx
Copy link
Contributor

njx commented Mar 18, 2014

Random aside: your use of filter()/reject() makes me think that lodash ought to have a function that does both at once - i.e., given a collection and a function, return two arrays, one with the items for which the function is truthy and one with the items for which it's not. Of course it's easy enough to just use both functions, but it would be slightly nicer/more efficient to be able to do it in one pass.

@peterflynn
Copy link
Member

I think it would actually be ok to change the selection when you hit Copy with a simple cursor as well -- given that it does nothing today, it's not going to affect anyone's existing workflow. And it's actually a nice indicator of what got copied. And there's some precedent, since IntelliJ / WebStorm / etc. behave this way today.

I'm guessing the selection flash in the Cut case could be avoided by moving this code into a "beforecut" event handler. (Although it's not a W3C standard event, I believe all major browsers support it).

@njx Do you think Marijn is likely to implement a solution in CM fairly soon though? He usually seems to move pretty quickly. If that's going to happen before 38 ships anyway, then it could be worth just waiting for that cleaner fix...

@wnr
Copy link
Author

wnr commented Mar 19, 2014

@njx Okay great, thank you! Yes, you are totally right about the combination of filter / reject, I'll fix this tomorrow.

@peterflynn Interesting, I never expanded that thought to copy, but it does make sense. Yes, beforecut would probably solve the flashing (didn't know it existed till now). Shall I rewrite my code to use that instead?

I agree that if CM supports this feature soon, it's a better alternative. Maybe we can ask Marijn about the time frame and if he'll implement the feature after 38 ships, we can use my fix until CM supports it natively?

@njx
Copy link
Contributor

njx commented Mar 21, 2014

FYI looks like Marijn implemented this - please give it a try and give him feedback (I haven't tried yet) codemirror/codemirror5@12e3efe

@wnr
Copy link
Author

wnr commented Mar 24, 2014

Cool! I played around with it and it seems to work roughly the same. Still the visual behaviour of the selections. There are some things I do not like however:

  • ctrl-x results in a highlight flash while ctrl-c doesnt.
  • ctrl-x with mix of word / line cursors breaks the cutting (only the words get cut).
  • Same as above goes for ctrl-c.

I'm going to give Marijn the feedback, and hopefully we can use it later :)

@marcelgerber
Copy link
Contributor

It's in Brackets now (and works) as CodeMirror was updated.
Going to be in Release 39.

@peterflynn
Copy link
Member

Much rejoicing!

@wnr
Copy link
Author

wnr commented Apr 21, 2014

Great!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants