Skip to content
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

Item movers need some fixes #8492

Closed
dbofmmbt opened this issue Apr 12, 2021 · 5 comments · Fixed by #8967
Closed

Item movers need some fixes #8492

dbofmmbt opened this issue Apr 12, 2021 · 5 comments · Fixed by #8967

Comments

@dbofmmbt
Copy link
Contributor

dbofmmbt commented Apr 12, 2021

Hey, I really enjoy item movers (#6823), but I think it needs a little polishing.

The problems

  1. I can't move a function if my cursor is under the argument list. (update: the current feature of "move item up/down" over the argument list is to change their order)
  2. My cursor doesn't stay in the same position/column it was before applying the move. It seems that, depending on the function signature (maybe its size?) the cursor goes to a specific column.

These two, in combination, may be annoying, because sometimes my cursor goes to the argument list while I'm moving functions through a file and I have to move the cursor's position back to the function name in order to move it up again.

Example

fn main() {
    println!("Hello, world!");
}

fn hi(arg: usize) {}

If I move hi up, my cursor goes to column 8. If I move it down, it goes to column 2.

Environment

  • Linux
  • Rust Stable
  • Rust-Analyzer Nightly
  • VS Code
@dbofmmbt
Copy link
Contributor Author

dbofmmbt commented Apr 12, 2021

Oh, I just noticed that "problem 1" happens because currently when you use item movers over the argument list it moves the order of the arguments. IMO it fells weird to "move up/down" and the argument list be changed, as it is a more "horizontal" thing.

It seems to me that the suggestion in #8340 to use "move left/right" to do it is much more natural. Also, when we move the elements in the argument list, the callers are not being changed. That's another good suggestion present there.

@ivan770
Copy link
Contributor

ivan770 commented Apr 13, 2021

Current cursor algorithm is implemented here:
https://github.com/rust-analyzer/rust-analyzer/blob/10a243ea55565a0dd1de52f8f802c3e3a7bfef54/editors/code/src/commands.ts#L165-L173
It simply looks up for the range, that seems to be moved in a provided direction, and sets cursor position on its end.
Currently, I don't have any ideas about a better algorithm, that relies strictly on client code.

@dbofmmbt
Copy link
Contributor Author

dbofmmbt commented Apr 14, 2021

@ivan770 about the function you referenced above, isn't the cursor always ´null´ in the conditions of lines 166 and 170? (I may be missing something)

@ivan770
Copy link
Contributor

ivan770 commented Apr 14, 2021

@ivan770 about the function you referenced above, isn't the cursor always ´null´ in the conditions of lines 166 and 170? (I may be missing something)

There are multiple conditions - one for missing cursor, and one for the cursor in desired direction. If we don't have any cursor - set it. If current range is closer to desired direction - replace cursor

@jonas-schievink
Copy link
Contributor

I've opened #8510 to move the cursor position along with the item it's in

dbofmmbt added a commit to dbofmmbt/rust-analyzer that referenced this issue May 24, 2021
dbofmmbt added a commit to dbofmmbt/rust-analyzer that referenced this issue May 24, 2021
@bors bors bot closed this as completed in 3dce8a3 May 24, 2021
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 a pull request may close this issue.

3 participants