p42ai / js-assistant

120+ refactorings and code-assists for Visual Studio Code
https://marketplace.visualstudio.com/items?itemName=p42ai.refactor
MIT License
125 stars 8 forks source link

Swapping if/else does not work #48

Closed hediet closed 2 years ago

hediet commented 2 years ago

I thought this used to work:

recording

But it does not swap the branches.

If this is a who-wins thing, I would definitely expect the if branch to move when the cursor is on the if or else.

lgrammel commented 2 years ago

Yes, it used to. However, I found it hard to remember when to use left/right vs up/down, and there were conflicts with existing shortcuts (expand/shrink selection). Therefore I've changed most move actions to up/down and removed left/right. In cases where there is a conflict (here: move statement vs move if branch), the less common action (here: move if branch) is only accessible through the move context menu (or the refactor context menu). You can access it with Ctrl+Alt+m on Windows.

It seems you have custom shortcuts set up tho? Maybe there is a good way to provide code action kinds that would work for such custom setups 🤔

hediet commented 2 years ago

It seems you have custom shortcuts set up tho? Maybe there is a good way to provide code action kinds that would work for such custom setups 🤔

Just removed them.

Therefore I've changed most move actions to up/down and removed left/right.

I like that!

In cases where there is a conflict (here: move statement vs move if branch), the less common action (here: move if branch) is only accessible through the move context menu (or the refactor context menu). You can access it with Ctrl+Alt+m on Windows.

I would use anchors to distinguish between these cases. E.g. when the cursor is no the else or if, it should swap branches. Otherwise the statement.

lgrammel commented 2 years ago

I would use anchors to distinguish between these cases. E.g. when the cursor is no the else or if, it should swap branches. Otherwise the statement.

I was thinking the same and first prototyped enabling move up/down from the else keyword. That leads to issues with the inverse action, so really the branch switch also needs to be activated on the if as well. However, this conflicts with move statement, because you can move all other statements at the beginning of the statement. I'm sure there are ways around it with different activation ranges (i.e. cursor positions / selections), but they are not necessarily obvious or easy to learn imo.

My current take is that learnability is one of the main challenges with the JavaScript Assistant, and I want to make it easier by having fairly simple systems for common cases (move up/down) and then context menus for different intents (e.g. move, extract, convert) with more specialized actions. While different activation ranges are part of it, I try not to make them too complex - otherwise it gets hard to find/activate the right code assist.

I'm open to changing this of course if I get more feedback or additional functionality like move in / move out requires it.

hediet commented 2 years ago

Fair points. What about using the if/else-if condition as anchor?

lgrammel commented 2 years ago

That could work - re-opening the issue. The one conflict I could see is with Move expression in homogenous condition (a && |b && c -> |b && a && c) (note: overlap/replacement of 'flip operator'), so I'll wait until that is available and then I'll experiment to see how activating on the condition would work.

hediet commented 2 years ago

Hmm, it is a bummer that this still does not work:

Code_-_Insiders_hcGgnz5lPY

I would say reordering is the most valuable feature that p42 offers, but it often does not work :(

So I have to do the reordering of the if branches manually again...

lgrammel commented 2 years ago

Hey thanks for bringing this to my attention. The functionality is there, but it is currently hidden in the refactor menu. I'll look into a better key mapping this week, now that things have settled a bit around the shortcuts and the code action menu.

My current thinking on the anchors:

lgrammel commented 2 years ago

Check out v1.157.1 - the move if-else up/down can now be triggered on the condition. Outside of the condition, a regular move statement is triggered (to enable both operations to be easily accessible).