Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Truncate inlay hint label #2514

Merged
merged 42 commits into from
Nov 10, 2024
Merged

Feature: Truncate inlay hint label #2514

merged 42 commits into from
Nov 10, 2024

Conversation

bivashy
Copy link
Contributor

@bivashy bivashy commented Sep 15, 2024

Description

This pull request allows inlay hints to be truncated, which could be useful if inlay hints are too large. For example: TypeScript has absurdly long inlay hints (see screenshot 1).
The default truncate limit is 1000.
Truncated content reveal is not implemented, which means you cannot see the full content of the inlay hint if it is truncated.

Tested with:
LSP-typescript
LSP-gopls
LSP-rust-analyzer


1. Long inlay hint

long-inlay-hint

2. `LSP-typescript` (limit 30)

lsp-typescript-inlay-hint

3. `LSP-gopls` (limit 30)

lsp-gopls

4. `LSP-rust-analyzer` (limit 30)

lsp-rust-analyzer

plugin/inlay_hint.py Outdated Show resolved Hide resolved
plugin/inlay_hint.py Outdated Show resolved Hide resolved
LSP.sublime-settings Outdated Show resolved Hide resolved
@jwortmann
Copy link
Member

jwortmann commented Sep 17, 2024

Could you use instead of ...? I think that would look nicer.

I wonder whether this really needs a setting, maybe long labels could instead always be truncated if we pick a reasonable cutoff length? And if the label gets truncated, it could be added to the tooltip, so you can easily see the full inlay hint on mouse hover. So in that case the html.escape of the full label and probably also a \n (or <br>?) should be prepended to the title attribute at

result += f'<span title="{(tooltip + instruction_text).strip()}">{html.escape(label)}</span>'

and
result += f"<span title=\"{(tooltip + instruction_text).strip()}\">{value}</span>"

Could be useful to try whether that works and how it would look like.

@bivashy
Copy link
Contributor Author

bivashy commented Sep 19, 2024

Could you use instead of ...? I think that would look nicer.

Maybe we should make this configurable from the preferences? Or is this redundant?


And if the label gets truncated, it could be added to the tooltip, so you can easily see the full inlay hint on mouse hover. So in that case the html.escape of the full label and probably also a \n (or &lt;br&gt;?) should be prepended to the title attribute at

result += f'<span title="{(tooltip + instruction_text).strip()}">{html.escape(label)}</span>'

and

result += f"<span title=\"{(tooltip + instruction_text).strip()}\">{value}</span>"

Could be useful to try whether that works and how it would look like.

I'll try to implement that.

I am not sure how we would choose a reasonable length, because it might vary by language and user preference, someone might be fine with big inlay hints, but other people would want to have very short inlay hints.

@rchl
Copy link
Member

rchl commented Sep 19, 2024

Could you use instead of ...? I think that would look nicer.

Maybe we should make this configurable from the preferences? Or is this redundant?

We strive to limit the number of exposed options so no. But also is the correct one to use semantically. Three separate dots don't mean much semantically. And also it's slightly shorter.

@bivashy
Copy link
Contributor Author

bivashy commented Sep 19, 2024

I've used suggested characters instead of plain ..., I think it looks nice.

more-compact-dots


truncation-tooltip

plugin/inlay_hint.py Outdated Show resolved Hide resolved
plugin/inlay_hint.py Outdated Show resolved Hide resolved
Copy link

netlify bot commented Sep 22, 2024

Deploy Preview for sublime-lsp ready!

Name Link
🔨 Latest commit 1ba72a6
🔍 Latest deploy log https://app.netlify.com/sites/sublime-lsp/deploys/66f045ef1176bd0008be3e70
😎 Deploy Preview https://deploy-preview-2514--sublime-lsp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 22, 2024

Deploy Preview for sublime-lsp ready!

Name Link
🔨 Latest commit 1c313b5
🔍 Latest deploy log https://app.netlify.com/sites/sublime-lsp/deploys/66f04600868a650008791ad6
😎 Deploy Preview https://deploy-preview-2514--sublime-lsp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 22, 2024

Deploy Preview for sublime-lsp ready!

Name Link
🔨 Latest commit 8e86dc3
🔍 Latest deploy log https://app.netlify.com/sites/sublime-lsp/deploys/6730d04c54b4ae0008299b68
😎 Deploy Preview https://deploy-preview-2514--sublime-lsp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

plugin/inlay_hint.py Outdated Show resolved Hide resolved
plugin/inlay_hint.py Outdated Show resolved Hide resolved
plugin/inlay_hint.py Outdated Show resolved Hide resolved
plugin/inlay_hint.py Outdated Show resolved Hide resolved
@jwortmann
Copy link
Member

I think a better default length would be something like 30 or 40, and I would probably name the setting "inlay_hints_truncate_length" or "inlay_hints_max_length" (just a suggestion).

@bivashy bivashy requested a review from rchl October 22, 2024 17:29
plugin/inlay_hint.py Outdated Show resolved Hide resolved

remaining_truncate_limit = truncate_limit
full_label = "".join(label_part['value'] for label_part in label)
full_label_truncated = len(full_label) >= truncate_limit and truncate_limit > 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also be > here or not?


I have tested the PR with a server that uses the regular label (no label parts) and I think it works well with the tooltips.

My only suggestion left would be to use a lower default value for the setting. I think 30 would be a sane default. I would say the truncation is actually a good thing to do, and some users might not check settings or read update notes, so it should be applied by default. Opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also be > here or not?

I have tested the PR with a server that uses the regular label (no label parts) and I think it works well with the tooltips.

My only suggestion left would be to use a lower default value for the setting. I think 30 would be a sane default. I would say the truncation is actually a good thing to do, and some users might not check settings or read update notes, so it should be applied by default. Opinions?

I'll check it out. Thanks for pointing that out.


I think inlay hint is not enabled by default, so most users will not be affected. So I agree that this feature should be enabled by default. I'll wait for more opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check it out. Thanks for pointing that out.

Done in d6a544a.


Done in a43e68a

I will not resolve this conversation in case of reconsideration, should I?

sublime-package.json Outdated Show resolved Hide resolved
sublime-package.json Outdated Show resolved Hide resolved
@@ -180,6 +180,9 @@
// or a custom keybinding.
"show_inlay_hints": false,

// Maximum length for inlay hints. Truncates exceeding characters and adds an ellipsis. Zero or less means no truncation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A linebreak would be nice here. Less than zero would violate the JSON schema btw.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a small change for the description.

Is there anything more or can we merge the PR? (I have tested only for the regular inlay hints without label parts)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a small change for the description.

Is there anything more or can we merge the PR? (I have tested only for the regular inlay hints without label parts)

I think it is ready to use. I don't have any more changes.


Thanks to all reviewers for their patience!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A linebreak would be nice here. Less than zero would violate the JSON schema btw.

Linebreak after each sentence, if I understand correctly

@rchl rchl merged commit 2f37a49 into sublimelsp:main Nov 10, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants