Skip to content

Move lines with Alt+Up/Down - #230

Merged
Leonard Hecker (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
Leonard Hecker (lhecker) merged 7 commits into
microsoft:mainfrom
juemrami:feat_move_cursor_or_selection_lines_with_alt_up_down

Conversation

@juemrami

@juemrami Julio Ramirez (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 Julio Ramirez (juemrami) changed the title Feat move cursor or selection lines with alt up down May 22, 2025
@juemrami

Julio Ramirez (juemrami) commented May 22, 2025

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree
lhecker
Leonard Hecker (lhecker) reviewed May 23, 2025
View reviewed changes

@lhecker Leonard Hecker (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

Julio Ramirez (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
Julio Ramirez (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
[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
Julio Ramirez (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

Janlu (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

Janlu (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 Leonard Hecker (lhecker) mentioned this pull request Jun 17, 2025
Closed
@lhecker Leonard Hecker (lhecker) changed the title [feat]: move cursor or selection lines with alt+up/down Jun 19, 2025
DHowett
Dustin L. Howett (DHowett) reviewed Jun 19, 2025
View reviewed changes
Comment thread src/buffer/mod.rs
DHowett
Dustin L. Howett (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 Dustin L. Howett (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 Leonard Hecker (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 Leonard Hecker (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
Dustin L. Howett (DHowett) approved these changes Jun 19, 2025
View reviewed changes

@DHowett Dustin L. Howett (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
Leonard Hecker (lhecker) enabled auto-merge (squash) June 19, 2025 22:14
DHowett
Dustin L. Howett (DHowett) approved these changes Jun 19, 2025
View reviewed changes

@DHowett Dustin L. Howett (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
Leonard Hecker (lhecker) merged commit 091b742 into microsoft:main Jun 19, 2025
Lou32 (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

Labels

None yet

4 participants

@juemrami @JanluOfficial @DHowett @lhecker