Skip to content

Move lines with Alt+Up/Down #230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

juemrami
Copy link
Contributor

@juemrami juemrami commented May 22, 2025

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
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Member

@lhecker lhecker left a comment

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
Copy link
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
- 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 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
Copy link

JanluOfficial commented Jun 2, 2025

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
Copy link

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

@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

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 {
Copy link
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..?

Copy link
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.

Copy link
Member

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@DHowett DHowett left a comment

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
Copy link
Member

@DHowett DHowett left a comment

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow lines to be moved up and down
4 participants