Insert inlay hint text edits on double instead of single click #2175

predragnikolic · 2023-01-22T20:02:54Z

This PR will fix the issue where inlay hints could be accidentally inserted in the view.

It introduces a lsp_click command, that accepts the flowing arugments:

command - the command to execute
args - the args of the command that will be executed
count - the number of times the user needs to click it

The default time to wait for the click is 500ms - Yes I know, other systems might tweak this value, but I still think that 500ms is ok.

predragnikolic · 2023-01-22T20:10:21Z

I experimented with other approaches, like sublime-mousemaps.

That would be the best approach, it it worked.
I can create a mousemap, and specify the count to 2,
but then I would lose the default behavior when clicking a word 2 times.

I discovered that mouse-maps also do not support context, but even if they did, I don't know how I would use it to my advantage...

Other options I have considered,
I wanted to open an issue at the ST repo to ask for a new API. For the event to also introduce a count property, next to x and y values.

I also tried this
but I was not happy with that implementation/behavior.

rchl · 2023-01-22T20:26:15Z

I forgot which server has clickable inlay hints. Can you remind me?

Feels like this would create a bit of confusing UX because the mouse pointer will indicate that the thing is clickable but nothing will happen on a single click and there is no indication anyway that it's possible to double click it. Maybe the tooltip (if there is one) should include something like "double click to insert".

plugin/click.py

predragnikolic · 2023-01-22T20:41:54Z

"double click to insert".

Good point,

I took a look,
ST LSP implement showing the tooltip on the client,
and it looks like it is the server responsibility to provide the toolitp content.

export interface InlayHint {
        /**
	 * The tooltip text when you hover over this item.
	 *
	 * Depending on the client capability `inlayHint.resolveSupport` clients
	 * might resolve this property late using the resolve request.
	 */
	tooltip?: string | [MarkupContent](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#markupContentInnerDefinition);

That said, I would not put any string on the client like "double click to insert",
instead I would suggest opening a feat request to the server to do that.

rchl · 2023-01-22T21:31:50Z

That said, I would not put any string on the client like "double click to insert",
instead I would suggest opening a feat request to the server to do that.

It's not the responsibly of the server to say that because server can't dictate how the UI should work. Whether editor should insert text on single, double click or even using keyboard.

What I was suggesting was to either set our text or append it to the existing text (in parenthesis for example).
That said, I don't think it's a great suggestion since once user is familiar with how those work, it will just be an annoyance. But I really think that it can be confusing otherwise, if the mouse pointer shows "hand" but nothing happens on single clicking.

predragnikolic · 2023-01-22T21:35:19Z

You have a point,
Will push a commit when I grab the time

plugin/tooling.py

plugin/inlay_hint.py

plugin/tooling.py

plugin/inlay_hint.py

Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>

…tooltip message

jwortmann · 2023-01-23T15:45:39Z

plugin/inlay_hint.py

            })
            value += '<a href="{command}">'.format(command=inlay_hint_click_command)
        value += html.escape(label_part['value'])
        if is_clickable:
            value += "</a>"
        # InlayHintLabelPart.location is not supported
        result += "<span title=\"{tooltip}\">{value}</span>".format(
-            tooltip=format_inlay_hint_tooltip(label_part.get("tooltip")),
+            tooltip=tooltip + '(Double-click to execute)' if is_clickable else tooltip,


There is currently no space between the original tooltip and the addition. So it will look like Original tooltip(Double-click to execute).

I'm also not confident that this addition is really needed. Instead we could add a sentence about it in the LSP docs, that you can insert inlay hints with double click (if supported by the server).

Or alternatively, use "Double click to insert" only if there is no original tooltip from the server.

Or alternatively, use "Double click to insert" only if there is no original tooltip from the server.

I'm not sure about this distinction because server is not gonna say how it can be inserted so it can still be a case that is confusing.

But I guess I'll give up here as it's not that important.

My idea was that if the server provides a tooltip, then the available space is already taken, because "Server's tooltip text (Double-click to insert)" looks a bit ugly and we don't know what the server uses as the tooltip text (maybe it could even result in more confusing confusing combinations with the server's tooltip text). And with the mouse cursor being the hand/finger icon, users should still easily find out that you can double-click the inlay hint (if supported by the server), so this tooltip hint probably doesn't provide a lot of value.

Honestly I don't agree with the statement that "with the mouse cursor being the hand/finger icon, users should still easily find out that you can double-click the inlay hint". At least when talking about myself, I'm not accustomed to having to double click when cursor uses a hand icon. I can not think of any other app/place where that would be the case. Certainly not in browsers with which most people are familiar with.

I have tried this change with mocked, clickable payload just a while ago and the behavior even confused me since I've clicked and I didn't see any reaction so I've started looking through the document to check if anything has changed. For people that didn't see this PR or are not that familiar with what inline hints are supposed to do on clicking it will be even more confusing.

Maybe I can suggest something else - no extra text on the tooltip but instead show a message in the status field if the lsp_on_double_click timeouts on a single click?

Hm, okay I could indeed not find any example of another application where you need to double click when the hand icon is shown. ST's minihtml doesn't support the CSS cursor property, though.

I searched a bit on GitHub how inlay hint tooltips are used, and VSCode seems to render a horizontal separator between the tooltip and the double click message:

We cannot do that, because ST's tooltips don't support HTML markup, but it seems that you can force a linebreak by including \n in the "title" string:

So maybe we could use that, i.e. always show the "Double click to insert" message, but if the server also provides a tooltip, then use server_tooltip + "\nDouble click to insert". Not sure if that would be a better alternative than just a space.

Also servers can use MarkupContent for the tooltip, for example see https://github.com/sjsone/vscode-neos-fusion-lsp/blob/80ea71b2d23c05abbcf47c51711888669b915be4/server/src/languageFeatures/InlayHintLanguageFeature.ts#L43-L51 where it's used for a fenced code block. That would be rendered as literal characters in ST...

I would be fine with a newline but also I'm having second thoughts about using title for that. It's quite shouty in ST being all yellow and weird looking. It might seem a little too much for users of servers that set text edits. And user just needs to be educated about it and not necessarily shown on every occasion when the inlay hint is hovered. In that sense I think my suggestion about showing status text on timeout might make sense.

being all yellow and weird looking

It's controlled by the theme, and has a white background in the Default, Default Dark and Adaptive themes. For the beautiful tooltips from my second screenshots, try out my "Brackets" theme :)

But I'm fine with trying out the status message on timeout. The implementation should be made generic in that case, i.e. I would pass another optional argument timeout_message: str = "" for the lsp_on_double_click command, so that it is only shown if this message is defined.

For now I just pushed a commit to always append the instruction text if a inlay hint contains a command or text edit.
That way the user will know if the inlay hint will insert text, or if a command will be executed as part of double clicking phantoms.

I also tough to completely remove the instruction text from the title and just put that info in the release notes and tell users that they have to double click phantoms. (note only one server supports text edits for inlay hints, and not many people use it, don't know any server that supports commands... so I would not put to much effort and thoughts into this).

plugin/tooling.py

…lse ""

…args) are passed

This is not necessary, but in case the arguments is a big dict it would be nice to clean that up

rchl · 2023-01-24T20:00:19Z

I've actually now realized that the inlay hint can include a command and that is not necessarily gonna insert anything. We should probably differentiate those cases and only include the text when there is a text edit present.

predragnikolic · 2023-01-24T21:14:58Z

inlay hint can include a command and that is not necessarily gonna insert anything

Regarding tooltip text for commands.

If we look at inlayHints.label: InlayHintLabelPart[]), we will see that a InlayHintLabelPart may contain a command.

This PR will show the following text in the title tooltip when iterating over InlayHintLabelParts:

InlayHintLabelPart.tooltip if exist
else "Double-click to execute" if InlayHintLabelPart.command exist
else it will not display anything

Regarding tooltip text for text edits.

This PR will show the following text in the title tooltip if an inlay hint contains textEdits:

InlayHint.tooltip if exist
else "Double-click to insert" if InlayHint.textEdits exist
else it will not display anything

We should probably differentiate those cases and only include the text when there is a text edit present.

I would say that we are handling those cases,
but maybe I'm missing something that you have concluded.

jwortmann · 2023-01-24T22:28:45Z

plugin/inlay_hint.py

            })
            result += '<a href="{command}">'.format(command=inlay_hint_click_command)
-        result += html.escape(label)
+        result += '<span title="{tooltip}">{value}</span>'.format(
+            tooltip=tooltip or 'Double-click to insert' if is_clickable else "",


I'm not 100% sure, but I think this is not exactly right, because or has higher precedence than if ... else - https://docs.python.org/3/reference/expressions.html#operator-precedence

So this is currently the same as (tooltip or 'Double-click to insert') if is_clickable else "", but we actually want tooltip or ('Double-click to insert' if is_clickable else "")

Because what we want (I think):

if is_clickable, and server provides a tooltip, we show the tooltip

if is_clickable, but server doesn't provide a tooltip, we show "Double-click to insert"

if not clickable, but server provides a tooltip, we show the tooltip (perhaps a possible case for this would be something like label is : bool and tooltip is Boolean value)

if not clickable, and server doesn't provide a tooltip, we show no tooltip at all

(but I have not tested with any language server, so please check whether what I wrote makes sense)

There is another similar place below, for the label parts.

You could also consider to not set the title attribute at all, if tooltip is "" (empty). That would probably make the code a bit more complex, for example by building the result HTML string step by step. But feel free to ignore if <span title=""> is fine.

So this is currently the same as (tooltip or 'Double-click to insert') if is_clickable else "", but we actually want tooltip or ('Double-click to insert' if is_clickable else "")

You are right.
will push a fix

currently it was `(tooltip or 'Double-click to insert') if is_clickable else ""` but I wanted `tooltip or ('Double-click to insert' if is_clickable else "")`

…_command

reintroduce is_clickable

plugin/inlay_hint.py

* main: (80 commits) Perform inlay hint action on double instead of single click (#2175) support canceling pending completions request (#2177) Implement type hierarchy request (#2180) fix stale state or lack of updates on changing branches (#2182) Add timestamp and request duration in LSP logs (#2181) don't show diagnostics panel on save by default (#2179) trigger "on_server_response_async" also for "initialize" response (#2172) feat(API): allow setting additional text in permanent server status (#2173) Implement call hierarchy request (#2151) workaround for View Listeners not being attached for new transient view (#2174) Make Document Symbols behavior more consistent with built-in Goto Symbol (#2166) docs: fsautocomplete config - remove redundant argument (#2165) Allow missing window/workDoneProgress/create request from the server (#2159) Use "plaintext" language ID for plain text files (#2164) Improve type annotations (#2162) Don't use "escapeall" extension when formatting with mdpopups (#2163) Cut 1.21.0 Fix inlay hint parts wrapping into multiple lines (#2153) Ensure commands triggered from minihtml run on correct view (#2154) Add "Source Action" entry to the "Edit" main menu (#2149) ...

predragnikolic added 3 commits January 22, 2023 20:48

feat: add double click command

a8df5af

refactor: LspClick that accepts a (click)count instead of LspDoubleClick

c4deab1

fix lint

01a650f

refactor

d6ddc6e

rchl reviewed Jan 22, 2023

View reviewed changes

plugin/click.py Outdated Show resolved Hide resolved

plugin/click.py Outdated Show resolved Hide resolved

plugin/click.py Outdated Show resolved Hide resolved

predragnikolic added 2 commits January 22, 2023 21:42

remove ()

5c0e9c5

rename to LspDoubleClick, remove count and move to tooling

dea82b9

rchl reviewed Jan 22, 2023

View reviewed changes

plugin/tooling.py Outdated Show resolved Hide resolved

plugin/tooling.py Outdated Show resolved Hide resolved

WIP experiment toolitp

3c7f07c

rchl reviewed Jan 22, 2023

View reviewed changes

plugin/inlay_hint.py Outdated Show resolved Hide resolved

predragnikolic added 6 commits January 22, 2023 23:20

show Double Click + tooltip if clickable else show tooltip

4460481

rename to LspOnDoubleClickCommand

f79da71

move to class method

c46b643

show tooltip content first than "Double Click)" if clickable

6eff3ec

lint

d512ca5

Merge branch 'main' into lsp_double_click

027feed

rchl reviewed Jan 23, 2023

View reviewed changes

plugin/tooling.py Outdated Show resolved Hide resolved

plugin/inlay_hint.py Outdated Show resolved Hide resolved

predragnikolic and others added 4 commits January 23, 2023 16:09

Update plugin/tooling.py

a5ab27a

Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>

remove tooltip title that is not necessary and be more clear for the …

af4253c

…tooltip message

Change "Double-click to run" to "Double-click to execute"

216eabf

Double-click to insert

bc5fd33

jwortmann reviewed Jan 23, 2023

View reviewed changes

rchl reviewed Jan 23, 2023

View reviewed changes

plugin/tooling.py Outdated Show resolved Hide resolved

rchl reviewed Jan 23, 2023

View reviewed changes

plugin/tooling.py Outdated Show resolved Hide resolved

rename to LspOnDoubleClickCommand

128682e

predragnikolic and others added 4 commits January 23, 2023 19:40

show tooltip, or show "Double-click to insert/execute" if clickable e…

6e48d6a

…lse ""

remove classmethod and only increase counter if the same command(and …

17590ab

…args) are passed

also reset prev_command and prev_args when

ac053af

This is not necessary, but in case the arguments is a big dict it would be nice to clean that up

order

3660505

rchl changed the title ~~Add lsp_click command~~ Insert inlay hint text edits on double instead of single click Jan 24, 2023

rchl approved these changes Jan 24, 2023

View reviewed changes

jwortmann reviewed Jan 24, 2023

View reviewed changes

predragnikolic added 4 commits January 26, 2023 23:55

fix condition

c032bef

currently it was `(tooltip or 'Double-click to insert') if is_clickable else ""` but I wanted `tooltip or ('Double-click to insert' if is_clickable else "")`

rename is_clicable to something more explicit: has_text_edits and has…

b6d3726

…_command

forgot to add "or can_resolve_inlay_hint" in a condition bellow

03472f8

reintroduce is_clickable

always show instructions if the inlay hint has a command or text edits

ea8df03

rchl reviewed Jan 30, 2023

View reviewed changes

plugin/inlay_hint.py Outdated Show resolved Hide resolved

strip tooltip content

3d866c4

rchl approved these changes Jan 30, 2023

View reviewed changes

rchl merged commit e1d9864 into main Jan 30, 2023

rchl deleted the lsp_double_click branch January 30, 2023 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert inlay hint text edits on double instead of single click #2175

Insert inlay hint text edits on double instead of single click #2175

predragnikolic commented Jan 22, 2023

predragnikolic commented Jan 22, 2023

rchl commented Jan 22, 2023 •

edited

Loading

predragnikolic commented Jan 22, 2023

rchl commented Jan 22, 2023 •

edited

Loading

predragnikolic commented Jan 22, 2023

jwortmann Jan 23, 2023

predragnikolic Jan 23, 2023

rchl Jan 24, 2023

jwortmann Jan 24, 2023

rchl Jan 27, 2023 •

edited

Loading

jwortmann Jan 27, 2023

rchl Jan 28, 2023 •

edited

Loading

jwortmann Jan 28, 2023

predragnikolic Jan 29, 2023

rchl commented Jan 24, 2023

predragnikolic commented Jan 24, 2023

jwortmann Jan 24, 2023

predragnikolic Jan 26, 2023

Insert inlay hint text edits on double instead of single click #2175

Insert inlay hint text edits on double instead of single click #2175

Conversation

predragnikolic commented Jan 22, 2023

predragnikolic commented Jan 22, 2023

rchl commented Jan 22, 2023 • edited Loading

predragnikolic commented Jan 22, 2023

rchl commented Jan 22, 2023 • edited Loading

predragnikolic commented Jan 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl commented Jan 24, 2023

predragnikolic commented Jan 24, 2023

Regarding tooltip text for commands.

Regarding tooltip text for text edits.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl commented Jan 22, 2023 •

edited

Loading

rchl commented Jan 22, 2023 •

edited

Loading

rchl Jan 27, 2023 •

edited

Loading

rchl Jan 28, 2023 •

edited

Loading