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

feat(code actions): display code actions within a quick panel #1611

Merged
merged 5 commits into from
Mar 13, 2021
Merged

feat(code actions): display code actions within a quick panel #1611

merged 5 commits into from
Mar 13, 2021

Conversation

bpingris
Copy link
Contributor

Display the code action in a quick panel instead of the context menu, this makes us able to use more keybindings to control which code action we want to use.
With the context menu you're only able to select an item with up/down arrows or the mouse, with a quick panel you can add custom keybindings and also use the search feature of the quick panel.

@rchl
Copy link
Member

rchl commented Mar 11, 2021

  • Code actions can also be triggered from the hover popup and that is not handled in those changes so far.
  • we don't like using abbreviated variable names (win => window)
  • we should probably update naming while making this change. So change show_popup_menu to something more generic like show_code_actions.

@bpingris
Copy link
Contributor Author

Are you referring to that hover popup?

I don't see where the code actions should be displayed in that popup, or maybe I'm looking to the wrong one!

@rchl
Copy link
Member

rchl commented Mar 11, 2021

When you have an error in the code and you hover the error.
Screenshot 2021-03-11 at 11 01 38

@predragnikolic
Copy link
Member

I will try this PR today 🙂

@predragnikolic
Copy link
Member

predragnikolic commented Mar 11, 2021

I tried this and I don't think that the quick panel provides a better user experience than the context menu:

Context menu:
context

Quick Panel:
output

note: how when the quick panel is shown it covered most of my code so I can loose context to where I initially invoked the code action

@rchl
Copy link
Member

rchl commented Mar 11, 2021

note: how when the quick panel is shown it covered most of my code so I can loose context to where I initially invoked the code action

I'm not sure I agree with that because the focus goes back to the cursor and so there shouldn't be any confusion or disorientation IMO.

The fact that you can filter items is also a plus and usability improvement.

I've only used this new way for a minute or so, so far.

@bpingris
Copy link
Contributor Author

Yea I understand the "fear" of losing some context.

I must say in my case it's much more comfortable, even if I was working with a layout more or less like the one you gave in your example (small window size, sidebar opened etc) this wont annoy me as when I open code actions, I know where (in the code) I call it and for what purpose so it wont bother me.

I was also looking in a way to use the completion list (so we have keybindings, consistent styling and no code overflow) but my guess is that its only purpose is: completion? So we can't add a on_done callback when we select an item in it and use the code action's handler.

Copy link
Member

@rwols rwols left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion about this. All in all looks good to me.

@predragnikolic
Copy link
Member

I do like the ability to filter items. (but I still think that visually the context menu is more suited for this kind of stuff)
Maybe I just need more time to get used to it. :)

So lets merge this. We can act later based on the feedback from other people.

@predragnikolic predragnikolic merged commit c504cd5 into sublimelsp:st4000-exploration Mar 13, 2021
@predragnikolic
Copy link
Member

BTW, thanks for the PR :)

@princemaple
Copy link

auto save + auto format on save + LSP switching code action from inline context menu to command palette is not playing well

The problem with this is, I trigger auto import from LSP, it pops up the command palette, which causes loss of focus from the file, it auto saves and triggers auto format, and the auto import becomes invalid (either because of request id or code change, doesn't matter), choosing from the commands ends with up noop. I have to do it again, or save beforehand, which loses the point of auto save

Previously I'd write my code and go through all the unimported names and run auto import on them, then switch to browser or something, which triggers auto saves.
Now I have to go to a name, either save the file first then auto import, or be too quick and auto import directly and realize it's noop and have to do it again, and repeat for every name waiting to be imported.

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.

5 participants