Add support for moving lines or selections above and below

[sireliah]

PR implementing moving lines or selections up and down. See more in #2245

[workingj]

love it !
😃

[the-mikedavis]

The behavior of this on fully selected lines is fine. Moving the full line when only a subset of the line is selected though conflicts with the select-then-act paradigm.

You end up paying for it both in terms of code (conflict resolution functions for overlapping selections; also see all the deleted code in #4123 for a similar situation) and in multi-cursor behavior (for example: C and then C-j moves the cursors oddly; C and then C-k removes one of the cursors).

To reiterate #2245 (comment) (especially since #4458): this can be roughly implemented in terms of existing commands

[keys.normal]
C-j = ["extend_to_line_bounds", "delete_selection", "paste_after"]
C-k = ["extend_to_line_bounds", "delete_selection", "move_line_up", "paste_before"]

So unless this adds anything extra like handling indentation, I don't see a need to add extra commands for this.

[the-mikedavis]

This can be written more cleanly without itertools; no need to pull in an extra dependency.

let mut new_changes: Vec<Change> = Vec::new();

for change in changes {
    match new_changes.last() {
        Some(last_change) if last_change.0 >= change.0 || last_change.1 >= change.1 => {
            return new_changes;
        }
        _ => new_changes.push(change.clone()),
    }
}

new_changes

[sireliah]

Thanks for the suggestions!

Moving the full line when only a subset of the line is selected though conflicts with the select-then-act paradigm.

Yeah, I also wondered about this being a bit inconsistent in the sense that modifications are usually applied to the selected area. The way I solved the problem is similar to what other editors do, such as sublime text.

If we have in mind that helix always has some selection (cursor itself), then moving not fully selected line would be conflicting with select-then-act paradigm. Do you have alternative suggestion that would keep the feature usable?

multi-cursor behavior (for example: C and then C-j moves the cursors oddly; C and then C-k removes one of the cursors).

As I mentioned in the issue comment, I've left some edge case unresolved until I get signal that community want feature like this to be supported at all. If we want to have full multi cursor support, I can work on these cases.

So unless this adds anything extra like handling indentation, I don't see a need to add extra commands for this.

Again, I communicated I would like to add indentation as the next step, but there's nothing preventing me from doing that in this PR (since it's just one function).

[sireliah]

This can be written more cleanly without itertools; no need to pull in an extra dependency.

let mut new_changes: Vec<Change> = Vec::new();

for change in changes {
    match new_changes.last() {
        Some(last_change) if last_change.0 >= change.0 || last_change.1 >= change.1 => {
            return new_changes;
        }
        _ => new_changes.push(change.clone()),
    }
}

new_changes

Thanks, I'll do it!

[7ombie]

To reiterate #2245 (comment) (especially since #4458): this can be roughly implemented in terms of existing commands

This is incorrect. There's currently no way to implement this functionality in Insert Mode, and it's broken in Normal Mode (the cursor position is lost). Sorry, but "roughly implemented" here just means partial and buggy.

So unless this adds anything extra like handling indentation, I don't see a need to add extra commands for this.

I like to use Shift with the arrow keys to move text up, down, left and right. This allows me to easily (manually) alter the indentation as I move blocks up and down. I prefer this over an editor that indents and dedents code automatically as I move it (I always found that janky and inconsistent).

Helix should implement these commands, if only for the sake of correctness.

[pascalkuthe]

To reiterate #2245 (comment) (especially since #4458): this can be roughly implemented in terms of existing commands

This is incorrect. There's currently no way to implement this functionality in Insert Mode, and it's broken in Normal Mode (the cursor position is lost). Sorry, but "roughly implemented" here just means partial and buggy.

So unless this adds anything extra like handling indentation, I don't see a need to add extra commands for this.

• dking aything in insert mode besides inserting text and automcompletions is an anti feature and a usecase we do not support. You should use normal mode
• Helix follows the selection -> action model so any moved lines must be first selected so what you call losing the selection is on purpose because text must be selected to be acted upon. This is an absolutely fundamental concept of the editor that we will not deviate from.

[sireliah]

Helix follows the selection -> action model so any moved lines must be first selected so what you call losing the selection is on purpose because text must be selected to be acted upon. This is an absolutely fundamental concept of the editor that we will not deviate from.

I'd like the helix maintainers and community to finally agree on the acceptance criteria of this feature. In the current solution the "selection -> action model" is not honored, because not selected lines are moved.

@pascalkuthe @the-mikedavis would you mind setting up some kind of poll or question that would settle this matter? At this point I'm interested in clear decision what behaviour is expected in the editor. Otherwise I'm fine with closing this PR and forgetting about this feature request.

[7ombie]

@pascalkuthe - Just to be sure that I read that correctly, I've paraphrased it here:

Doing anything in Insert Mode, besides inserting text and auto-completions, is an anti-feature and a usecase we do not support. You should use Normal Mode.

Sorry, but calling a usecase an anti-feature, then just stating that it's a thing we do not support, without any elaboration, doesn't really explain anything.

I guess I now know that my Helix configuration is entirely at odds with some unspecified philosophy, and that Helix forces it's users to edit text as it's developer's intended, but don't see why I can't bind whatever I like to whatever I like. It's my editor after all.

Helix follows the selection -> action model so any moved lines must be first selected so what you call losing the selection is on purpose because text must be selected to be acted upon. This is an absolutely fundamental concept of the editor that we will not deviate from.

This makes sense. Point taken.

[7ombie]

Please help me understand what I'm missing here...

In Insert Mode, I use simple bindings (Top Layer and Shift Layer) for editing text. In Normal Mode (and Select Mode), I use those bindings to run commands.

The chorded bindings (using Alt and Ctrl) are always available (in Insert Mode and Normal Mode), and there's no reason for them behave inconsistently across modes. Only the simple bindings need to be modal.

I personally want Normal Mode and Select Mode to use the same bindings, and for Insert Mode to only differ in terms of what it binds to the Top Layer and Shift Layer. This seems so simple and intuitive, but it's not allowed.

I can save a file in Insert Mode, but I'm not allowed to indent code, or swap adjacent lines of code, or comment-out a selection (even though I have bindings for those operations that don't conflict with anything). I have to switch back and forth between modes for no apparent reason. They're just "anti features" otherwise.

I'd be grateful if anyone could elaborate.

[the-mikedavis]

I'm strongly against this behavior since it breaks with select-then-act. I think this would be a good test when the plugin system exists that the API is flexible enough but I don't want to see it in core. I won't close it outright since others may be interested in merging your work into their own forks but I won't be reviewing or merging it.

[the-mikedavis]

I guess I now know that my Helix configuration is entirely at odds with some unspecified philosophy, and that Helix forces it's users to edit text as it's developer's intended, but don't see why I can't bind whatever I like to whatever I like.

See vision.md. We're opinionated about modal editing and select-then-act and not interested in supporting other editing modes. Insert mode is just for basic text insertion/deletion and more complicated edits belong in normal or select mode. We also don't like the readline-like commands in insert mode (see here).

It's my editor after all.

It can be if you fork it. I'm not trying to be snarky here: I genuinely recommend forking and making whatever changes you want. Then it's truly your editor.

I can save a file in Insert Mode, but I'm not allowed to indent code, or swap adjacent lines of code, or comment-out a selection

FWIW saving a file in insert mode also isn't supported and we're not interested in changing that: #2883.

[7ombie]

@the-mikedavis - Thank you for taking the time to reply. I appreciate that.

FWIW saving a file in insert mode also isn't supported and we're not interested in changing that.

Sorry. I misread the docs. Ctrl-S is bound to commit_undo_checkpoint. I just thought that meant save and update the version control or something. I had to re-read it.

It can be if you fork it. I'm not trying to be snarky here: I genuinely recommend forking and making whatever changes you want. Then it's truly your editor.

I can't argue with that. It's your project after all. I don't like Rust, so I'll have to try something else, but you're right still. Again, thanks for taking the time. All the best.

P.S. I was able to get it working (I just toggle modes inside the command, so it acts like it's still in Insert Mode). I'll still need to find something else though, longer term.

[omentic]

This patch is very nice, thanks for the hard work. It is buggy when Unicode characters are involved however - the cursor position will skip forward or backward a few positions upon every movement until it skips itself off the line and starts moving something else. It's probably just accidentally treating Unicode characters as their size rather than as a single grapheme, I'll sit down and bughunt for it in the next few days.

[GeekPro101]

I don't know anything about the internals of Helix, but is it not possible to require entire line(s) be selected, and that they're in insert mode before the command will do anything? Then the requirement of selection->action isn't broken, and you don't get the issue of moving partial bits of a line. Might well be misunderstanding above discussion however.

[workingj]

Hi, any progress in sight here?

[omentic]

I have it (and a number of other patches) merged into my fork. I'll rebase on master and see about distribution tomorrow.

[omentic]

Found the bug: change.line_text.as_ref().map_or(0, |x| x.len()) messes up with Unicode because len() returns the length of the string in bytes. You need .chars().count().

Made a couple of trivial changes to this patch and merged into my fork at https://github.com/omentic/helix-ext (also on the AUR).

[sireliah]

Awesome, thanks for debugging that @omentic!

[milesgranger]

@the-mikedavis Referred to this PR by the issue after this comment.

To my understanding, the argument of select-then-act being broken b/c of moving a partial line selection up or down is inconsistent as we can currently select a partial line and de/indent the entire line.

To be clear, I'm in no way advocating de/indenting should only apply to the selection of a partially selected line, that would be maddening. :) Only that the logic doesn't hold here when talking about moving partially selected lines up or down, when it's allowed when moving side to side.

[workingj]

@sireliah grate work so far, would you mind finishing it? :D
my knowledge and programing experience are quite not there yet to tackle this i think...

[omentic]

@workingj It is finished. The Helix devs have expressed that they don't want it in the editor. If you want it you can try my fork.

[7ombie]

The Select Then Act Principle evidently works very well as a design guideline, but it makes no sense to adhere to it when it doesn't work. Indentation is a perfect example. Some operations are intrinsically line-based, so having operations that implicitly extend selections to the entire line seems totally natural (and already necessary).

I'm not convinced we need the feature (in its current form), and have explained why elsewhere. I just generally disagree with dismissing popular feature requests out-of-hand, just because they conflict with a design guideline.

[workingj]

@workingj It is finished. The Helix devs have expressed that they don't want it in the editor. If you want it you can try my fork.

Thankyou!
Ill do so. 😀