Skip to content

Implemented Line shifting using Alt+Up/Down #166

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

Closed
wants to merge 5 commits into from

Conversation

JanluOfficial
Copy link

About this PR

This PR adds the requested feature from #27, that being the ability for lines to be moved up and down using Alt + Up/Down

Demo Video

2025-05-21.01-14-14.mp4

@JanluOfficial
Copy link
Author

@microsoft-github-policy-service agree

@lhecker
Copy link
Member

lhecker commented May 20, 2025

I'm too sleepy to do a full review today still1, but one thing I noticed at a cursory glance is that this code can be abstracted away, can't it? Moving a line up and down should roughly be similar logic in both directions so I think you could move this into a single function with a bool parameter. This would help keep the binary size low.

Footnotes

  1. I've been triaging and responding to comments for the last 15 hours haha. The reaction to this project is way more positive than I had anticipated!

@diabloproject
Copy link
Contributor

Does not work in iTerm2, may also not work under other ones?

CleanShot.2025-05-21.at.03.21.17.mp4

@JanluOfficial
Copy link
Author

JanluOfficial commented May 21, 2025

I'm too sleepy to do a full review today still1, but one thing I noticed at a cursory glance is that this code can be abstracted away, can't it? Moving a line up and down should roughly be similar logic in both directions so I think you could move this into a single function with a bool parameter. This would help keep the binary size low.

Footnotes

  1. I've been triaging and responding to comments for the last 15 hours haha. The reaction to this project is way more positive than I had anticipated!

Yeah I wrote that while I was dead tired as well. Only reason I was up this late was because Google I/O went on for so long. I'll try to make both solutions closer to one another. I remember that I tried to make them essentially the same yesterday but that was causing some weird behavior. I'll see if I can get that weird behavior state for a recording when I try to adjust my current implementation. Honestly, I haven't worked with rust that much before, so this is good practice.

Once I get moving down working with basically the same code as moving lines up, I'll abstract it.

Edit: I managed to get back to that "weird behavior" by doing what I initially did yesterday, i.e. copying the "Shifting up" code and trying to adjust it for shifting down. I'll figure this out after school.

2025-05-21.08-06-20.mp4

@JanluOfficial
Copy link
Author

JanluOfficial commented May 21, 2025

Does not work in iTerm2, may also not work under other ones?

CleanShot.2025-05-21.at.03.21.17.mp4

That is really weird. I only added a condition to vk::Up and vk::Down for when Alt is being held... My only guess is that it's an issue with input handling on macOS. Can you or anyone else with a mac test if this also applies to the regular terminal or if it's only an iTerm2 thing? I don't have the money to buy a mac right now so I'm unable to test this myself. I'll test it under WSL to see if it's an issue on Linux, if so then it's likely something in input.rs by my guesses.

I'll definitely get back to you about the Linux test after school, gotta head out in a few minutes so I don't miss the bus xD.

@diabloproject
Copy link
Contributor

diabloproject commented May 21, 2025

Can you or anyone else with a mac test if this also applies to the regular terminal or if it's only an iTerm2 thing?

CleanShot.2025-05-21.at.11.04.06.mp4

It behaves differently than iTerm2, but it still does dos not work. It just moves the cursor.

@JanluOfficial
Copy link
Author

JanluOfficial commented May 21, 2025

It just moves the cursor

I'm going to make the assumption that the ALT modifier is not triggered by its mac equivelant key (Option if we want to mimic VS Code's shortcuts). If anyone wants to make Option trigger the Alt modifier, it would be greatly appreciated. The relevant code seems to be in input.rs starting at line 206. That said, I have no idea if my assumption is correct.

Still no idea why iTerm2 would react differently to be honest. Maybe something with the shortcuts configuration in iTerm2?

@diabloproject
Copy link
Contributor

Still no idea why iTerm2 would react differently to be honest. Maybe something with the shortcuts configuration in iTerm2?

Well, I have a default keymap, and with default keymap iterm behaves like this. You 100% can change it through key remapping, but how many people are going to remap half of the keys if the terminal works in other use cases?

@diabloproject
Copy link
Contributor

Maybe we can use this? @JanluOfficial

    fn textarea_shift_line(&mut self, tb: &mut TextBuffer, direction: i32) {
        let line_count = tb.visual_line_count() as i32;
        let current_line = tb.cursor_visual_pos().y;
        let target_line = current_line + direction;

        if target_line < 0 || target_line >= line_count {
            return;
        }

        let original_cursor = tb.cursor_visual_pos();
        let first_line = current_line.min(target_line);
        let second_line = current_line.max(target_line);

        tb.cursor_move_to_logical(Point { x: 0, y: second_line });
        let content = tb.extract_selection(true);
        tb.cursor_move_to_logical(Point { x: 0, y: first_line });
        tb.write(&content, true);

        tb.cursor_move_to_logical(Point { x: original_cursor.x, y: target_line });
    }

@JanluOfficial
Copy link
Author

Maybe we can use this? @JanluOfficial

    fn textarea_shift_line(&mut self, tb: &mut TextBuffer, direction: i32) {
        let line_count = tb.visual_line_count() as i32;
        let current_line = tb.cursor_visual_pos().y;
        let target_line = current_line + direction;

        if target_line < 0 || target_line >= line_count {
            return;
        }

        let original_cursor = tb.cursor_visual_pos();
        let first_line = current_line.min(target_line);
        let second_line = current_line.max(target_line);

        tb.cursor_move_to_logical(Point { x: 0, y: second_line });
        let content = tb.extract_selection(true);
        tb.cursor_move_to_logical(Point { x: 0, y: first_line });
        tb.write(&content, true);

        tb.cursor_move_to_logical(Point { x: original_cursor.x, y: target_line });
    }

Let me check if this works the exact same way as my current abstracted function.

@JanluOfficial
Copy link
Author

JanluOfficial commented May 21, 2025

Okay so there is a bug, and I confirmed it happens with both your function and my lesser optimized function. Essentially, trying to shift up the last line in the file (or I guess technically the buffer) just adds it to the front of the line above instead of moving the line above down. I'll see what I can do here.

Other than that, the new (way more optimized) function works perfectly.

@diabloproject
Copy link
Contributor

fixes the bug:

    fn textarea_shift_line(&mut self, tb: &mut TextBuffer, direction: i32) {
        let line_count = tb.visual_line_count() as i32;
        let current_line = tb.cursor_visual_pos().y;
        let target_line = current_line + direction;

        if target_line < 0 || target_line >= line_count {
            return;
        }

        let original_cursor = tb.cursor_visual_pos();
        let first_line = current_line.min(target_line);
        let second_line = current_line.max(target_line);

        tb.cursor_move_to_logical(Point { x: 0, y: first_line });
        let mut content = tb.extract_selection(true);
        tb.cursor_move_to_logical(Point { x: 0, y: second_line });
        if second_line == line_count - 1 {
            tb.write(b"\n", true);
            tb.cursor_move_to_logical(Point { x: 0, y: second_line });
            content.pop();
        }
        tb.write(&content, true);

        tb.cursor_move_to_logical(Point { x: original_cursor.x, y: target_line });
   }

@JanluOfficial
Copy link
Author

Thank you @diabloproject for the help. I greatly appreciate it. As for the macOS stuff, I can't really fix that since I don't own a mac, which I'd need for testing. Hopefully someone else can take care of that.

@diabloproject
Copy link
Contributor

No problem. I think the keyboard shortcuts for mac could be made in a separate PR that will introduce mappings, later, since it is probably out-of-scope for this task.

@JanluOfficial
Copy link
Author

Then I'd consider this PR mergeable. I'll let someone else take the mac thing because I don't own a mac unfortunately.

@lhecker
Copy link
Member

lhecker commented Jun 17, 2025

I'll close this in favor of #230, as it's more thorough. Counter-intuitively though, its approach is not actually as much better as you may expect. It will require some simplifications. 🙂

@lhecker lhecker closed this Jun 17, 2025
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.

3 participants