dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.02k stars 4.03k forks source link

Roslyn lightbulb actions should have a distinct 'code generating fix' action category #24998

Closed gundermanc closed 11 months ago

gundermanc commented 6 years ago

Version Used: Visual Studio 15.7stg

Explanation: This bug is a companion work item that in conjunction with #24996 should improve the sorting + icons for LightBulb actions.

A subset of the existing lightbulb fixes are currently categorized as ‘errorfixes’, and though they do in-fact fix compiler errors, I think that they rarely fix the user’s omissions or mistakes and are more of a means of development by ‘code generation'. Take for example, the following issue:

image

In this scenario, I mistyped 'ITextBuffer'. Immediately, the IDE suggests that I rename all occurrences of this type to 'ITetBuffer'. #24996 mentioned above accounts for this suboptimal sort order by always prioritizing lightbulb actions by severity. Once we have accounted for sort order, however, 'Generate type' is still above 'spell check' in the fixes list. Though 'Generate type' will address the compiler error, it's not really fixing the error, it's generating a type to enable the developer to continue fleshing out a work-in-progress.

It is my feeling that such a fix isn't a fix at all (fix as it is used here meaning something that corrects a deficiency in the code). Rather, it is a means of using lightbulb to perform user-driven code generation, similar to refactorings.

To this end, I think we can drastically improve the user experience of Roslyn lightbulb actions by splitting the error fixes into two separate categories:

The general rule is that any fix that corrects compiler errors by refactoring existing types, classes, etc. or that requires subsequent actions by the developer, should fall into this category.

The new prioritization of lightbulb actions should be:

errorfix > codegenerationfixes > suggestion > refactoring

Code generation/refactoring actions should be mapped to the screwdriver icon to indicate that they don't actually 'fix' the problem the developer is facing, and they should appear in a distinct section in the lightbulb menu below that of the errors.

sharwell commented 6 years ago

❓ When you set up this example, which of the following did you do?

  1. Type ITetBuffer
  2. Type ITextBuffer, and then change it to ITetBuffer

For the second case, the change to existing code is detected as an intentional rename operation which is why Rename is offered at the top of the suggestions menu.

gundermanc commented 6 years ago

I don't remember the exact sequence of events, I may have changed it after the fact.

Either way, I still think that we should distinguish code generating fixes from the trivial error fixes. In this case, 'spell check' seems to be a more likely solution to the problem than 'generate type'.

CyrusNajmabadi commented 6 years ago

What is the difference between the code-generation that happens when you add a using, vs the code-generation that happens when you generate somethign else? :)

Can you break down the set of features that would go in the "Error fixes: trivial fixes, such as add using, spell check," category? In other words, what else goes there other than add-using and 'spell-check'? I ask because it will help me get a mental model of what that group represents versus the other group.

Thanks!

gundermanc commented 6 years ago

What is the difference between the code-generation that happens when you add a using, vs the code-generation that happens when you generate somethign else? :)

Fair, I've been struggling to determine what the distinguishing trait should be. It's an issue of 'I know them when I see them' :)

Here's what I'm thinking:

I admittedly haven't found too many examples of these actions and the categorization is broad enough that perhaps they should just be considered to be refactors.

The main point I was trying to capture is that these actions do not usually 'fix' the error but rather attempt to change the code base to compile with the error.

CyrusNajmabadi commented 6 years ago

What means "GenerateType" sufficiently "sweeping" vs GenerateLocal? Where does GenerateMethod sit?

What is a "sweeping refactor[ing]" :) ?

gundermanc commented 6 years ago

What means "GenerateType" sufficiently "sweeping" vs GenerateLocal? 'GenerateType' doesn't fit in 'error fix' under my proposed change not because it is a sweeping refactoring but because it breaks the second and third condition of 'error fix' membership:

Where does GenerateMethod sit?

Depends on what GenerateMethod does. If it generates runnable code in the common case and fixes the compiler error, it should be part of the 'error fixes' category. If it's generating a method that throws NotImplementedException(), it should be in the new codegen/refactor category, since it requires additional steps to make the code runnable. It is therefore, not a fix for a mistake or omission but rather an intentional user action that facilitates 'development by refactoring/codegen'.

The intent of this issue is to suggest that the most-likely trivial fix be presented first in the list with the error icon and the less-likely 'code generating / refactoring' actions should appear lower in the list.

CyrusNajmabadi commented 6 years ago

The intent of this issue is to suggest that the most-likely trivial fix be presented first in the list with the error icon and the less-likely 'code generating / refactoring' actions should appear lower in the list.

I think this is making a big assumption. For example, it's making the assumption that the user most likely wants to perform the most trivial fix. That's not at all clear to me that that's what they will want.

CyrusNajmabadi commented 6 years ago

It doesn't return code to a runnable state

Sure it does. :)

And, for the EnC developer, it fits into the loop of: generate code, f5, edit when exception is hit, rinse/repeat.

It doesn't address a minor mistake or omission

That's circular :) What makes it minor?

(sorry, i'm being pedantic because i think it will help flesh out categories here).

gundermanc commented 6 years ago

Pretty good example of the sorts of behavior I'm trying to work around:

image

Generate 'TestViewHelper' is being offered instead of spell correcting and adding the using for 'TextViewHelper'

CyrusNajmabadi commented 6 years ago

Generate 'TestViewHelper' is being offered instead of spell correcting and adding the using for 'TextViewHelper'

This is very intentional behavior. Spell-checking and adding a using is a very invasive operation, and not something people often want. Indeed, you can often just write simple names that will 'spell check' to some existing name in some namespace that you have absolutely no interest in. When we did the "spell-check + add-using" this was a significant initial problem with it.

We addressed this by deprioritizing this, with the presumption that (more often than not) a name like this is intended to be 'fresh' and to generate something new rather than to represent something you want to spell-check-and-add-import for. And, in teh rarer cases you want that, having to go manually down in the list is reason and helps ensure the user is specifying exactly what they want.

gundermanc commented 6 years ago

This is very intentional behavior. Spell-checking and adding a using is a very invasive operation, and not something people often want.

This feels like an assumption and one that I very much disagree with.

Indeed, you can often just write simple names that will 'spell check' to some existing name in some namespace that you have absolutely no interest in. When we did the "spell-check + add-using" this was a significant initial problem with it.

This is interesting. Perhaps spell check should 'score' the closeness of the match and change priority dynamically? Though I still think 99% of the time I want the local fix and not the action that generates code or refactors my solution.

Was an experiment ever run on this? I'd go as far as to suggest that perhaps reduced usage of 'rename' is a reasonable cost to potentially better usage engagement that might result from this grouping change.

CyrusNajmabadi commented 6 years ago

This is interesting. Perhaps spell check should 'score' the closeness of the match and change priority dynamically?

Well, it already does this to some extent. Spell checking already only surfaces things we think are very close matches. i.e. no more than 2 character change for a large name, and no more thna 1 character change for a small name.

Was an experiment ever run on this?

Well, we got a load of feedback immediately from the team when spell checking was added. It turns out it comes up far too often in practice because in programs there's a large tendency for names to be close. So people would often hit this and then not at all want the spell checking.

A similar thing happened when we made it so that we could do 'fuzzy add using'. We got a lot of complaints that people would make something new, and then lightbulb, and then then would end up with us changing theit name and adding a using. Something they didn't even realize until several keystrokes later. Undoing was then very annoying.

Since tweaking things to highly deprioritize these, you're now the first person to complain about the ordering :)

So, it's all very unscientific. But just based on volume and intensity of complaints alone, it feels like we landed on a far better default. I would be extremely wary of changing things here.

justcla commented 6 years ago

The intent of this issue is to suggest that the most-likely trivial fix be presented first in the list with the error icon and the less-likely 'code generating / refactoring' actions should appear lower in the list.

I think this is making a big assumption. For example, it's making the assumption that the user most likely wants to perform the most trivial fix. That's not at all clear to me that that's what they will want.

I agree with the assumption that the user is most likely to want to make the trivial fix.

And most certainly, issues that fix errors should be surfaced higher than issues that fix warnings, and those should be higher than fixes that don't fix any known problem.

justcla commented 6 years ago

This image shows two items and I don't know which is a suggestion and which is a tool.

image

There needs to be some visual indicator to help me understand which is which.

(It doesn't help that the low-pri item is above the high-pri item. So I can't even use the relative ordering to give me some sens of it.)

CyrusNajmabadi commented 6 years ago

(It doesn't help that the low-pri item is above the high-pri item. So I can't even use the relative ordering to give me some sens of it.)

Why do we feel that 'add accessibility modifiers' is higher pri than 'change signature'?

--

Tagging @sharwell . This seems like an area where we need to surface an analyzer (i.e. suggestion/lightbulb), more like a refactoring (i.e. screwdriver) if its severity is none/refactoring. Is that something that is on your plate as part of the work you're doing?

justcla commented 6 years ago

Thanks @CyrusNajmabadi.

Yes, I am assuming the "Add modifiers" is a higher priority because it was surfacing as a warning. (ie. something my VS is suggesting I fix), whereas there is nothing (else) wrong with my method signature and there is no reason whatsoever for me to need to choose that action.

I agree with your comment to @sharwell . I am proposing that suggestions be surfaced higher that tools.

CyrusNajmabadi commented 6 years ago

Yes, I am assuming the "Add modifiers" is a higher priority because it was surfacing as a warning. (ie. something my VS is suggesting I fix

Is it actually a warning? I don't see a squiggle in your code.

whereas there is nothing (else) wrong with my method signature and there is no reason whatsoever for me to need to choose that action.

There's the rub. That's very hard to determine (esp. if there is no adornment in the code itself). Maybe you did want to go to this method to change it's signature :)

CyrusNajmabadi commented 6 years ago

Note: to be very explicit about how i think the experience should be:

  1. if there is an adornment in the code (i.e. suggestion-dots/warning/error) and you're on that adornment. Whatever is adorned should likely get highest priority.
  2. If there isn't an adornment in the code (i.e. it's a refactoring, or an analyzer with 'none/refactoring' severity), then standard ordering applies.

Note: being 'on' the adornment is also trick to figure out. If something on column 5 is adorned, but i go to code at column 75 to perform some op, it's likely what what i'm directly on should have highest priority.

This has been a general problem with the 'line-oriented' lightbulb approach. In previous versions of VS we had the lightbulb at the cursor location, so it was tied tightly to "where is the user". Now, because it collapses everything on the line, it gets very ambiguous and confusing as to what should be shown.

For someone like me, it's absolutely always about what my cursor is on. But other people sometimes just navigate to the line, see hte issue, and click on the lightbulb on the left expecting to be able to fix it.

justcla commented 6 years ago

Is it actually a warning? I don't see a squiggle in your code.

My bad. It might not have been a warning. All I know is that when I addressed that issue, the overall lightbulb indicator changed to a screwdriver. It guess it highlights how hard it is to determine the severity of the fix being targeted or the importance of the fix being suggested.

Maybe you did want to go to this method to change it's signature :)

Maybe. But not likely while there is a specific fix being suggested to you.

CyrusNajmabadi commented 6 years ago

Maybe. But not likely while there is a specific fix being suggested to you.

I disagree completely. IN something like roslyn, it's quite likely the case that on most lines i have some sort of analyzer suggestion being offered. Often just with suggestion dots. Do i want to fix those? Not especially. They're just suggestions about things i can do. I'm often going to the line to work on other issues. Now, this may be different if it's an actual error/warning. And this is where the complexity comes in. I think this whole space needs a lot of thought given to it because it's not all clear that thre's a one-size-fits-all approach that will work here. For more detail see my post on the other thread here:

https://github.com/dotnet/roslyn/issues/24996#issuecomment-401491721

CyrusNajmabadi commented 6 years ago

IMO, we should actually try to formalize all of this. Bring up all the relevant concepts that could affect if we wanted a lightbulb item higher/lower and try to actually figure out how they all interact. Then we can actually take that and code up a solutoin.

Note: such work is still predicated on the assumption that actually ever changing ordering at all is a good idea. There are strong arguments that it isn't.

justcla commented 6 years ago

I agree that there's a lot of complicated issues, and various user behaviors. It certainly feels like a topic that deserves a full and thorough discussion.

Was just chatting with Christian. Some of the other issues to tackle are:

justcla commented 6 years ago

If we could introduce icons on the left of the items, users will be less confused when the lightbulb does not match the severity of the first item in the list. Also, they will be able to know when to stop scanning through suggestions - and can focus on the error fixes and warning suggestions.

Then we can argue all day about ordering, but at least the users will be informed about what the items are (ie. Code fix, suggestion, tool, etc).

CyrusNajmabadi commented 6 years ago

@justcla I'm very open to such an idea. However, note that using iconography for other purposes is also something we've wanted to do for many items (and what we already use for some). For example, the 'add nuget reference' action already uses the an nuget-package icon to help draw out that some serious bizniss is about to go down.

This opens up another issue to raise in the discussion: if everythign has an icon, you lose the ability to really make an item stand out from teh rest in this manner. Maybe that's acceptable, or maybe its' a problem. Yaay designing things...

justcla commented 6 years ago

@CyrusNajmabadi Indeed! Yes, icons are already well used through VS to help give context.

if everythign has an icon, you lose the ability to really make an item stand out from teh rest in this manner. Maybe that's acceptable, or maybe its' a problem.

In this case, there is no need for an item to stand out. The icons are not there to make things stand out. They are there to help users understand more about the item. If all items have screwdrivers on them, users know they can safely ignore all options. If any items have lightbulbs, users should probably look closely at those suggestions.

For me, that's the main point. I want to know which items I can safely ignore - as in - I don't even want to waste time reading the text. Just a quick scan for lightbulbs, then shut it down. Without icons, you must at least read the text and in many cases fly out the item to see whether it is an error that needs to be fixed or just a tool.

CyrusNajmabadi commented 6 years ago

For me, that's the main point. I want to know which items I can safely ignore - as in - I don't even want to waste time reading the text. Just a quick scan for lightbulbs, then shut it down.

That's an interesting use case. Definitely something to consider!

In this case, there is no need for an item to stand out. The icons are not there to make things stand out. They are there to help users understand more about the item.

in this case yes, that's true. however there are other cases we also use the lightbulb for where we do want things to stand out. And i want to make sure we don't lose that capability.

Note that a core problem here is trying to infer intent around a feature that intentionally mixes many different sorts of user available operations. For example, a lot of the time i'm using the list just to refactor, not necessarily 'fix'.

Now, it seems attractive to then say: icons will make this better, because you can just look for the screwdriver. However, as i pointed out above, this now means we have a problem distinguishing light/cheap/safe features from potentially hugely expensive and impactful features (like 'install nuget package').

ideas on how to address this:

  1. have two icon locations (maybe at hte start/end). Then we can always have lightbulb/screwdriver show up, but features could still have another place to put a feature-specific icon in place.
  2. have the dropdown always show lightbulb/screwdriver by default, but allow features to supply their own icon that overrides that.

Both of these would allow for the 'nuget' icon to appear somewhere, letting me know what i was about to get myself into:

image

justcla commented 6 years ago

however there are other cases we also use the lightbulb for where we do want things to stand out.

we have a problem distinguishing light/cheap/safe features from potentially hugely expensive and impactful features

I don't think this is an issue. The degree to which a feature is impactful is on sliding scale. There are really no discreet values to it; some features are very small, some medium, some big, some very big. What's "impactful" is very subjective.

And anyway, the NuGet icon does not indicate to me that this is impactful. It just indicates that it's NuGet. Some packages have fun icons. Some don't. The decoration is superfluous - unless you have some inbuilt link between the package icon and the impact it has on y our project.

Also... as far as I'm aware, the menu system is setup to use just one icon. The change to try to incorporate two icons would require additional work that would likely see the whole things pushed back.

As such, I propose we go with the concept of the single icon (lightbulb-error/lightbulb-warning/screwdriver) against each item in the lightbulb list.

KirillOsenkov commented 6 years ago

I personally don't see why all menu items couldn't have icons. NuGet icon will still stand out. Would indeed be nice and informative to have the screwdriver/lightbulb next to each item.

Look in Solution Explorer, all files have icons next to them!

CyrusNajmabadi commented 6 years ago

And anyway, the NuGet icon does not indicate to me that this is impactful. It just indicates that it's NuGet. Some packages have fun icons. Some don't. The decoration is superfluous - unless you have some inbuilt link between the package icon and the impact it has on y our project.

I disagree. The very fact that thsi has an icon and that the icon is Nuget calls out that this is more than just any of the normal code actions. Without a special icon here, you will get more people invoking this thinking it's just some sort of simple 'add using' and ending up installing nuget packages they don't want.

CyrusNajmabadi commented 6 years ago

As such, I propose we go with the concept of the single icon (lightbulb-error/lightbulb-warning/screwdriver) against each item in the lightbulb list.

I don't think this is sufficient. I would be ok with this if certain items can override the icon. But not otherwise.

CyrusNajmabadi commented 6 years ago

I don't think this is an issue. The degree to which a feature is impactful is on sliding scale. There are really no discreet values to it; some features are very small, some medium, some big, some very big. What's "impactful" is very subjective.

I also disagree with this. When literally all but two fixes just make source code changes, and then two fixed actually go and change your project, that's very impactful. It's also not something that is obvious while coding, and may only be something you realize much later when (or if) you're examining a diff.

I don't think we should ignore this. This has been an issue in the past and people did feel like it was unexpected that a simple lightbulb they were invoking ended up causing these sorts of issues.

justcla commented 6 years ago

and that the icon is Nuget calls out that this is more than just any of the normal code actions

This is strongly based on your understanding of NuGet. This is what I meant by having some inbuilt link (in your head) between NuGet and the impact likely to occur. I don't think we can expect all users to be able to easily link specific icons to the impact of the change. I don't know if there really IS such a thing. Does every tool's icon tell you how impactful the change is? Or is it just that you are personally aware that NuGet will add a package reference?

For me, I would need to see a lightbulb or a screwdriver against the action to know whether I should do it. Knowing that it's a NuGet action doesn't help me there. Do I do it or not? Is VS suggesting I do this? This is the information that I would get if there was a lightbulb or screwdriver.

I think we assume too much about a user's knowledge of the product, it's tools and the effect they have if we think that the tool/package icon will clearly help the user understand the impact. For most things, the user will learn by trying it. It's likely those icons will just cause noise, and the inconsistency of it will just cause confusion.

If, instead, each item clearly showed the "level of suggestion" (bulb/screwdriver), then I am empowered to start learning and applying good coding practices. Simply by taking all the suggestions, I'll be heading in the right direction. But if I don't know what's a suggestion (which is where I am today), then I'm likely to apply fixes that were NOT suggested and could have negative affects on my code. I would stop trusting suggestions and then lightbulb suggestion paradigm looses its value. Again...this is actually where I am today. Until we get icons on every action, I treat all actions with skepticism and mistrust.

justcla commented 6 years ago

I don't think we should ignore this. This has been an issue in the past and people did feel like it was unexpected that a simple lightbulb they were invoking ended up causing these sorts of issues.

OK, if this is an issue, then maybe we need to look at special consideration for items like this.

However, I don't understand how the NuGet icon tells a beginner user that this change will affect their project file.

Perhaps the better solution for this and other specific fixes that have impact wider than the source code is to have an alert/confirmation dialog for those actions.

But where do we draw the line? Modifies one line? One Method? Multiple methods? Multiple files? CSProj? other config files? This comes back to the non-discreet issue of impact. What do we consider has impact enough that the user needs to beware?

Whatever it is, we'd need something more obvious than a package's icon, as only through experience can a user start to relate the package icon to the impact. (Even then... not always).

justcla commented 6 years ago

I don't think this is sufficient. I would be ok with this if certain items can override the icon. But not otherwise.

At the end of the day, we need to come to some consensus in order to move forward. (We still need to convince Christian. lol)

The simplest solution to implement would probably involve adding screwdriver icons to all code fixes and lightbulb items to all Roslyn analyzer fixes (error-bulb or warning-bulb). Items like the NuGet set will probably fall outside this treatment and will keep the icons they have today.

So instead of certain things overriding the icons, we'd go the other way and just add icons to things that didn't already have them. (ie. Code fixes and suggestions).

You'll still see the NuGet icon (and interpret that how you like). And I'll get to know what's a suggestion so I can start improving my code and learning good habits.

Would that work?

CyrusNajmabadi commented 6 years ago

This is strongly based on your understanding of NuGet. This is what I meant by having some inbuilt link (in your head) between NuGet and the impact likely to occur.

That's not actually waht i'm saying :)

I'm saying that this item is sufficiently differentiated to draw attention to the user that there is something special here. So the user pays more attention to it, and then gets more clues that there will be something more going on here vs other fixes they perform.

This is similar to what happened jsut when we deprioritized fixes. Because a user now has to go explicitly move to select the item, it seems as if they pay more attention and realize this may end up doing more than they may have originally though based on just a cursory glance/invoke.

--

My major point aobut icons is that currently we provide a lot of signal by using icons sparingly. It makes these items very different, and thus hopefully causes more attention to be paid to them. I simply do not want to lose that. :)

CyrusNajmabadi commented 6 years ago

Would that work?

I would certainly be open to trying it out :) I'd want to see how it looked and i'd want to try out the experience a while as well. Because, for all i know, there isn't actually an issue here, and we don't actually need to necessarily make any changes.

In other words: making changes may actually make things worse. So i only want do make changes here if they feel strictly better.

justcla commented 6 years ago

It makes these items very different, and thus hopefully causes more attention to be paid to them. I simply do not want to lose that.

In the case of the lightbulb menu, I think it's more important to call out which are suggestions vs tools than to help identify which ones will have the greater effect. But I'm prepared to accept they might both be valid options. In both cases there is useful information that could help a user make a decision about whether to take the action.

justcla commented 6 years ago

Because, for all i know, there isn't actually an issue here, and we don't actually need to necessarily make any changes.

There definitely is an issue. As a user, I am unable to determine which items are suggestions and which are tools, and this is affecting my ability to use the lightbulb. It caused me real issues last Friday, which is why I sent through my feedback about my challenge in knowing what I should do.

I am still struggling with this. This is a very real problem for me.

So i only want do make changes here if they feel strictly better.

We all agree here. I propose that adding icons to those that previously had none will create no harm but will add value. Of course, we'll really only be able to validate that once we try it.

CyrusNajmabadi commented 6 years ago

In the case of the lightbulb menu, I think it's more important to call out which are suggestions vs tools than to help identify which ones will have the greater effect.

I don't know. Making large scale, unintentional changes to a user's project is a pretty bad thing to do. I mean, we did get the feedback about this, and we've used this sort of feedback to drive to where we are today. We've gotten a lot less feedback about the types of concerns that @gundermanc has brought up.

So i'm curious why we're thinking that this new concern is more important. What is actually driving that beyond this bug?

CyrusNajmabadi commented 6 years ago

There definitely is an issue. As a user, I am unable to determine which items are suggestions and which are tools, and this is affecting my ability to use the lightbulb. It caused me real issues last Friday, which is why I sent through my feedback about my challenge in knowing what I should do.

I'm definitely getting confused then. Because you opened this issue with:

image

Here you've described how you don't think ti's good that "generate type" is above "spell-check."

However, the suggested changes to lightbulbs don't change any of this. Both of these will become "lightbulbs" if we add icons here. Both are 'fixes' to try to 'correct' the problem in your code.

about my challenge in knowing what I should do.

I think the problem is: from a tooling perspective we have no idea what you should do either. Is spell-checking the right thing? Maybe... maybe TokenInfos really was was a mispelling of TokenInfo (that is pulled in from another namespace). Or maybe you really want to create a type called TokenInfos because that's something you're creating to solve whatever problem you're working on now.

justcla commented 6 years ago

What is actually driving that beyond this bug?

I have never had an issue with the size of the impact of a change. And I don't even think adding a single package reference is a particularly impactful change. (Unused references don't really impact the project.) Also, I'm used to the fact that some changes will affect a single line and some will affect multiple files (ie. rename). It just comes with the territory. I'm curious as to when the issue of knowing the impact of the change became a bug.

What's driving this bug is the very real situation I found myself in while trying to address all lightbulbs in my file. (That is the intent of the bulb, right? To highlight where an issue must be addressed?) I was simply unable to know which of the three items in the list were suggestions that I should follow or just tools I could use. Only by applying each one could I tell (seeing if the lightbulb went away).

That is what's driving this bug. We have this great facility for promoting suggestions to the user, but we fil it with other actions that are NOT suggestions. And we do not make this separation clear.

CyrusNajmabadi commented 6 years ago

What's driving this bug is the very real situation I found myself in while trying to address all lightbulbs in my file. (That is the intent of the bulb, right? To highlight where an issue must be addressed?)

Not to my knowledge, no. The point is to make you aware you can do things in this location. These drop downs have existed since 2005, and have just been a way for us to surface functionality to the user that we feel is relevant to their contextual locaiton.

I was simply unable to know which of the three items in the list were suggestions that I should follow or just tools I could use. Only by applying each one could I tell (seeing if the lightbulb went away).

I don't see how the iconography helps here. We can't answer which item "[you] should follow". That's up to you. There are many things offerered precisely because there are many sorts of solutions that might work.

--

And, in practice, many of the 'solutions' may not be the right one, or may end up just leading to more squiggles/fixes that need to be applied. For example, if you write: Foo.Bar.Baz, there will likely be a suggestion to introduce a 'Foo' variable. But once you do that, you'll still have continued squiggles and fixes you may need to apply to address additional problems on that line.

The goal of the lightbulb isn't "keep invoking me until code is correct". The point goal of the lightbulb is: here are things you can do here. Depending on what you're doing and what you're trying to address, pick whichever of these you feel is most relevant for you getting the task done.

justcla commented 6 years ago

@CyrusNajmabadi , That was @gundermanc who opened with that. And my apologies, there are several threads that seem to be across these issue. I raised feedback directly to Christian last week about the issue I had determining which was a suggestion vs lightbulb.

I agree with you about that:

CyrusNajmabadi commented 6 years ago

I have never had an issue with the size of the impact of a change. And I don't even think adding a single package reference is a particularly impactful change.

That's not what generally happens with .nuget. When you add such a package, you will get things like a package.json (which you may not want at all). It will also add a packages folder to your project, and suck in a bunch of files into it.

CyrusNajmabadi commented 6 years ago

@justcla Sorry for confusing you two :-/ That's my bad.

perhaps we should start fresh. I'd like to have an issue that clearly shows a case where we have an issue here, along with the suggested fix and how it directly addresses that problem.

I realize we may already have that in this bug. but i think the conversation has bounced around far too much, and we should have a clean slate so we can just drive resolution on that case wthout all the clutter and baggage here :)

Is that ok with you?

justcla commented 6 years ago

The goal of the lightbulb isn't "keep invoking me until code is correct". The point goal of the lightbulb is: here are things you can do here. Depending on what you're doing and what you're trying to address, pick whichever of these you feel is most relevant for you getting the task done.

That might have been the original purpose when lightbulb was all there was. But now we have the ability to distinguish between code fixes and suggestions - so I want to take advantage of that.

Also, we've received user feedback that showing a lightbulb on every line where an action is possible is terribly distracting. People are asking for the tools to be removed rather than be forced to see a lightbulb everywhere.

So, I'm proposing that we improve the experience for the user here.

I've seen this behavior in other tools and it's very helpful.

justcla commented 6 years ago

I don't see how the iconography helps here. We can't answer which item "[you] should follow". That's up to you. There are many things offerered precisely because there are many sorts of solutions that might work.

We absolutely can.

We can show which items "might provide the fix you're looking for" (all lightbulbs) and with a screwdriver we can indicate "this will definitely not fix any known issues". That is SO important to me. I am happy to carefully look through the list of suggestions to see which one will give me the fix I want. But I don't want to look at any "tools" as that's just wasting my time.

CyrusNajmabadi commented 6 years ago

Also, we've received user feedback that showing a lightbulb on every line where an action is possible is terribly distracting. People are asking for the tools to be removed rather than be forced to see a lightbulb everywhere.

It's unclear to me how any of the suggestions given so far address that. Won't the lightbulb still be shown everywhere? :)

Note: i'm 100% for improvements in this area. I just want us to be focused and disciplined about it. We need to understand the cases and decision making that let to current designs, and we have to be cognizant about how changes may negatively affect very important aspects of that design.

justcla commented 6 years ago

I will create a new issue to address to problem I'm having with no knowing which are suggestions or tools.

CyrusNajmabadi commented 6 years ago

I will create a new issue to address to problem I'm having with no knowing which are suggestions or tools.

Thanks!