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 #8054

Merged
merged 9 commits into from
Mar 22, 2021
Merged

Item movers #8054

merged 9 commits into from
Mar 22, 2021

Conversation

ivan770
Copy link
Contributor

@ivan770 ivan770 commented Mar 16, 2021

Closes #6823

moveitem.mp4

Implementation issues:

  • Most of items are non-movable, since movability of any item has to be determined manually. Common ones are movable though
  • Cursor should move with the item

crates/ide/src/move_item.rs Outdated Show resolved Hide resolved
@lnicola
Copy link
Member

lnicola commented Mar 16, 2021

changelog feature add "Move item up/down" commands (LSP extension)

@ivan770
Copy link
Contributor Author

ivan770 commented Mar 19, 2021

Demo with improved cursor movement and ArgList mover

demoupd.mp4

Adding similar movers should be trivial

crates/ide/src/move_item.rs Outdated Show resolved Hide resolved
@matklad
Copy link
Member

matklad commented Mar 22, 2021

bors r

This actually is very reasonable, sorry for not getting to review this earlier, this could have landed in today's release.

@bors
Copy link
Contributor

bors bot commented Mar 22, 2021

@bors bors bot merged commit d4fa672 into rust-lang:master Mar 22, 2021
@ivan770 ivan770 deleted the item-movers branch March 22, 2021 13:20
@puremourning
Copy link

What was the rationale for this being a custom message instead of code action?

@ivan770
Copy link
Contributor Author

ivan770 commented Mar 22, 2021

What was the rationale for this being a custom message instead of code action?

Is it possible to provide a direction in which we are moving an item via code action?

@puremourning
Copy link

Have one action for up and one for down?

@matklad
Copy link
Member

matklad commented Mar 22, 2021

I don't think code action UI is right here: code actions should be available in narrow scope, while this is an always available thing. In IntelliJ, item movers are also a separate thing with dedicated shortcuts.

Using code-actions API for this might be interesting (ie, the editor still doesn't show these in code action list, and binds them to dedicated shortcuts, but internally it issues code action requests), but I am not entirely sure if that would be a good move. Sticking to LSP's "one request for one UI concern" rule of thumb is more compatible with potential future extensions (multible cursors, left-right movers, etc).

@lnicola
Copy link
Member

lnicola commented Mar 29, 2021

1

2

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.

Port item up down movers from IntelliJ
4 participants