Skip to content

Move lines with Alt+Up/Down#230

Merged
lhecker merged 7 commits into
microsoft:mainfrom
juemrami:feat_move_cursor_or_selection_lines_with_alt_up_down
Jun 19, 2025
Merged

Move lines with Alt+Up/Down#230
lhecker merged 7 commits into
microsoft:mainfrom
juemrami:feat_move_cursor_or_selection_lines_with_alt_up_down

Conversation

@juemrami

@juemrami juemrami commented May 22, 2025 •
edited by lhecker
Loading

Copy link
Copy Markdown
Contributor

Closes #27

In this pr

[feat]: move cursor or selection lines with alt+up/down

  • addresses Allow lines to be moved up and down #27
  • Respects vscode functionality:
      1. if no selection is made, move the line at the current cursor up/down
      1. if a selection is made, move all lines spanning the selection range up/down
      • if the end cursor is at the start of a newline it doesn't count as part of the range

[dev][feat]: add tests for move_selected_lines command

  • These can be removed from the PR (maybe keep 1-2 of the edge case checks?)
  • I mostly made them for my sanity while dev'ing because I kept hitting weird edge cases.
  • I figured id include them in the PR to just show some semblance of correctness for reviewers.

Important

I couldnt figure out how to get the TextBuffer's scratch arena to work with parallel tests so they need to be run with --test-threads=1

Additional Notes

  • Atm undoing an operation ctrl+z sets the selection to the entire range of the source text & swap location text.
    • This is due to how i use a selection to make sure im writing to the correct location with TextBuffer.write()
    • This is wrong and the restored selection should be the original selection before the move.
    • Any suggestions on correcting this?
    • Addressed in this rebase
  • I'm unsure if the way im creating the reordered text is efficient or not.
    • maybe i should be using a scratch_arena ?
  • Also i'm thinking maybe i should have used a Result return instead of a boolean for the failure cases.

images
WindowsTerminal_QdRVlK8i3I
image

@juemrami juemrami changed the title Feat move cursor or selection lines with alt up down [feat]: move cursor or selection lines with alt+up/down May 22, 2025
@juemrami

juemrami commented May 22, 2025

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

lhecker
lhecker reviewed May 23, 2025
View reviewed changes

@lhecker lhecker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, that's a thorough PR. 😅 This may take me a bit to come back to, as I focus on bug fixes first.

There's also #166 which is a competing solution for this. I believe yours is likely to be more correct though. I think I may just merge the other one first and then yours, given that the other PR is very small.

@juemrami

juemrami commented May 23, 2025

Copy link
Copy Markdown
Contributor Author

for sure. No problem.

And no pressure btw, I know this project got a pretty big hug of death from the community and you got a lot on your plate rn so thanks for even looking at this.

I might look for some other smaller things to tackle. GL!

@juemrami juemrami force-pushed the feat_move_cursor_or_selection_lines_with_alt_up_down branch from 124d978 to d37c46f Compare May 24, 2025 08:01
juemrami added 2 commits May 27, 2025 11:21
@juemrami
[feat]: move cursor or selection lines with alt+up/down
30d93d7 
- Respects vscode functionality:
 - 1. if no selection is made, move the line at the current cursor up/down
 - 2. if a selection is made, move all lines spanning the selection up/down
@juemrami
[dev][feat]: add tests for move_selected_lines command
15d9953
@juemrami juemrami force-pushed the feat_move_cursor_or_selection_lines_with_alt_up_down branch from d37c46f to 15d9953 Compare May 27, 2025 18:38
@JanluOfficial

JanluOfficial commented Jun 2, 2025 •
edited
Loading

Copy link
Copy Markdown

This solution is definitely better than mine is currently. I'm probably going to rewrite mine soon-ish to be closer to this. Not trying to make this some sort of competition though xD

@JanluOfficial

JanluOfficial commented Jun 11, 2025

Copy link
Copy Markdown

After a bit of thinking, I think my solution should be retired. This is like... 10x better.

@lhecker lhecker mentioned this pull request Jun 17, 2025
Closed
@lhecker lhecker changed the title [feat]: move cursor or selection lines with alt+up/down Move lines with Alt+Up/Down Jun 19, 2025
@lhecker lhecker requested a review from DHowett June 19, 2025 19:27
DHowett
DHowett reviewed Jun 19, 2025
View reviewed changes
Comment thread src/buffer/mod.rs
DHowett
DHowett reviewed Jun 19, 2025
View reviewed changes
Comment thread src/buffer/mod.rs

self.cursor_move_to_logical(Point { x: 0, y: paste });
self.edit_begin(HistoryType::Write, self.cursor);
if line.is_empty() || self.cursor.logical_pos.y != paste {

@DHowett DHowett Jun 19, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| self.cursor.logical_pos.y != paste

what common occurrence does this handle? The cursor couldn't be moved to the paste line, so it's not on the paste line..?

@lhecker lhecker Jun 19, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I wanted to document this code before submitting it and forgot. Let me fix that.

@lhecker lhecker Jun 19, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

DHowett
DHowett approved these changes Jun 19, 2025
View reviewed changes

@DHowett DHowett left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I think I get it

@lhecker lhecker enabled auto-merge (squash) June 19, 2025 22:14
DHowett
DHowett approved these changes Jun 19, 2025
View reviewed changes

@DHowett DHowett left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's beautiful. Thanks both of you for working on it

@lhecker lhecker merged commit 091b742 into microsoft:main Jun 19, 2025
3 checks passed
Lou32Verbose pushed a commit to Lou32Verbose/edit that referenced this pull request Jan 11, 2026
@juemrami @lhecker
Move lines with Alt+Up/Down (microsoft#230)
e32431c 
Closes microsoft#27

Co-authored-by: Leonard Hecker <leonard@hecker.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhecker lhecker lhecker left review comments

@DHowett DHowett DHowett approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow lines to be moved up and down

4 participants

@juemrami @JanluOfficial @DHowett @lhecker