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

Annoying "lightbulb" whenever placing cursor into constant js/ts expression #33241

Closed
weinand opened this issue Aug 28, 2017 · 10 comments
Closed
Assignees
Labels
editor-code-actions Editor inplace actions (Ctrl + .)
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Aug 28, 2017

  • VSCode Version: Insiders
  • OS Version: macOS

Steps:

  • open js or ts file
  • place cursor in any constant expression (e.g. in the "1.16.6" below)

Observe: a "lightbulb" appears:
2017-08-27_23-22-57

IMO this is an idiotic suggestion and I find this quite annoying.

Do we have telemetry info that shows that users are actually using this?

@mjbvz
Copy link
Contributor

mjbvz commented Aug 28, 2017

@weinand This was just added by TS 2.5. Showing extract method in this location looks like a TS bug to me, but I also think we need to think about what we want the UX to be in other case. I'd argue that we should only show the lightbulb when the user has made a text selection. The CodeActionProvider does not currently support controlling whether or not the light bulb is shown. This may require an API change similar to what I've proposed in another issue: #33049 (comment)

@mjbvz mjbvz added javascript JavaScript support issues typescript Typescript support issues labels Aug 28, 2017
@weinand
Copy link
Contributor Author

weinand commented Aug 30, 2017

See #33282

@jrieken jrieken added editor-code-actions Editor inplace actions (Ctrl + .) and removed javascript JavaScript support issues typescript Typescript support issues labels Aug 30, 2017
@jrieken jrieken self-assigned this Aug 30, 2017
@jrieken jrieken added this to the August 2017 milestone Aug 30, 2017
@jrieken
Copy link
Member

jrieken commented Aug 30, 2017

This is a general issue with the light bulb and how we check if it should be shown. There two things;

  1. The "Quick Fix"-command which is bound to Ctrl/Cmd+.
  2. The lightbulb that is trying to sell this feature

To show the light bulb we monitor what is happening in the editor, ask silently for code actions, and iff there are any then we show the light bulb. The monitor-logic today is the following: Check to show lightbulb when the cursor is on a diagnostic, (2) check to show lightbulb when we have a non-empty selection on a "word". The latter is what is annoying and the plan is to change it to check to show lightbulb only when there is non-empty selection.

@rcjsuen
Copy link
Contributor

rcjsuen commented Aug 30, 2017

In Eclipse the light bulb decoration is always there regardless of where the text cursor is. As a result, I was initially very surprised to see that the light bulb only appeared in VS Code when the text cursor entered the range the area that the diagnostic underlined.

I feel that it will become very difficult for new users to realize that quick fixes are available as users will now be forced to make a text selection where the diagnostic is to get the light bulb to appear. The fact that you currently have to place your cursor where the diagnostic is to "check for the existence of quick fixes" is already non-obvious to users in my opinion.

Whereas in Eclipse I can immediately tell visually if quick fixes are available, in VS Code I have to click on every diagnostic to verify this. With the suggested upcoming new behaviour, I will have to double-click every diagnostic to select a word or click inside the diagnostic and then hit Ctrl+. to test if a quick fix is available.

@Gama11
Copy link
Contributor

Gama11 commented Aug 30, 2017

I'm also not excited about code actions becoming less discoverable by requiring a selection - the easy discoverability is what makes code actions so useful to begin with. IntelliJ also doesn't require a selection, and for some code actions, requiring a selection really wouldn't make much sense:

There is one thing that annoys me about some code actions that are almost always available, which is them getting in the way of breakpoints (#25174). This can be solved differently though, IntelliJ solves this by showing the light bulb in the editor area, not the gutter:

@jrieken
Copy link
Member

jrieken commented Aug 30, 2017

@rcjsuen and @Gama11 - Thanks for the input! Let me try to slice and dice this into pieces

In Eclipse the light bulb decoration is always there regardless of where the text cursor is. As a result, I was initially very surprised to see that the light bulb only appeared in VS Code when the text cursor entered the range the area that the diagnostic underlined.

Agreed. That is useful and something we will look into for #25149

I feel that it will become very difficult for new users to realize that quick fixes are available as users will now be forced to make a text selection where the diagnostic is to get the light bulb to appear

Nothing will change there: when you are inside an error/warning having an empty selection is enough. Again, we can improve this by showing the lightbulb for errors/warnings more often, e.g. when you are on the line.

This is about getting a lightbulb while you type and, worst, the lightbulb preventing you from setting breakpoints. @Gama11 presents a nice sample for having a lightbulb even for empty selections and while I agree on the usefulness, the lightbulb/breakpoint conflict still exists.

To put this in perspective, this is a "tactical" fix for the August release. We want to ship with TypeScript refactorings but we don't want to steel the ability to set breakpoints. So, while we work on #25174 we will have the slightly changed lightbulb behaviour - not because that's the right thing but because it is less bad.

This is how it looks/behaves with my changes:

aug-30-2017 15-28-19

@Gama11
Copy link
Contributor

Gama11 commented Aug 30, 2017

We want to ship with TypeScript refactorings but we don't want to steel the ability to set breakpoints.

That's fine and all, but this affects more than just TypeScript. I can't say I like the idea of a temporary (as it seems to be), "tactical fix" (which is really just an embellishment for "hack") for TypeScript affecting all other language servers with code actions.

In the Haxe extension, we've had an "Extract Variable" code action for a while, but one of the reasons we have decided not to include it in releases yet because of this exact issue (it getting it in the way).

It might necessary for code actions to have flag that determines whether they are "selection-based" - some, like "Extract Method" or "Extract Variable" probably don't make sense without a selection. Which raises the question whether they should be code actions in the first place - the other alternative that I can see is to not implement these as code actions, but commands. This is what IntelliJ does - it doesn't implement these as code actions:

The more I think about it, the more I think that's the right approach, because code actions are usually "quick fixes" where the language server can easily tell that something could be improved a bit (remove unused code, replace anonymous function with short lambda, etc). They don't require input from the user, like a selection.

Refactorings like extracting methods or variables are something that require more "intent" by the user. They're usually something you specifically request the editor to do, because they're too complex for the editor to come up with good suggestions. With code actions, it's the other way around: you "stumble" over them and (hopefully) think "good point editor, let's make that a bit nicer".

@mjbvz
Copy link
Contributor

mjbvz commented Aug 31, 2017

Thanks @jrieken! With the workaround, should we keep this bug open? In my testing, the original issue seems mitigated


From this discussion, I that our refactoring story needs some more thought. I have opened #33555 to track issues / missing features with our current refactoring support and linked back to the comments here from that issue. Feel free to add your thoughts to the new issue as well

@jrieken
Copy link
Member

jrieken commented Aug 31, 2017

Yeah, I'll close this and we will continue the conceptual work in #33555 and the UX work will happen in #25174

@jrieken jrieken closed this as completed Aug 31, 2017
@jrieken jrieken modified the milestones: August 2017, September 2017 Aug 31, 2017
@jrieken
Copy link
Member

jrieken commented Aug 31, 2017

That also means that we will restore the lightbulb with empty selection when having a less annoying UX

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-code-actions Editor inplace actions (Ctrl + .)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants