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

move items 2.0: move {left,right} or {before,after} and {add, remove} #8340

Open
JakubKoralewski opened this issue Apr 5, 2021 · 3 comments
Labels
A-assists fun A technically challenging issue with high impact S-actionable Someone could pick this issue up and work on it right now

Comments

@JakubKoralewski
Copy link

JakubKoralewski commented Apr 5, 2021

The item movers feature (#8054, #6823) is really cool. I'd like for it to be explored further. I have some ideas. Let me know if I should create separate issue for each, but I fear this isn't something you want to explore. Here are the ideas:

  1. Idea#1: Left & Right

    My initial idea was to also enable moving left and right. This would allow moving left and right the following:

    • function arguments,
      1. updates definition from fn x(z, y) to fn x(y,z)
      2. updates all callers from x(z,y) to x(y,z)
    • generic arguments,
    • tuple item positions. This would , tuple instantiation to flip/swap the positions of certain arguments. These move item left and move item right commands would have the same UI "API" for the user as currently move item up and move item down do now.
  2. Idea#2 Add & Remove

    New commands: add item and remove item. Remove item is pretty simple as it removes the given provided function arguments, tuple items, struct generics, struct lifetimes etc. Add item is more tricky as it wouldn't make much sense in the context of a tuple or function, but it would be pretty nice for struct generics.

    Add item could work for (including but not limited to):

    • for tuple items

      Very simply just adds the provided type to the end, left and right could be used by the user after.

    • for a function

      It would first add an argument with the type provided by the user to the end, and then the user could move it left and right to their liking to place it where they want

    • type bounds requirements on the struct definition for the given arguments and update all the impls to also include the new bound. Same goes for remove item in this context.

      For example, given struct SomeStruct<T: Sized> with your cursor anywhere in T: Sized is you could hit the Add item command, popup appears and you type in Debug as the other one, and the following happens:

      1. original struct turns into: struct SomeStruct<T: Sized + Debug>
      2. all impls now are impl<T: Sized + Debug> for SomeStruct {..}
    • struct generic arguments/parameters

      For example for a struct SomeStruct<T:Sized> with you cursour anywhere in the SomeStruct adding another item would add another type like struct SomeStruct<T: Sized, U: ProvidedByUser>. With ProvidedByUser being inputed with some sort of popup.

  3. Idea#3 Before & after

    This idea is to replace {up,down,left,right} with a more general solution: before and after. Moving a function "before" would have the same semantics as moving it up currently; conversely for down and "before". Moving an argument "before" would move it as Idea#1's left; conversely for Idea#1's right.

Ideally I would love for the combination of Idea#2 and Idea#3. Also, given Idea#2 and Idea#3 one could have the basics for a future Change signature type of refactor where the user could get a nice UI for modifying the given signature to the new one, as all signature refactoring is moving, renaming, adding and removing (correct me if I'm wrong); but given Idea#2 and Idea#3 the user could in theory do the whole change signature action manually by calling the necessary commands themselves. Another nice addition would be #2178

@matklad
Copy link
Member

matklad commented Apr 5, 2021

Yeah, we should have horizontal movers. I think learning how IntelliJ UX works for those would be useful.

For remove item, we already have https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ide_assists/src/handlers/remove_unused_param.rs, and that should be extended to cover more things, like stuct fields, etc. We probably should change the "is unused" check, sometimes it's useful to remove a used thing and do fixups later.

Adding items is tricky. I think a nice UX should be possible here, but I expect this to be a "researchy" problem.

@JakubKoralewski
Copy link
Author

JakubKoralewski commented Apr 5, 2021

Yeah, we should have horizontal movers. I think learning how IntelliJ UX works for those would be useful.

I think IntelliJ does not implement horizontal movers as they have a much more complex visual refactoring tool (that I mentioned could be implemented by composing the various move, rename, add, delete commands):

obraz
It includes:

  • visibility modifier
  • name modifier
  • return type modifier
  • for each argument:
    • type modification
    • name modification
  • moving arguments up and down
  • removing arguments
  • deleting arguments

I actually think that a cursor-contextual "move before" and "move after" commands bound to keybinds could be more powerful than what a visual interface like IntelliJ's could provide. My thinking why is because it's (at least perceived to be) faster to press some keyboard shortcuts instead of having to open up a UI and point and click.
The preview is nice though.

@lnicola lnicola added A-assists fun A technically challenging issue with high impact S-actionable Someone could pick this issue up and work on it right now labels Apr 8, 2021
@ivan770
Copy link
Contributor

ivan770 commented Apr 13, 2021

Remove item seems like a nice idea. However, I think it should be implemented as group of separate assists for each kind of item we should remove (remove_param, remove_tuple_item, etc.) as creating a separate command for something, that works in a specific context feels just like 💡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-assists fun A technically challenging issue with high impact S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

No branches or pull requests

4 participants