Roslyn: Only prioritize location-specific fixes when caret on squiggly

[justcla]

The order of items in the lightbulb menu changes depending on where your caret is on the line.

It's great to be able to have location-specific items surface at the top of the list when your caret is ON an item, or when there is a selection that would trigger location-specific items, but when the caret is NOT ON a squiggly, the order of items is confusing and unpredictable.

I propose that location-specific lightbulb items only be prioritize when the caret is actually on an squggly. Otherwise, the items should be ranked consistently with whatever rules are in the system (ie. highest pri, left-to-right, etc).

This continues from a discussion started in Issue #28241

----- Original post: --------------------------------------------------------------
On the issue of sorting by closest distance, I find this odd.
I totally get that when your caret is ON a squiggle, or your selection would trigger an action (like extract variable), those actions should be ranked higher than other actions on that line that are not EXACTLY where the caret/selection is. That is important, as you mentioned, so that you can navigate to the point and trigger the first item in the menu.
But I think the flaw is in using "nearest match". When your caret is between two squiggles, you will get a different order depending on how far to the left or right the caret is between the items.
My view is, unless you are actually ON the squiggle, it should not be artificially surfaced to the top.

This would provide better consistency and predictability. I should be able to get used to knowing the order items will appear in; whether I press Home or End, the items in the list should be in the same order - unless I happen to have navigated onto a squiggle or made a selection that triggers specific actions.

[justcla]

I'm not sure i understnad your perspective here. The model we have eases the burden on the user. They don't have to know the exact location to put their caret. They can just be typing, see that an issue occurred, and bring up the lightbulb and have much more relevant fixes to where they are appear.

What you've just described is the case where the caret is to the right of something just typed. I agree, in that scenario we should offer help on what was just typed. We have an issue with this today, as the block tagger seems to want to associate the caret with the item to the right. There is an issue I mentioned in the other thread where we fail to do this properly. Is it possible this nearest-match conundrum is mostly designed to fix that particular scenario?

But what's the scenario where a user has moved their caret a few positions to the left? Or where the caret is between issues? That's not "I was just typing, now I want help on that item." That's "I'm moving around, potentially investigating errors." In those cases the user often uses the mouse to hover over an item, at which point we apply exact-matching priority if they hover on a squiggle. Or if they are navigating with the keyboard, why move the caret to a spot that's not on an error? Maybe because there is no easy "Go to next issue" shortcut, which would put them right on the issue. (Note: GoToNextIssue is available in the current VS 15.8 preview.)

So, yeah, I get the point about wanting relevant items for things you just typed. But we might be going too far to stretch that treatment everywhere. Instead, we should probably look at addressing the bugs that fail to bring up issues on things just typed.

[justcla]

Again, if we don't do this, then you could have fixes show up for things literally 80 characters away, instead of for the issue 2 characters away from you when you bring up the lightbulb. Not only would this be extremely bad and confusing, it would definitely be a muscle memory break for many people. Here's the relevant example again:

Yep. That's what I would want. If I'm not ON an issue, I don't want the noise from issue affecting the full list of items available on the line. Let me have a consistent list of items I can address. If I want the order to change, I will navigate to a particular item.

What if the warning item two chars away has a whole bunch of items (that I really don't care about), but there's an error further along the line. I've arrived at the line position 0. Which do I want to address? Most likely I want to fix the error first. I probably don't even want to address the warnings. So now the near-position matching is making me move far down the list to find the fix I need.

It's the opposite of "bad and confusing" IMO. It's great and consistent.

I still don't get why you want to be two chars away. What is that scenario and how does it arrive? Is this really because we are missing navigation features?

[justcla]

I've just written Bar(); and now i want to generate the new method 'Bar'. If we interfere with that by having Generate type 'Blorb' show up above htat, then that's a seriously bad experience.

Yeah. Fair enough. Again, this is the "I just typed something, now I want relevant fixes" thing. That's a valid scenario.

Conversely, if the user hits home to go to the start of the line and brings up the lightbulb, then helping them fix the issue at the start of the line is most relevant to where they are now.

Eh, maybe. Maybe not. One might argue that fixing the nearest error is best. (rather than a closer warning).

Consider that if we don't take position into account, users may even see fixes at the top for things that they can't even currently see on their screen.

I accept that. Sometimes that will happen anyway, no matter how we order the list.
Users need to control their screen width and line size.

[justcla]

Most particularly, when we get this stuff wrong, people tend to notice and get quite upset. Not taking location into account when we changed to the left-margin-lightbulb was a good example of that. For years people had been used to being able to just do ... code ... Foo();, hit ctrl-dot and generate their new 'Foo' member. When left-margin-lightbulbs came online and we only prioritized an issue if you were on it, this was a serious issue. People felt this immediately and didn't like that we were showing them fixes for things far away in code when they actually wanted fixed for what they had just been editing.

Again, repeating pattern. The "I just typed something". We need to ensure that always works. But that never puts you in the case where the caret is to the left of the item. Maybe the problem is that we applied a solution designed for one thing to situations that aren't relevant. (ie. navigating caret to other places on the line.)

[justcla]

I don't see anything that would have changed any of these issues.

Well for one, there have been changes to the way the lightbulb surfaces. (Now favoring left side bar more often). Who knows what subtle effects that's had on the previous design.

But also, most significantly, the Editor team are producing new navigation commands and shortcuts. In particular the GoToNextIssue, GoToPreviousIssue commands that will move the user's caret from one issue to the next. This enables users to easily put their caret directly on a squiggle to access the relevant actions from the top of the menu. Also a new command GoToLastEdit will move the caret to where the user just typed. Having better keyboard navigation commands is likely to mean that users will be navigating exactly to their position of interest.

If the user doesn't want those items showing up, by simply moving the caret off the squiggle, they should then return to their reliably ranked menu of items for that line. That provides consistent, predictable behavior.

[CyrusNajmabadi]

I propose that location-specific lightbulb items only be prioritize when the caret is actually on an squggly. Otherwise, the items should be ranked consistently with whatever rules are in the system (ie. highest pri, left-to-right, etc).

I think any such proposal needs to actually enumerate all the cases this will affect in Roslyn and show that this introduces an improvement.

[CyrusNajmabadi]

What you've just described is the case where the caret is to the right of something just typed. I agree, in that scenario we should offer help on what was just typed.

This differs from what your proposal was, which said:

I propose that location-specific lightbulb items only be prioritize when the caret is actually on an squggly.

When you're to the right of the squiggly, you're not on it. Can you restate what your proposal is?

[CyrusNajmabadi]

But what's the scenario where a user has moved their caret a few positions to the left? Or where the caret is between issues?

This often happens. The squiggly will be on some token (like the type of a local), but someone clicks on the 'name' of the local (or f12s there from some other location). They are two chars away from a squiggle on the type.

Note: i did this earlier today when i had a out SemanticModel model in my code. i went to definition 'model' to change this to out var model. That could have be deprioritized to something that could have been dozens of characters away.

[CyrusNajmabadi]

Again, if we don't do this, then you could have fixes show up for things literally 80 characters away, instead of for the issue 2 characters away from you when you bring up the lightbulb. Not only would this be extremely bad and confusing, it would definitely be a muscle memory break for many people. Here's the relevant example again:
Yep. That's what I would want. If I'm not ON an issue, I don't want the noise from issue affecting the full list of items available on the line.

I'm confused by thsi. If you're not prioritizing by location, then you have the exact issue i'm referring to. Where fixes show up for things very far away, instead of based on the current typing context.

What if the warning item two chars away has a whole bunch of items (that I really don't care about), but there's an error further along the line.

Then i would prefer to work on the warning. Because i moved right there and that's where my attention is.

Again, this is hte normal development flow for: i'm typing and i'm fixing up the things that i'm writing.

I still don't get why you want to be two chars away. What is that scenario and how does it arrive? Is this really because we are missing navigation features?

Because i'm typing... i've already given the situations where this happens. The expectation that my typing ends exactly on a squiggle isn't something that necessarily holds. I'm going to type however i want to type. I may or may not end up on a squiggle.

[CyrusNajmabadi]

Eh, maybe. Maybe not. One might argue that fixing the nearest error is best. (rather than a closer warning).

really? the nearest error may not even be visible. :)

I accept that. Sometimes that will happen anyway, no matter how we order the list.
Users need to control their screen width and line size.

i don't accept that. That is strictly degrading my experience. I honeslty don't know what problem this is actually solving. This will negatively impact the actual user experience immediately, for no apperant gain. I don't believe we've ever heard a message saying that the ordering of fixes here (based on distance) is actually a problem. So i'm not going to support any changes here unless they solve a real and actual problem enough of our userbase are having right now.

[CyrusNajmabadi]

Again, repeating pattern. The "I just typed something". We need to ensure that always works.

Ok. How do you ensure that that works? What is the full proposal here?

[CyrusNajmabadi]

(Now favoring left side bar more often). Who knows what subtle effects that's had on the previous design.

Many of these design decisions were made given the left-lightbulb-margin change.

But also, most significantly, the Editor team are producing new navigation commands and shortcuts. In particular the GoToNextIssue, GoToPreviousIssue commands that will move the user's caret from one issue to the next.

this isn't relevant. That only affects users who use those commands. Roslyn needs to consider users (and their muscle memory) who do not use those commands. We cannot just go and change the model in this circumstance because of new commands they may or may not use.

Note: i highly approve of GoToNext/PreviousIssue. However, none of those necessitate a design change. After all, if you GoTo the issue, then the issue will clearly be the nearest thing to you. So the fixes for that issue will show up first. Which is a good thing.

To me, it sounds like you want to change something here for the sake of changing it. When it hasn't been appropriately proven that hte change will actually be a net positive, and there is high risk of large negatives associated with the change.

[CyrusNajmabadi]

Also a new command GoToLastEdit will move the caret to where the user just typed. Having better keyboard navigation commands is likely to mean that users will be navigating exactly to their position of interest.

I don't know what this means. "GoToLastEdit" doesn't help me. My last edit is exactly where my caret is. And my caret is not on a squiggle. But there is a squiggle 2 characters before where i am. You're saying i would need some sort of special keystroke to get back to the squiggle? Or move back in my edit space, just to be able to bring up the right lightbulb?

This is not a good idea. It breaks muscle memory we've gotten in people for 10+ years. It throws it away to get something of questionable value that we have never gotten feedback on. It feels very much like change-for-change's sake.

[justcla]

What you've just described is the case where the caret is to the right of something just typed. I agree, in that scenario we should offer help on what was just typed.

This differs from what your proposal was, which said:

I propose that location-specific lightbulb items only be prioritize when the caret is actually on an squggly.

When you're to the right of the squiggly, you're not on it. Can you restate what your proposal is?

This is a problem: "When you're to the right of the squiggly, you're not on it."
It needs to be fixed. Whether you are at the beginning, end or middle of a token, you should get the items that relate to it. It seems we're using nearest sort to work-around this bug.

So to restate the proposal, it's as I said with the understanding that caret touching the item (on the right of the token) counts as caret ON the squiggly.

[CyrusNajmabadi]

This is a problem: "When you're to the right of the squiggly, you're not on it."
It needs to be fixed. Whether you are at the beginning, end or middle of a token, you should get the items that relate to it. It seems we're using nearest sort to work-around this bug.

What if you're not on the squiggly. I.e. i'm to the right of it. i.e.:

c# /* lots of code on this line */ + this.Foo(a, b, /*more code ...*/, d); $$ // <-- cursor is that the $$

How do i call "generate method Foo" here? My cursor is not on the squiggly. There may be many errors on this line (including ones at the start that i can't even see on my screen).

--

This is not a hypothetical. This is a core scenario we've designed for and supported from smart-tags, all the way through the lightbulb transition. When we've gotten this wrong, people scream, because we're actively destroying their muscle memory.

Again, this is common. People commonly write out a full construct, but then want to fix something about that construct. If you show issues that are further away, and you require the user them to move their caret to the squiggle, then this will not fly. It is as close ot a deal-breaker in this experience as i can imagine.

[justcla]

This often happens. The squiggly will be on some token (like the type of a local), but someone clicks on the 'name' of the local (or f12s there from some other location). They are two chars away from a squiggle on the type.
Note: i did this earlier today when i had a out SemanticModel model in my code. i went to definition 'model' to change this to out var model. That could have be deprioritized to something that could have been dozens of characters away.

Absolutely. That's what I propose. If there is a more important fix further away, it should come to the top. Why deal with a less important fix (or warning) just because it's arbitrarily closer to the caret?

Bear in mind that the fixes will still BE in the list. And if there are no other __more important__ fixes on that line, you're likely to still get the options at the top.

[justcla]

Again, this is common. People commonly write out a full construct, but then want to fix something about that construct. Requiring them to move their caret to the squiggle will not fly. It is as close ot a deal-breaker in this experience as i can imagine.

I guess this problem stems from the fact that we don't provide enough boiler-plate characters when we complete items.

If, when completing a method, we provided the parens and moved the caret inside (similar to the completion experience in json), then the user would not need to type out the rest of the expression, then "go back" to fix issues.

Again, it seems we're using nearest-match to make up for other issues that cause problems.

Perhaps we should fix completion instead. Then a user won't need to navigate off the thing they just typed.

[CyrusNajmabadi]

If, when completing a method, we provided the parens and moved the caret inside (similar to the completion experience in json), then the user would not need to type out the rest of the expression, then "go back" to fix issues.

This does not help with the case i listed:

```c#
/* lots of code on this line / + this.Foo(a, b, /more code ...*/, d); $$ // <-- cursor is that the $$

Even if the caret is here:


```c#
/* lots of code on this line */ + this.Foo(a, b, /*more code ...*/, d$$);  // <-- cursor is that the $$

You are still not on the squiggle.

[justcla]

I'm confused by thsi. If you're not prioritizing by location, then you have the exact issue i'm referring to. Where fixes show up for things very far away, instead of based on the current typing context.

If you're not on the issue, it's no longer the typing context. You've moved away.

What if the warning item two chars away has a whole bunch of items (that I really don't care about), but there's an error further along the line.

Then i would prefer to work on the warning. Because i moved right there and that's where my attention is.

"Because i moved right there"

No you didn't. You just said you're two chars away. That's not "Right there". hence my point. I agree that if you move "right there" you should get the items. But when you're not "right there", you're not there.

Reminds me of the Grand Old Duke of York.

And when they were up, they were up,
And when they were down, they were down,
And when they were only half-way up,
They were neither up nor down.

And when I'm on an issue, I'm on an issue. When I'm not, I'm not.

[CyrusNajmabadi]

Again, it seems we're using nearest-match to make up for other issues that cause problems.

No, we're using 'nearest match' because it really matches user expectation without imposing unnecessary restrictions on them with how they type or what other IDE features they use. A user can write out hte method-call however they want. They can write it out entirely manually. They can use some tool that spits out the end parts. But when they're finally done, they can then go generate it, without having to worry about how some errors dozens of characters away (that they may not even be able to see) can affect them.

This is not 'making up for other issues'. This is matching the common mental model of "this is what i'm working on, bring up the fixes related to that".

[CyrusNajmabadi]

If you're not on the issue, it's no longer the typing context. You've moved away.

@justcla This is not true. I'm sorry, but this conversatoin is extremely difficult because it belies a deep misunderstanding of the typing experience in c# and the mental model people have there, which we have supported for more than a decade.

[justcla]

We're making up for not providing full brace completion.
We're making up for not adhering to the token immediately to the left.
We're making up for not having good shortcuts to easily navigate to an issue.

And yes, nearest-matching helps overcome all these shortcomings.

the typing experience in c# and the mental model people have there, which we have supported for more than a decade.

Perhaps this can be improved.

[CyrusNajmabadi]

No you didn't. You just said you're two chars away. That's not "Right there".

yes it is. When i write: Foo(1, 2, 3, 4);, then my mental model is "i just called the 'Foo' method", not "oh... i'm after the semicolon, therefor if i bring up help, show me things 100+ characters away".

This is clear and consistent feedback we've gotten from users forever.

--

BTW, i don't think this back and forht is being helpful. If you'd like to discuss htis more, reach out on gitter.

[CyrusNajmabadi]

We're making up for not providing full brace completion.

How would brace completion help in this specific example?

We're making up for not adhering to the token immediately to the left.

How would 'adhering to the token immediately to the left' help in this example?

We're making up for not having good shortcuts to easily navigate to an issue.

How would that help in tihs example?

Be very specific. Because we have broken this typing scenario before. Even if we had all of the above, how would that help this specific typing scenario?

[justcla]

I guess we're coming from different coding styles and backgrounds and you're used to working in the world VS has created for users.
I've had different experiences, which have changed the way I work.

[CyrusNajmabadi]

We're making up...

it acutally sounds like you're proposing a whole host of editor features to make up for a change you are proposing which is problematic. Namely that not taking into account where the user is is actually a problem, and in order to address it you'd not only need to introduce a bunch of features, but you'd also need the user to start using them.

Again, i'm happy to discuss this more on gitter if you want. But until there is a real proposal that doesn't break this core scenario, and doesn't require all our users to have to learn a new way of interacting with lightbulbs to deal with this scenario, i don't think i can buy off on this sort of a dramatic change. Especially when the purported benefit is so incredibly nebulous, and has not been anything that customers have actually come to Roslyn to improve.

[justcla]

I'm gald you asked!

We're making up for not providing full brace completion.
How would brace completion help in this specific example?

If the user types Math.Pow and then it completes with parens and places the caret inside, the user doesn't need to type the extra characters ie. ")" that would move them away from the token they just typed. So they never need to "Go Back". They are ready to act on the token immediately. The user falls out of the habit of having to type-over the boilerplate chars.

We're making up for not adhering to the token immediately to the left.
How would 'adhering to the token immediately to the left' help in this example?

Because we don't correctly detect location when the caret is on the right, the system doesn't always add those items to the top. But by using the nearest-match, it gets there. If we just fixed that issue, then we wouldn't need nearest-match to surface actions on the item you just typed.

We're making up for not having good shortcuts to easily navigate to an issue.
How would that help in tihs example?

You see an issue. You want to go to it. You press one key. You're there. Press the same key to move to the next issue. Currently today users must press many keys to navigate to the item. This is likely leading to users not bothering - and just hoping they'll find the item in the list. When you can easily navigate onto an issue, then you're more likely to.

[justcla]

i don't think i can buy off on this sort of a dramatic change.

I guess it's the Roslyn team who needs to make the decision.

[justcla]

What if you're not on the squiggly. I.e. i'm to the right of it. i.e.:

/* lots of code on this line */ Foo(a, b, /*more*/, d); $$ // <-- cursor is that the $$

How do i call "generate method Foo" here? My cursor is not on the squiggly. There may be many errors on this line (including ones at the start that i can't even see on my screen).

Two points:

1. You will still get the option to Generate Foo. We're not discussing adding/removing items. Only the order they appear in the list. If it's a line-action, it will be available.

2. If you are at the end of a line that contains many issues, why wouldn't you address them from the beginning of the line first? You could address them in any order. So why favor last-to-first on the line? I would probably fix the first ones - as they are likely to be adding usings or correcting the method name, which would drive(and potentially fix) errors that were further down the line. If I wanted to address a specific error first, I would either navigate onto it, or just find it further down in the list.

[CyrusNajmabadi]

You will still get the option to Generate Foo. We're not discussing adding/removing items. Only the order they appear in the list. If it's a line-action, it will be available.

It needs to be the top item in the list. You will badly break muscle memory otherwise. This is not hypothetical. We did regress this at one point, and people were unhappy.

If you are at the end of a line that contains many issues, why wouldn't you address them from the beginning of the line first?

because that's not what people do. this is the major issue i'm having here with this discussion. You are questioning how people have been working and coding here here and giving us feedback on for years. For tons of people, they just wrote that call, and they want to generate that member. that's the mental process they're going through. You are suggesting we change that, so the onus is on you to defend this as this would not regress a core scenario we've heard about a lot.

You could address them in any order. So why favor last-to-first on the line?

As i've said so many times now: because that's what i just typed. It's where my head is at. And it's what people tell us they want when we have regressed this in the past.

I honestly dont' know what to tell you at this point. You keep disregarding this even though this is an area we've heard about from years and which we've fine tuned to give a great experience around. And you're proposing something which is not what we've had customers asking about.

Again, if you want to discuss this more, i would be happy to over gitter. But having to repeat thsi same refrain over and over again here is not helping.

[CyrusNajmabadi]

If the user types Math.Pow and then it completes with parens and places the caret inside, the user doesn't need to type the extra characters ie. ")" that would move them away from the token they just typed. So they never need to "Go Back". They are ready to act on the token immediately. The user falls out of the habit of having to type-over the boilerplate chars.

That does not help the code case i provided. Please show how this would actually help that case.

I'll walk you through it. User types. this.Foo(. Fine, so you complete with parens and the caret inside. Now they have this.Foo($$). Now they start typing the args they care about. So they have:

this.Foo(a, b, /*more*/, d$$)

Now they want to generate 'Foo'. Because that's their mental model of what they're working on, and that's what we've supported for 10+ years. They go to bring up the lightbulb. But now the lightbulb shows tehm something that is 120 characters away.

Worse, is that they've become so ingrained to this working, that they just hit ctrl-dot-enter. And instead of getting their generated method, they get something else they didn't intend.

[CyrusNajmabadi]

Because we don't correctly detect location when the caret is on the right, the system doesn't always add those items to the top. But by using the nearest-match, it gets there. If we just fixed that issue, then we wouldn't need nearest-match to surface actions on the item you just typed.

You keep saying this. But you will not answer the question: how does that help the code case i provided. I'll walk you through it:

I'll walk you through it. User types. this.Foo(a, b, /*more*/, d$$).

The caret is to the right of the squiggle. But 'generate method Foo' is not going to be the topmost item in the list, because you insist that the caret must be on the squiggle.

So, again, how would your proposal actually work here?

[CyrusNajmabadi]

We're making up for not having good shortcuts to easily navigate to an issue.

How would that help in tihs example?

You see an issue. You want to go to it. You press one key. You're there.

You have now regressed the scenario. The user's muscle memory is now broken. You have actively made things worse for many users by doing this. This is not hypothetical. When we break muscle memory here (and in other IDE features) people get very upset. You seem to be disregarding it, hoping that people will just use your new keystrokes. But that's not the scenario. Breaking someone's expectations, and then telling them it's ok because they can just cahnge how they work does not fly.

Press the same key to move to the next issue. Currently today users must press many keys to navigate to the item.

No. They don't. That's the point. They can just write the code out, and they can feel confident that the items higher in the list are more relevant to what they're currently working on. Indeed, they can depend on this and they do.

[CyrusNajmabadi]

I guess we're coming from different coding styles and backgrounds and you're used to working in the world VS has created for users.

Agreed. And those coding styles are not ones we can trivially or haphhazardly ignore. Even minor changes here can cause much pain for users. We've experienced that every time we've even tweaked completion ever so slightly.