Skip to content

Conversation

@predragnikolic
Copy link
Member

@predragnikolic predragnikolic commented Aug 16, 2025

This is a POC that could address #2600

Sublime.Text.mp4

This enables coping hover responses from server,
and diagnostics text, at the moment.

TODO:

  • Error diagnostics look broken, need to fix that.()
Screenshot 2025-08-16 at 11 14 27 -[] Add logic to copy signature help as well.

closes #2600

Copy link
Member

@rchl rchl left a comment

Choose a reason for hiding this comment

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

What about a case when the popup also shows other info like symbol documentation? Is the whole contents of the popup copied or each section is copied separately. If the latter then it would probably be hard to convey what is going to get copied.

@predragnikolic
Copy link
Member Author

predragnikolic commented Aug 17, 2025

or each section is copied separately

Yes, it is the later.
And it is visually hard to distinguish what gets copied sometimes. (I did enable pyright, basepyrigh and pylsp to try out this use case). It is a limitation I am not sure how to address yet.

@netlify
Copy link

netlify bot commented Sep 3, 2025

Deploy Preview for sublime-lsp ready!

Name Link
🔨 Latest commit ab65e90
🔍 Latest deploy log https://app.netlify.com/projects/sublime-lsp/deploys/68b87f1df926670008c069f2
😎 Deploy Preview https://deploy-preview-2618--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 project configuration.

@predragnikolic
Copy link
Member Author

The functionality is implemented and it should work fine.
I just need to fix the failing tests, before I mark the PR for review.


I have problems fixing the signature help tests.

I did update copy_text_html from:

def copy_text_html(html_content: str, copy_text: str | MarkupContent | MarkedString | list[MarkedString]) -> str:
    copy_text = _markup_to_string(copy_text)
    if not len(copy_text):
        return html_content
    return f"""<a title="Click to Copy"
       style='text-decoration: none; display: block; color: inherit'
       href='{sublime.command_url('lsp_copy_text', {
        'text': copy_text.replace(' ', ' ')
       })}'>{html_content}</a>"""

to: (to remove new lines)

def copy_text_html(html_content: str, copy_text: str | MarkupContent | MarkedString | list[MarkedString]) -> str:
    copy_text = _markup_to_string(copy_text)
    if not len(copy_text):
        return html_content
    return f"""<a title="Click to Copy" style='text-decoration: none; display: block; color: inherit' href='{sublime.command_url('lsp_copy_text', {
        'text': copy_text.replace(' ', ' ')
       })}'>{html_content}</a>"""

and I have updated the regex to be:

 r'''
+ <a [^>]+>
 <div class="highlight"><pre>
 <span style="color: #\w{6}">RUN </span>
 <span style="color: #\w{6}">\[</span>
 <span style="color: #\w{6}"> </span>
 <span style="color: #\w{6}">"command"</span>
 <span style="color: #\w{6}"> </span>
 <span style="color: #\w{6}; font-weight: bold; text-decoration: underline">"parameters"</span>
 <span style="color: #\w{6}">, </span>
 <span style="color: #\w{6}">\.\.\.</span>
 <span style="color: #\w{6}"> </span>
 <span style="color: #\w{6}">\]</span>
- </pre></div>
+ </pre></div></a>
 '''

But this does not work as I still get this error:

AssertionError: Regex didn't match: '<a  title="Click to Copy" style=\'text-decoration: none; display: block; color: inherit\' href=\'subl:lsp_copy_text {&quot;text&quot;:&quot;RUN [ \\&quot;command\\&quot; \\&quot;parameters\\&quot;, ... ]&quot;}\'><div class="highlight"><pre><span style="color: #\\w{6}">RUN </span><span style="color: #\\w{6}">\\[</span><span style="color: #\\w{6}"> </span><span style="color: #\\w{6}">"command"</span><span style="color: #\\w{6}"> </span><span style="color: #\\w{6}; font-weight: bold; text-decoration: underline">"parameters"</span><span style="color: #\\w{6}">, </span><span style="color: #\\w{6}">\\.\\.\\.</span><span style="color: #\\w{6}"> </span><span style="color: #\\w{6}">\\]</span></pre></div></a>' not found in '<a title="Click to Copy" style=\'text-decoration: none; display: block; color: inherit\' href=\'subl:lsp_copy_text {&quot;text&quot;:&quot;RUN [ \\&quot;command\\&quot; \\&quot;parameters\\&quot;, ... ]&quot;}\'><div class="highlight"><pre><span style="color: #ffffff">RUN </span><span style="color: #8c8a90">[</span><span style="color: #ffffff"> </span><span style="color: #8c8a90">"command"</span><span style="color: #ffffff"> </span><span style="color: #f6ac90">"parameters"</span><span style="color: #ffffff">, </span><span style="color: #8c8a90">...</span><span style="color: #ffffff"> </span><span style="color: #8c8a90">]</span></pre></div></a>'

I will try to fix the failing tests on another day.
If someone want to take a look at the failing tests in the mean while, feel free. Any help is welcome.

copy_text_html returns a block element,
before this commit, with the previous code, I moved the diagnostics source to be bellow the diagnostic message
now the diagnostic message and source will be on the same line and look as before
@predragnikolic
Copy link
Member Author

I probably closed the PR by accident (probably pressed the keyboard shortcut)

Seems like this code cause <a><div>...</a></div> to be rendered
and I suspect that the change to _render_label was the cause of it. F
For now I will revert it
@predragnikolic predragnikolic force-pushed the feature/copy_hover_content branch from 8f76525 to 18b26db Compare September 10, 2025 19:16
@predragnikolic predragnikolic marked this pull request as ready for review September 12, 2025 21:43
@predragnikolic
Copy link
Member Author

I pushed 18b26db because the generated HTML looked wrong -> "

...
" (a seemed to be closed before the div)... but it was my lack of concentration. The generated html was OK.

Because I wanted to support signature as well,
I dig into the issue with signature help and discovered that the regexes in tests I had locally were not right. So I just updated the tests.


Now, I consider this PR read for review.

return " ".join(content) # pyright: ignore[reportCallIssue, reportUnknownVariableType, reportArgumentType]


def copy_text_html(html_content: str, copy_text: str | MarkupContent | MarkedString | list[MarkedString]) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

This naming makes it hard to understand the purpose IMO.

Perhaps wrap_in_copy_to_clipboard_link with arguments link_text and text_to_copy?

And maybe the _markup_to_string should also be done separately so that this function is more specialized and less do-it-all.

predragnikolic and others added 5 commits September 13, 2025 11:37
Co-authored-by: Rafał Chłodnicki <[email protected]>
…content on why I've added it...

For now I will remove it, if there is a need it can easily be brought back
Comment on lines 851 to 855
return f"""<a title="Click to Copy"
style='text-decoration: none; display: block; color: inherit'
href='{sublime.command_url('lsp_copy_text', {
'text': copy_text
})}'>{html_content}</a>"""
Copy link
Member

@rchl rchl Sep 13, 2025

Choose a reason for hiding this comment

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

Seems to create an extra padding in the popups (the bottom one should be the same as all other):

Image

I have not looked into it too much. Perhaps there is an empty a element there?
Or perhaps it has some margin collapsing bug which ST is known for.

Copy link
Member Author

@predragnikolic predragnikolic Sep 13, 2025

Choose a reason for hiding this comment

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

Seems to be a

When I hover:

const foo = "bar"

the html content with this branch is:

mdpopups: =====HTML OUTPUT=====
mdpopups: 
<body><a title="Click to Copy"
       style='text-decoration: none; display: block; color: inherit'
       href='subl:lsp_copy_text {&quot;text&quot;:&quot;\n```typescript\nconst foo: \&quot;bar\&quot;\n```\n&quot;}'><div class="highlight"><pre><span style="color: #fcb6b6;">const</span><span style="color: #ffffff;"> </span><span style="color: #ffffff;">foo</span><span style="color: #ffffff;">:</span><span style="color: #ffffff;"> </span><span style="color: #fcd49e;">"</span><span style="color: #fcd49e;">bar</span><span style="color: #fcd49e;">"</span><br></pre></div></a><div class="lsp_popup--spacer"></div></body>

while the html with the main branch still contains a br

mdpopups: =====HTML OUTPUT=====
mdpopups: 
<body><div class="highlight"><pre><span style="color: #fcb6b6;">const</span><span style="color: #ffffff;"> </span><span style="color: #ffffff;">foo</span><span style="color: #ffffff;">:</span><span style="color: #ffffff;"> </span><span style="color: #fcd49e;">"</span><span style="color: #fcd49e;">bar</span><span style="color: #fcd49e;">"</span><br></pre></div><div class="lsp_popup--spacer"></div></body>

but there is margin bottom on main.

I will see what is going on.

@predragnikolic predragnikolic force-pushed the feature/copy_hover_content branch from 4b798e8 to 1ae508b Compare September 13, 2025 09:55
@jwortmann
Copy link
Member

jwortmann commented Sep 13, 2025

In my opinion the copying should be restricted to diagnostics messages and to the content of (fenced) code blocks.
I don't think that the current choice of just copying the entire hover content as Markdown is extremely useful, and I don't like that if you accidentally click on a hover popup, you will lose your previous clipboard content. Also the tooltip is misleading (wrong) when you hover over links in the hover content. If restricting the copy functionality to code blocks is not possible, then I would move this functionality to an additional link "copy" in the symbol action links at the bottom of the popup.

@predragnikolic
Copy link
Member Author

While copying content from the whole hover popup looked useful, this comment made me rethink it:

In my opinion the copying should be restricted to diagnostics messages and to the content of (fenced) code blocks.
I don't think that the current choice of just copying the entire hover content as Markdown is extremely useful, and I don't like that if you accidentally click on a hover popup, you will lose your previous clipboard content. Also the tooltip is misleading (wrong) when you hover over links in the hover content. If restricting the copy functionality to code blocks is not possible, then I would move this functionality to an additional link "copy" in the symbol action links at the bottom of the popup.

So, I've removed coping content from most hover popups,
but I only left coping diagnostics and tweaked the UI (there is a copy button now)
Screenshot 2025-10-15 at 21 13 32

Would you still find this PR useful if it only adds coping the diagnostics in the hover popup?

I know I've reduced the scope of the PR,
but any approach I try to come up to allow copying text in popup will not match the UX that we could have if ST supported selecting text from hover popups natively.

@jwortmann
Copy link
Member

Would you still find this PR useful if it only adds coping the diagnostics in the hover popup?

Yes, I like it. Would it be possible to make the copy button aligned to the right side of the popup?

Alternatively we could use icons instead of the "copy" button. For example:

clipboard clipboard@2x clipboard@3x copy copy@2x copy@3x

It is possible to include the images in minihtml via res:// URLs like in

lightbulb_html = '<span class="lightbulb"><img src="res://Packages/LSP/icons/lightbulb_colored.png"></span>' \

The icons would probably also need a dark variant for light color schemes.

But the "copy" button looks good too. I'm just throwing in some ideas.


I think if we want a copy button for the fenced code blocks too in the future, we would need to parse the html from mdpopups after the conversion from Markdown and then inject additional html for the buttons. I think it's not impossible, but probably not very easy and there could be various edge cases. Also the minihtml is quite limited, so I don't know where the best placement for the copy buttons would be in that case. A better way might be if this could be handled directly by mdpopups. But probably this is out of scope for mdpopups, and an implementation there might also be complicated.

docs_copy

@predragnikolic
Copy link
Member Author

Just some small update regarding the image icon.

Would it be possible to make the copy button aligned to the right side of the popup?

I don't think it is possible with the current minihtml API.
That could be achieved, if minihtml supported, "position: absolute".

FWIW, when I try to put the icon closer to the text.
I didn't even manage to perfectly align the icon even in such case.
Minihtml CSS and HTML just behaves differently to what I expect it to behave.

I had a couple of failed attempts.
When I added the image,
the error box height changed so elements because miss-aligned
a

One of the ways to address it is to set inline-block on the image, that fixed the height of the error box, but the copy image disappeared...
b

Other attempts I tried didn't have success either.


I think I am gonna leave out adding the copy image, just because it causes the error box to look miss-aligned(as in the first image) and I have not found a way to make the error popup look aligned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable text selection and copying in LSP pop-up

4 participants