LightBulb menu item sort is inconsistent with lightbulb severity icons

[gundermanc]

Version Used: Visual Studio 15.7 (d15.7stg)

Steps to reproduce: See 'Explanation'

Expected behavior: LightBulb menu items are sorted by severity so that 'error' fixes come before 'suggestions' which come before 'refactorings'. Doing so ensures that the most relevant fix is always at the top of the menu so that it is easily discoverable and so that pressing Ctrl+. followed by enter is more likely to give the user a solution to the most 'problem' on the line.

Actual behavior: Sorting is not relative to severity but rather seems to be relative to distance from the caret (in margin lightbulb).

Explanation: For 15.7 we completed experience 402316, adding new LightBulb icons to the VS lightbulb control. When the IDE asks the language service for actions, the language service has an opportunity to give back a set of action categories for which there are actions for the span under consideration. The built in categories used by Roslyn are error fix, code fix, and refactoring, and they have severity levels such that error fix > code fix > refactoring. When the lightbulb icon is rendered, the IDE chooses the icon that maps to the highest severity category for which there are known to be actions.

e.g.: If there are known to be code fixes (suggestions) and refactorings available, we display the yellow lightbulb. If there are only refactorings, we show the screwdriver. If there are errors, and lightbulbs, and refactorings, we display the 'red error bulb'.

Margin based scenario:

In one margin-based scenario, the user has their caret in the middle of 'this', which is set within their .editorconfig file to be a 'suggestion'. In the margin, we have an error bulb pop-up, indicating that there is a fix for the error on the line. However, when I invoke lightbulb by clicking on the margin control or by pressing Ctrl+., the top entry is for fixing the 'suggestion', and the actual error fix 'using System.Collections.Immutable' is the second highest entry.

In my mind, this feels misleading because the lightbulb control's icon suggests that we're offering up a fix to the error, but we have to dig a bit to find it. Before the severity icons there was a reason for this behavior. The assumption was that as a user types and their caret position changes, we'd provide them actions that are most relevant to their particular context. e.g.: if you caret is within the 'this', you probably want fixes for that exactly location. Now that we have the severity icons, however, it feels like we need to make one of the following changes:

1) In the margin-based lightbulb case, the span under consideration is usually the extent of the entire line, such that any suggestion in the line is surfaced in the lightbulb menu and reflected in the icon. We should change this to only reflect actions applicable to the current caret location, such that in the case listed above, we expose only the fix for the style warning and hide the error fixes altogether.

OR

2) We continue considering the entire line extent for applicable lightbulb actions on Ctrl+., but we use severity as a primary sort mechanism for actions, regardless of caret position, so that error fixes for an error anywhere on the line always come first, suggestions come next, and refactorings come last.

OR

3) We add visual treatment to the errors in the actions list to aid the user in quickly selecting error/suggestion fixes when they're lower on the list. This could be as simple as adding the error icon to the first item in the error actions set (the set of items just after the separator in the screenshot) and indenting all of the rest of the items to visually indicate their membership as part of the group.

Consider error squiggles:

Though it's possible to have multiple error squiggles in one location, the IDE has logic such that overlapping or intersecting squiggles are combined into a single squiggle that receives the styling of the highest severity error. e.g.: if a warning and an error overlap by more than zero characters, the green squiggle is dropped, and a single red squiggle is displayed.

Also consider the typical developer's inner loop:

• Create branch
• Write code
• Fix compiler errors (apply error lightbulb actions)
• Repeat until it runs
• ...
• Refactor as needed
• Address style (apply suggestion lightbulb actions)

With this in mind, I'd recommend taking the second proposed change because a user is unlikely to care about style fixes until the code compiles and runs in much the same way they won't care about warnings until the code compiles.

[CyrusNajmabadi]

Expected behavior: LightBulb menu items are sorted by severity so that 'error' fixes come before 'suggestions' which come before 'refactorings'. Doing so ensures that the most relevant fix is always at the top of the menu so that it is easily discoverable and so that pressing Ctrl+. followed by enter is more likely to give the user a solution to the most 'problem' on the line.

Actual behavior: Sorting is not relative to severity but rather seems to be relative to distance from the caret (in margin lightbulb).

I disagree with the expected behaviors.

If i put my cursor on some indicator int he code, and i bring up the lightbulb, then i want the fixes for the thing i'm on :)

[CyrusNajmabadi]

We add visual treatment to the errors in the actions list to aid the user in quickly selecting error/suggestion fixes when they're lower on the list. This could be as simple as adding the error icon to the first item in the error actions set (the set of items just after the separator in the screenshot) and indenting all of the rest of the items to visually indicate their membership as part of the group.

I would be ok with this.

[CyrusNajmabadi]

In my mind, this feels misleading because the lightbulb control's icon suggests that we're offering up a fix to the error,

This def feels like an issue with the lightbulb (and it's line-based nature). If put my caret on the faded code, and i bring up the lightbulb, then i'll want to be addressing the thing that is under my caret.

[CyrusNajmabadi]

I do not agree with your assessment of the "typical developer inner loop". i.e.

Also consider the typical developer's inner loop: Create branch Write code Fix compiler errors (apply error lightbulb actions) Repeat until it runs ... Refactor as needed Address style (apply suggestion lightbulb actions)

The point of the inner loop is that all of these things are happening. You've now defined "two loops":

1. get it running
2. get the style right

But hte point of the inner loop is that it's just one loop. I may be making style changes as i also deal with compiler issues. Indeed, that's how i normally operate 100% of the time :)

[gundermanc]

But hte point of the inner loop is that it's just one loop. I may be making style changes as I also deal with compiler issues. Indeed, that's how i normally operate 100% of the time :)

I usually try and fix compiler errors on the line before addressing style on that line. Maybe the typical loop is to actually fix errors on a line, fix style on a line, rinse and repeat. Either way, I'd expect fixing the errors in a particular context (maybe the line) to usually take precedence over what I'd consider to be elective actions (suggestions, refactors).

You make a great point that I am just one dev and not the average, so this is something that would be best to determine via concrete data instead of speculation :)

It seems like there is definitely room to improve how intuitive the fixes are for the average dev. A few possibilities come to mind. cc: @kuhlenh

• Experiment and see if ordering by severity increases user engagement with lightbulb. (If my theory about the inner dev loop is correct, I'd imagine that we'd see a rise in usage of error fixes with little to no drop in usage of suggestions and refactors. A drop in usage of suggestions and refactors would suggest that users don't usually fix errors before taking elective actions, thus, this would be a disruptive change.)
• Make it configurable so that a user can choose their development style (requires a tools/option, not very discoverable, what should be the default? can this option be exposed directly on the lightbulb for better discoverability?)
• Offer multiple lightbulb gestures, one for the entire line and one for the caret location (Same problem with discoverability. More gestures = more to remember. Increases risk that user will discover only one of the gestures, leading to a reduction in perceived value of the feature).
• Tweak the icons to only show fixes for the highest severity action under the caret instead of the entire line (addresses the issue but IMHO reduces the discoverability of higher severity fixes such as error fixes). 0
• Split lightbulb into two or more per-category gestures. e.g.: one category for fixes and one for elective actions (suggestions + refactors). (Same trade-offs as above).

[gundermanc]

If nothing else, adding visual treatment to the lightbulb menu for different categories would be a good visual aid.

[gundermanc]

Another great example of this issue:

Lightbulb pops up indicating existence of a suggestion but oddly a refactoring action 'Change signature' is the top item.

[justcla]

Sorting gets tricky when we consider ranking items closer to the caret/mouse point.

However, as I understood it, there was a different context menu that would surface when the lightbulb was triggered from a specific point in the line - and that one would favor the item in context. But there is the general "All issues on the line" lightbulb (shown in the margin).

Is this what's causing issues with the sort order? Because the simple fact is, it's strange to see a tool ranked higher than a fix for an error or a warning.

[CyrusNajmabadi]

Because the simple fact is, it's strange to see a tool ranked higher than a fix for an error or a warning.

Can you clarify this. I do not see an error/warning in your code. Is there one? In the case of no error/warning, IMO both features are effectively 'tools'.

However, as I understood it, there was a different context menu that would surface when the lightbulb was triggered from a specific point in the line

The problem is that ctrl-dot was hijacked. It used to mean 'give me the options where i am'. Now it's the 'give me the options for the line'. As a user who trained myself to use ctrl-dot for 5+ years, i have completely internalized the former meaning. And i don't really want to have to use another keystroke (nor do i want to have to configure my ide to get the old meaning :))

Is this what's causing issues with the sort order?

The issue is that, today, we have a very blunt mechanism for determining order. Just a general "is this provider more important than this other provider". That is too coarse, and fails to codify subtleties like:

1. Is the user actually being presented with an adornment in the UI (i.e. suggestion dots, squiggles, etc.).
2. Has hte user actually selected code or not.
3. How is the user bringing up the lightbulb (mouse, or keyboard).
4. Where is the user on the line.

And, finally, the worst subtlety of all:

1. Should we reorder things with some sort of dynamic system to surface better choices to the user?
2. Or should we attempt to keep things in a fixed order so that people are not confused why the list is differently ordered every time they invoke it depending on factors they don't understand.

--

i.e.: do you want your context menu's item's constantly changing order due to where you are in code? There's an arugment for: "yes. it allows me to get to the relevant stuff near the top". but htere's a strong argument for "no. hte lack of consistency is enormously confusing, and breaks things like muscle memory."

[CyrusNajmabadi]

Also, another case we need in the list (which i talked about in: https://github.com/dotnet/roslyn/issues/24998#issuecomment-395934491). We often want to deprioritize an item the more unexpected the change may be. For example, people often expect a fix to just make a single small change to their code. When a fix goes and does more than that, we have found htat having it higher up in the list leads to people being more confused and upset about the change that happened. That's because they didn't realize that lots of changes were going to happen, and so deprioritization has made them have to take more steps to invoke the action, which in turn has made htem pay attention more and realize something 'bigger' is going to happen. This often will then let them realize htat perhaps they didn't want to take such a large step.

Two examples of this are:

1. install nuget-package. This is heavily invasive action that goes and changes your project. Such a thing should not be invoked haphazardly. By deprioritizing (and giving specialized icons), people can really see that this is not just your normal text-changing code action.
2. spell-check-and-add-using. While this feature is what people want a bunch of the time. It also comes up a lot when people are just creating new things that have names similar to those found in other namespaces (which happens more often than you think :)). It's def good to bring in this fix. But we found a lot of people get confused and upset when they expected to generate code, but instead pulled in some random namespace they never cared about. Because the import-adding normally happens outside of the text view they're in, they don't realize that happened, and they are confused when they discover this was hte fix. By deprioritizing, we make things more like '1', and users normally have to go explicitly take this action, and they better realize what's going on.

--

Basically, ordering is also good for making people put more thought into the action they're aobut to take, which better makes them understand what's going on, and which highly lowers confusion.

--

This, along with the things i mentioned in the previous post are all very subtle parts of what goes into lightbulbs, and are why there isn't just a 'simple fix' to make lightbulbs great and understandable for all users. :)

[gundermanc]

Also, another case we need in the list (which i talked about in: #24998 (comment)). We often want to deprioritize an item the more unexpected the change may be. For example, people often expect a fix to just make a single small change to their code. When a fix goes and does more than that, we have found htat having it higher up in the list leads to people being more confused and upset about the change that happened.

This was the spirit behind my suggestion for a refactoring category The idea was that these are actions that are impactful, elective in nature, and aren't minimal fixes. I'm not 100% confident that 'refactoring' is the right name/categorization but I do like the idea of presenting them lower in the list and with a different icon (screwdriver rather than error bulb).

spell-check-and-add-using. While this feature is what people want a bunch of the time. It also comes up a lot when people are just creating new things that have names similar to those found in other namespaces (which happens more often than you think :)). It's def good to bring in this fix. But we found a lot of people get confused and upset when they expected to generate code, but instead pulled in some random namespace they never cared about. Because the import-adding normally happens outside of the text view they're in, they don't realize that happened, and they are confused when they discover this was hte fix. By deprioritizing, we make things more like '1', and users normally have to go explicitly take this action, and they better realize what's going on.

This one seems less clear to me. I'd always consider adding a using to be less impactful or out of the ordinary than generating types/methods etc. Was the negative feedback given from external users and do we have an idea of what portion of the population that they represented?

I understand I'm speculating with this proposal but it seems to me that if we accept that actions have some inherent precedence, and that this precedence should be exposed via the lightbulb control, then the menu should really show the highest precedence actions first. If showing 'high precedence' items first isn't a change that improves the experience, then we could make the case that 1) our current precedence for the items is incorrect or 2) the items have no natural precedence and we shouldn't be changing the lightbulb icons or 3) the user doesn't care about the action's precedence and it should not be exposed.

[CyrusNajmabadi]

This was the spirit behind my suggestion for a refactoring category The idea was that these are actions that are impactful, elective in nature, and aren't minimal fixes. I'm not 100% confident that 'refactoring' is the right name/categorization but I do like the idea of presenting them lower in the list and with a different icon (screwdriver rather than error bulb).

Note: i wasn't against this. I just couldn't come up with a rational set of rules about what went in this category :)

"Aren't minimal fix" is realllllllly hard to define. Indeed, i woudl want a breakdown of all roslyn analyzers, and something explaining what is miminal and what isn't :)

[CyrusNajmabadi]

This one seems less clear to me. I'd always consider adding a using to be less impactful or out of the ordinary than generating types/methods etc. Was the negative feedback given from external users and do we have an idea of what portion of the population that they represented?

It was external and internal. Here's the explanation (as best as i can make it). When making a change, people often have an expectation of what the change will do, and what 'scope/area/region' of the code that will be affected. As i mentioned in my post, having the usings change here was definitely not what people were expecting.

One thing we've been noodling on is actually using the 'namespace' glyph here to help call out that this is special (and because a 'namespace' is going to be imported). By having there be a glyph here, we hope to have hte same effect that the nuget-glyph seems to have had. Namely that people seem to take a little more time to think about what it's going to do and to decide that 'yes, it is what i want'.

[justcla]

Minimal fix, complex fix - these are very subjective. What you consider large, I might consider small. There's no clear way to make such a distinction.

It's either an option that will potentially fix an issue, or it WILL NOT fix an issue. If it will not fix an issue it's a tool (or setting). If it will fix an issue, it should have a lightbulb.

[justcla]

I'd always consider adding a using to be less impactful or out of the ordinary than generating types/methods etc.

Not if importing the using will fix the issue. If there's an import for a specific class name I've typed, I probably want that one. When I create my own classes, the names tend to be unique. Not always - but most the time. So a rule that says "If there is a matching class that can be imported, use it" will probably return the desired result most time.

Importing usings is one of the tools I use most. I'm glad it's at the top when I want it

[CyrusNajmabadi]

If there's an import for a specific class name I've typed, I probably want that one.

This is not necessarily the case at all. The space of used type-names is very large. I'm actually consistently surprised by how often i type a name that is provided elsewhere, when i'm wanting to generate my own type :)

[CyrusNajmabadi]

Closing out as nothing has changed in the design here, and there are no plans to do that.