Move Up/Down Code Actions

[hediet]

Personally, I think generic Up/Down Move Code Actions are extremely useful. There should be two clever commands: Move Up and Move Down. Dependening on the selection and context, they do "the right". Because there are only two, they should have good keybindings.

Here is an (incomplete) collection of what they would enable:

• [x] Move Statements / Declarations
• [x] Move Declarations inside multi-variable declaration
• [x] Move Class Members
• [x] Move Array Items (taking care of commas)
  • [x] Move array destructuring items
• [X] Move Object Properties (taking care of commas)
  • [x] Move object destructuring properties
• [x] Move If/Else Branches https://github.com/p42ai/refactor-vscode/issues/28
• [x] Move nested if-statements up/down
• [ ] https://github.com/p42ai/refactor-vscode/issues/41
• [ ] Move expression in homogenous condition (a && |b && c -> |b && a && c) (note: overlap/replacement of 'flip operator')
• [ ] Move argument in function invocation (f(1, |2) -> f(|2, 1))
• [ ] Move Parameters (fixing all call-sites) (note: technically hard - requires multi-file, dataflow, etc.; more limited version possible)
• [ ] Move JSX elements
• [x] Move JSX attributes
• [x] Move Case-Branches in Switch Statements
• TypeScript:
  • [x] move interface members
  • [x] move type members
  • [ ] re-order union types

Sometimes a move is safe, sometimes not.

A key property of move commands should be that "Move up" and "Move down" are inverse.

[lgrammel]

Great idea, thanks for the writeup!

Regarding one command: I'd much rather leverage grouping through code action kind prefixes and have individual code actions - this gives users more flexibility configuring their keyboard shortcuts without losing anything ( see https://p42.ai/documentation/p42-for-vscode/editor-integration#keyboard-shortcuts for some examples).

Code action kind prefixes could be:

• "refactor.move.up.*"
• "refactor.move.down.*"
• (TBD) "refactor.move.swap.*"

This would allow grouping all move refactorings (incl. to different files in the future), grouping by up/down/swap, and mapping individual code actions.

One challenge is that sometimes several move up/down refactorings may be available for the same position (could be solved by configuring the shortcut to show a menu when there is > 1 action for a position).

[hediet]

have individual code actions

Maybe it makes sense to distinguish them, but I think discoverability is much more important than precision for refactorings.

ReSharper has one unified move down/up system, and it works flawlessly.

Having one unified system where always only one thing is applicable makes it much more predictable. In my opinion, one universal tool provides much more value than many smaller (more precise) tools.

[lgrammel]

I don't understand how this would impact discoverability. There would be a single shortcut for "move up" and a single shortcut for "move down". The additional flexibility is just in case a user wants it, and there have to be separate implementations anyways because there are different matchers, transformations, safety evaluations, etc.

[hediet]

There would be a single shortcut for "move up" and a single shortcut for "move down"

Then you would need to use many context keys to compute upfront which of those "move up" commands wins effectively. Alternatively you would need a single dispatching move-up and move-down command (which would also enable changing the keybinding for all move commands at the same time).

[lgrammel]

There would be a single shortcut for "move up" and a single shortcut for "move down"

Then you would need to use many context keys to compute upfront which of those "move up" commands wins effectively. Alternatively you would need a single dispatching move-up and move-down command (which would also enable changing the keybinding for all move commands at the same time).

Maybe, but it could also depend on the activation ranges etc. I'll probably stick with the current system for now, since I have significant tooling around, and will see how far I get. I can always implement some prioritization algorithm when there is a need for it.

[lgrammel]

Move statements is available in v1.132.0:

Let me know what you think about the current keyboard shortcut setup or if you run into any bugs.

[hediet]

That is very cool! When invoking the shortcut, I wouldn't expect the context menu though.

[lgrammel]

That is very cool! When invoking the shortcut, I wouldn't expect the context menu though.

Yeah - I agree. I'm still trying to figure out how to best deal with the following 2 constraints:

• sometimes there are overlapping move refactorings (e.g. move stmt & move if-else-branch) for the same activation range. Could be solved by
  • prioritization algorithm - but e.g. it is unclear to me how move down branch from an if-statement would work in this case
  • showing the menu only when there are > 1 options (could still lead to friction)
• currently there is no good way of showing safety information without the context menu. Possible solutions:
  • show confirmation dialog if the refactoring would result in an error
  • show notification when a refactoring is directly invoked and there is a info/warning/error safety information

Would love to hear your thoughts, in particular if there are other options that I'm not considering.

[hediet]

In general, I would expect move commands to be non-safe, so safety information is not very helpful for me for this code action.

showing the menu only when there are > 1 options (could still lead to friction)

I would always select the element closer to the cursor. Maybe there could be a second command that shows the context menu. Or even a setting.

[lgrammel]

I think some conflicts will be unavoidable if there is only up/down (e.g. move if branches conflicts with move statement). I'll introduce move left/right in addition to move up/down.

Move up/down

• Move Statements / Declarations
• Move Class Members
• Move JSX elements
• Move Case-Branches in Switch Statements
• TypeScript: move interface members, move type members, re-order union types, etc.

Move left/right

• Move Declarations inside multi-variable declaration
• Move Array Items (taking care of commas)
• Move Object Properties (taking care of commas)
• Move If/Else Branches https://github.com/p42ai/refactor-vscode/issues/28 https://github.com/p42ai/refactor-vscode/issues/41
• Move expression in homogenous condition (a && |b && c -> |b && a && c) (note: overlap/replacement of 'flip operator')
• Move argument in function invocation (f(1, |2) -> f(|2, 1))
• Move Parameters (fixing all call-sites) (note: technically hard - requires multi-file, dataflow, etc.; more limited version possible)
• Move JSX attributes

This should reduce conflicts and still be fairly intuitive. I'll also remove the menu.

[lgrammel]

The keyboard shortcuts directly execute (when there is only 1 option for now) starting with v1.132.2:

[serchserch]

Hi! how can I disable it? ctrl+alt+up/down is a very usefull hotkey to move code lines in any file type but now I'm getting this message when I press these commbination in files different to .ts .js

[lgrammel]

@serchserch you can disable it in the keyboard shortcuts panel when you record the "ctrl+alt+up+ keybinding:

Command Bar: >Preferences: Open Keyboard Shortcuts

Thanks for the report, I'll look into limiting the shortcut to the js/ts/vue suffixes.

[lgrammel]

@serchserch I've released v1.132.3, which limits keyboard shortcuts to supported files. Please let me know in case the JavaScript assistant still provides keyboard shortcuts for unsupported files.

[hediet]

Hi! how can I disable it? ctrl+alt+up/down is a very usefull hotkey to move code lines in any file type but now I'm getting this message when I press these commbination in files different to .ts .js

This is a good point - can you just move lines up/down in case there is no applicable refactoring?

[lgrammel]

Hi! how can I disable it? ctrl+alt+up/down is a very usefull hotkey to move code lines in any file type but now I'm getting this message when I press these commbination in files different to .ts .js

This is a good point - can you just move lines up/down in case there is no applicable refactoring?

Interesting idea - I'll think about it. My concern is that is might be confusing for the users and that they would often have another shortcut for this. I'll wait until the whole move system is further along to see if it would fit in naturally.

[lgrammel]

v1.133.0 supports moving JSX attributes:

[hediet]

I think for Windows, shift+alt+{up, down, left, right} might be better. Ctrl+Alt+... is already used for multi-cursor and is quite important.

[lgrammel]

I think for Windows, shift+alt+{up, down, left, right} might be better. Ctrl+Alt+... is already used for multi-cursor and is quite important.

Thanks for letting me know - definitely need to change this for windows. Unfortunately "shift + alt + {direction}" is already taken as well on windows, and "ctrl + shift" is a standard for extending selections afaik.

Would "win + alt + {direction}" work for you? (I did not find that shortcut in the list)

[hediet]

shift+alt+{up, down, left, right}

I think duplicating lines up and down is probably not used by many, so I think it should be okay to overwrite that.

[lgrammel]

shift+alt+{up, down, left, right}

I think duplicating lines up and down is probably not used by many, so I think it should be okay to overwrite that.

Is there any downside to the win + alt + {direction} option that I'm missing? I'd rather not mess with any built-in shortcuts (even tho I agree not many people are likely to use this one).

[hediet]

win + alt + {direction} is already taken by the OS. I think the "win" modified should be avoided for shortcuts.

[lgrammel]

win + alt + {direction} is already taken by the OS. I think the "win" modified should be avoided for shortcuts.

Hm that makes things more difficult. I wanted to use shift as a modifier for move into/out of operations, e.g. in the following situation:

|statement1();
{
  statement2();
}

Shortcut w/o shift: move statement down

{
  statement2();
}
|statement1();

Shortcut w/ shift: move into block:

{
  |statement1();
  statement2();
}

That way correct "barriers" would exist, but can easily be overcome by adding an additional modifier key.

Current alternatives I'm considering:

• shift+alt+{direction} (for move) and shift+alt+pgdown/up (for move into/out of)
  • disadvantage: not as easy to do move into/out of
  • disadvantage: conflicts with expand/shrink selection & copy lines
• move to a different area of the keyboard instead of the arrows, e.g. ijkl
  • disadvantage: hard to find on keyboard, other overlapping shortcuts

Since the conflict for shift+alt is also on expand/shrink selection, I'm torn between the 2 alternatives.

[lgrammel]

v1.134.0 supports moving array elements:

[lgrammel]

v1.135.0 supports moving object properties:

[lgrammel]

v1.136 supports moving class members:

[hediet]

Nice work!!

Top level declarations would also be super nice, as I often reorder classes to clean-up code.

[lgrammel]

Nice work!!

Top level declarations would also be super nice, as I often reorder classes to clean-up code.

Thanks! Top-level declarations should be moveable in v1.136.1 - are there any cases where it does not work?

[hediet]

Nice, it works perfectly!

[hediet]

Would you be able to track the text ranges before/after the move of the moved element? Maybe we can do a cool animation

[lgrammel]

Would you be able to track the text ranges before/after the move of the moved element? Maybe we can do a cool animation

An animation would be cool - can VS Code do that?

Yes, the edit ranges and the selection ranges are used in the code action already.

[hediet]

can VS Code do that?

Not yet, but this would be a great use-case to expose that functionality to extension authors. There is something internal that could be enhanced a bit to make it work.

[lgrammel]

can VS Code do that?

Not yet, but this would be a great use-case to expose that functionality to extension authors. There is something internal that could be enhanced a bit to make it work.

Would love to try it, for move refactorings this could be very useful to help the user understand what's going on.

Probably the animations would need to be able to turned off for power users (animations can easily slow down the user interactions, I almost always disable them when I can).

[hediet]

Btw. you should reveal the location for the moved source. For large classes the moved text just disappears.

[lgrammel]

Btw. you should reveal the location for the moved source. For large classes the moved text just disappears.

Thanks for letting me know - I just shipped v1.137.0, which reveals the (moved) text selection / cursor and adds "move switch case":

[lgrammel]

v1.138.0 supports moving variable declarations:

[lgrammel]

v1.139.0 supports moving array elements in destructuring expressions:

[hediet]

An animation would be cool - can VS Code do that? Yes, the edit ranges and the selection ranges are used in the code action already.

For now, you could use a decoration to highlight the target location for a short period of time.

[lgrammel]

An animation would be cool - can VS Code do that? Yes, the edit ranges and the selection ranges are used in the code action already.

For now, you could use a decoration to highlight the target location for a short period of time.

I'll hold off on the animation for now - while it might look cool at first glance and could make for good demos, I found that animations in areas of direct user interaction tend to get into the way in practice more often than not (e.g. by slowing the user down). When there is some built-in VSCode API that I can leverage I might look into prototyping / revisiting this.

How are you envisioning the decoration? Would it be shown after the edit is applied, or before (and delay the edit)?

Update: I've started experimenting a bit with this idea and I think I can come up with something that could work.

[hediet]

I would only show the decoration for the location where the code moved to, for about 300ms. Maybe you can even do some tricks with backgroundColor: `red; transition: all 0.5s ease-out, to get a nice fade out, but I don't know.

I don't think this animation will be annoying, it should be very subtle. But it will help to see where the text moved to.

[lgrammel]

I would only show the decoration for the location where the code moved to, for about 300ms. Maybe you can even do some tricks with backgroundColor: `red; transition: all 0.5s ease-out, to get a nice fade out, but I don't know.

I don't think this animation will be annoying, it should be very subtle. But it will help to see where the text moved to.

I agree, that is along the lines of what I'm implementing. If the CSS in the background color does not support animation (docs say only rgba is allowed), I might code the animation in JS.

[lgrammel]

Quick preview:

https://user-images.githubusercontent.com/205036/183908024-1de7f43f-783b-4820-8838-00680f484e4a.mov

This looks nice, will clean it up and include it in the next release.

[lgrammel]

v1.139.1:

Thanks for the suggestion! Animations might have potential for other refactorings such as extract or inline as well.

[lgrammel]

Move object property in destructuring (v1.140.0):

[hediet]

Very nice!

[lgrammel]

I think for Windows, shift+alt+{up, down, left, right} might be better. Ctrl+Alt+... is already used for multi-cursor and is quite important.

v1.141 switches to "shift+alt+direction" on Windows/Linux.

[lgrammel]

v1.142 simplifies the shortcuts:

Action	Type	Mac Shortcut	Windows/Linux Shortcut	Code Assists
Move	context menu	CTRL + CMD + M	CTRL + ALT + M	17 code assists
Move Up	direct	CTRL + ALT + UP	SHIFT + ALT + UP	9 code assists
Move Down	direct	CTRL + ALT + DOWN	SHIFT + ALT + DOWN	9 code assists

The activation depends on the cursor position. Less frequently used move operations that would conflict with others are now availalbe in the move menu (e.g. move if-else branches).

[lgrammel]

Move nested if statements in v1.143:

[lgrammel]

A quick update on why progress has been slow in the last few months: in order to get this right, I need to handle whitespace and comments differently, which requires some larger changes to the refactoring engine. I've put this on a lower priority for now, since many of the core parts already work, but I hope to get back to it in Q1/23.