Show code actions with diagnostics in hover popup

[jwortmann]

This is what I had in mind. Let me know what you think.

Example:

If there are multiple code actions, all of them will be shown on individual lines. But only "quickfix" code actions are requested now for the hover popup. For "refactor" actions you must use the context menu or main menu.

If there are multiple diagnostics at the hover point, only code actions which can safely be matched to a diagnostic (via diagnostics property in the code action) are shown. If a code action applies to multiple diagnostics, then the same code action is shown in each of the diagnostics that it resolves.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeAction

	/**
	 * The diagnostics that this code action resolves.
	 */
	diagnostics?: Diagnostic[];

---

The code action (or command) is now encoded into the link URI directly (using a special code-action URI scheme), which makes additional logic to match a selected index to available code actions unnecessary.

---

I'm a bit unsure about the lightbulb icon in the popup, for now I have used it only for code actions that have the isPreferred property set. We can reconsider that and could also remove it entirely.

---

There was some very basic caching for code actions in the hover popup in the previous implementation (in CodeActionsManager), and that isn't applied anymore. Instead, if there are any diagnostics at the hover point, we now always make a new code action request (in addition to the hover request). I could probably add back the caching, but I'm not sure if it is really needed for the manual mouse hover, and also we currently don't cache the hover response for the point.

---

I have only tested with the basedpyright server, so maybe some more testing could be useful.

[rchl]

I'm a bit unsure about the lightbulb icon in the popup, for now I have used it only for code actions that have the isPreferred property set. We can reconsider that and could also remove it entirely.

I think it looks weird to have it only for some items. Especially since the icon doesn't really match the meaning (not sure why bulb would represent "preferred"). Or does it match behavior in vscode for example?

I guess the options are to either remove for all or add for all? Maybe having it for all would look too busy?

[rchl]

It's more of a style refactor issue but I wonder whether we should only add the horizontal separator between adjacent diagnostics of same severity - in the screenshot between two "blue" diagnostics".
The horizontal lines can look busy and there seems to be missing separation between same-severity diagnostics

[jwortmann]

Especially since the icon doesn't really match the meaning (not sure why bulb would represent "preferred"). Or does it match behavior in vscode for example?

I think in VSCode the lightbulb just means "quick fix available". But they don't show it in the popup or gutter, but instead as a floating icon that can be clicked to open a popup menu: https://code.visualstudio.com/docs/typescript/typescript-tutorial#_quick-fixes

What kind of icon would you associate with "preferred"? The set of available icons to choose from is at https://primer.style/octicons/ (the error/warning/info/lighbulb icons are all from this icon set so that they look consistently).

---

I've pushed a commit 3cb3bba to show the icon to the right side of the link, so that all code action links are left-aligned. Maybe that looks better? We could also remove the lightbulb entirely, but how do we show the isPreferred information then?

If you hover with the mouse over the lighbulb in the popup, it shows a "Preferred Quick Fix" tooltip. So I think that is an intuitive way to learn what it means (although that is not fully consistent with the lightbulb icon in the gutter - so yes we can try out different things and discuss what would be the best solution).

[jwortmann]

It's more of a style refactor issue but I wonder whether we should only add the horizontal separator between adjacent diagnostics of same severity - in the screenshot between two "blue" diagnostics".
The horizontal lines can look busy and there seems to be missing separation between same-severity diagnostics

I think you could add a 1 pixel border-bottom to the severity classes at

LSP/popups.css

Lines 79 to 84 in 32c3f14

	.error,
	.warning,
	.information,
	.hint {
	color: var(--foreground);
	}

and then test how it looks. Otherwise adding <hr> between diagnostics of the same severity could be a bit complicated for the implementation, because at the moment I just sort by severity and then do a ''.join().

Btw, I have also tested to remove the <hr> between multiple code actions for a diagnostic (like in your first screenshot), but then the links look too cramped together in my opinion and you could easily misclick a code action link if they don't have a visible separator line.

[rchl]

What kind of icon would you associate with "preferred"? The set of available icons to choose from is at https://primer.style/octicons/ (the error/warning/info/lighbulb icons are all from this icon set so that they look consistently).

Could we create our own variation of the bulb icon - for example a bulb icon with a small star in the corner?

I think it would be good to have icon for all code actions but then having completely different icon for one of them would look out of place IMO. So that's where my idea comes in.

EDIT: Or a bulb icon with filled in bulb (turned on).

[rchl]

I think you could add a 1 pixel border-bottom to the severity classes at and then test how it looks. Otherwise adding <hr> between diagnostics of the same severity could be a bit complicated for the implementation, because at the moment I just sort by severity and then do a ''.join().

I think the first option would not look that good:

Here is an example with no separators at all but with more prominent spacing that kinda makes it more clear where the separation is:

(we don't have to address it in this PR)

EDIT: Quickly hacked (paddings are not correct) variant with separator but I've just noticed that those last two are actually not the same severity. One is hint and one is information. So logic based on severity wouldn't really help here:

[jwortmann]

Could we create our own variation of the bulb icon - for example a bulb icon with a small star in the corner?

I'll try that out later perhaps.

I think the first option would not look that good:

Ok could you try with padding-bottom: 1px margin-bottom: 1px istead? Maybe that looks more subtle. I prefer to keep the <hr> between related information and between code actions.

[rchl]

I think the first option would not look that good:

Ok could you try with padding-bottom: 1px margin-bottom: 1px istead? Maybe that looks more subtle. I prefer to keep the <hr> between related information and between code actions.

To me this looks weird. I prefer our flat look without margin.

[jwortmann]

I have added a modified version of the lightbulb icon for preferred code actions, and the other code actions use the regular lightbulb now. Could you test that with a server that sets the isPreferred property, and tell whether it looks good now?

[rchl]

I have added a modified version of the lightbulb icon for preferred code actions, and the other code actions use the regular lightbulb now. Could you test that with a server that sets the isPreferred property, and tell whether it looks good now?

If I'd to comment I'd say that it's a bit hard to tell what it means. It covers most of the bulb and of course the icon is quite small so it's probably hard to make it better.

What about filling the bulb itself with the star icon? :)

[jwortmann]

On Windows the icon is slightly bigger (13 pixel) than in your screenshot (11 pixel), so it looks a bit better here. I think I would prefer the current version with the star in the corner.

---

It seems that the image is always positioned at the baseline of the font (see the following screenshot). I wonder whether there is some CSS trick to move the image a few more pixel to the bottom, below the font baseline. Then we could increase the image size a bit:

LSP/popups.css

Lines 107 to 110 in bad4089

	.code-actions .lightbulb img {
	width: 0.9rem;
	height: 0.9rem;
	}

But that is probably not possible in minihtml, I guess?

[rchl]

It seems that the image is always positioned at the baseline of the font (see the following screenshot). I wonder whether there is some CSS trick to move the image a few more pixel to the bottom, below the font baseline. Then we could increase the image size a bit:

I've pushed something to that effect with 1.1rem size. 1.0 would potentially work too.

Not sure how portable that is in different font sizes...

Also, I quite like the icon with star inside the bulb, to be honest.

1rem

1.1rem

[rchl]

Two cases - hovering diagnostics and hovering gutter icon:

---

• Is it expected that code actions don't show up in latter?
• Why is the related information link rendered differently in both cases?

[jwortmann]

I've set the size to 1rem, because 1.1rem looks too huge on Windows. I initially used 0.9rem because that would match the size from the gutter icon exactly (on Windows). But 1rem looks good on Windows too.

---

• Is it expected that code actions don't show up in latter?

It still follows the "show_code_actions" setting, like before. If we want to change that behavior and always include code actions when hovering the gutter icon, we should probably convert that into a boolean setting "show_code_action_annotations": true/false.

We could then also consider to include "refactor" code actions in the annotations. Also maybe instead of "choose (3 available)" we could show all code actions directly in the annotation, because ST only shows the first line of the annotation anyway (with additional linebreak icon if there are multiple lines), and if there are more code actions they would get revealed on mouse hover. But maybe that would be for another PR.

Btw, if there are multiple diagnostics on the line from a single server, then the code actions are only shown when hovering the gutter icon if the code actions include the diagnostics property.

---

• Why is the related information link rendered differently in both cases?

Should be fixed now.

[rchl]

self._code_actions_for_selection contains diagnostics from the line where selection is which is not necessarily the line corresponding to hovered gutter position. This means that we only show code actions if selection is on the same line.

I think we should request diagnostics code actions for the whole line for this purpose.

[jwortmann]

self._code_actions_for_selection contains diagnostics from the line where selection is which is not necessarily the line corresponding to hovered gutter position. This means that we only show code actions if selection is on the same line.

Absolutely. This preserves the current behavior on main.

I think we should request diagnostics for the whole line for this purpose.

We could, but then we should reconsider/update the show_code_actions setting to reflect that (see #2818 (comment)).

[rchl]

We could, but then we should reconsider/update the show_code_actions setting to reflect that (see #2818 (comment)).

I'm not sure I see the connection with show_code_actions... Why couldn't we just request code actions for the whole line and keep the setting as is?

That said, yes, it's an old issue and you can choose not to address it here.

(I of course meant "code actions", not diagnostics (corrected my original comment))

[jwortmann]

I guess we could always request code actions for the entire line when hovering a gutter icon. I'll think about it.

I think the current behavior is partly for historic reasons. First, we only had the code action annotations on the righthand side, and the lightbulb icon, but no popup when hovering over the lightbulb. Then I added the popup with code actions when hovering over the lightbulb. I'm not quite sure, but I believe the diagnostics when hovering a diagnostic gutter icon were added even later into that popup. But still the code actions were only in the popup for the line with the lightbulb icon, which kind of makes sense, I would say, because in theory code actions don't need to be coupled to diagnostics, and previously we displayed them independently of diagnostics.

[jwortmann]

9fa1c0e