Add Copy action to hover popup to copy plaintext to clipboard#2885
Add Copy action to hover popup to copy plaintext to clipboard#2885kylebebak wants to merge 2 commits into
Conversation
- Append a `Copy` link after `Rename` in the hover actions row, wired to the existing `LspCopyTextCommand` - Add `hover_plaintext()` on `LspHoverCommand` to source plaintext directly from the LSP `Hover.contents` value, preserving spacing and line breaks without round-tripping through HTML - Add `plaintext_from_lsp_content()` to handle all three LSP content shapes (`MarkedString`, `MarkupContent`, `list[MarkedString]`) - Add `strip_single_code_fence()` to drop the surrounding ` ``` ` when the hover is a single fenced code block
|
This is pretty bad UX. A random copy button that copies some arbitrary block. Also fragile regexp and parsing content is not really the way to go. |
@rchl, it seems like you really didn't like this change. I don't know whether that's because you're opposed to the feature or the implementation. I'm guessing it's not the feature, given that you merged a PR with a similar feature here (copy buttons for diagnostics). So let's assume it's the implementation What would make the UX better? Specifically, what would make the Copy button less "random"? A copy icon like the one added in the PR I just mentioned? Where would you want that icon? Also, what would make the block less "arbitrary"? It's copying the full contents of the popup. If you don't like As for "parsing content", I assume you're referring to @jwortmann @rwols Is this typically how you guys respond to first-time contributors? |
|
I would just suggest discussing solution before implementing to avoid cases like this. It's just not a good UX and we knew already that there isn't really a good way of implementing it. Also:
|
|
Thanks to both of you for responding
It sounds like you guys have already thought about this in depth, and UX nits aside you would simply rather not implement this feature
This would mean you can't copy e.g. the class or function docstring that appears below the code block. Copying the docstring is useful IMO Also, I don't know whether it's possible to have multiple fenced code blocks in this hover panel. If it is, I don't think having e.g. multiple copy icons to the right of the code blocks, like the copy icons rendered for diagnostics, would be better UX than having a single Copy button/icon. In any case, based on @jwortmann 's comment, it's not really feasible to implement it that way I think this feature is both simple enough and useful enough to warrant inclusion in the plugin, and that the UX in my implementation is neither beautiful nor ugly. With some minor tweaks maybe you'd like it more. It gets the job done. Thanks again for your feedback |
|
Thanks for putting the effort into creating this PR. Usually PR don't go this way. So ideally, Sublime would allow selecting text in the popup. (You can follow this issue sublimehq/sublime_text#1649) That would be the best user experience. In the meantime, LSP had lots of discussion on how to implement the copy functionally in popups #2618 in a way that makes most sense. (Thus we limited the feature to only support the most common use case - copying diagnostic messages from the popup. Originally I did implement it to copy more sections in the popup, but due to ST API constraints and feedback, we decided to not do that - your PR is similar to something we already tried) To sum up, |


This PR adds a copy link/command/button to the hover popup, so users can copy the plaintext rendered by this popup to the clipboard
E.g. when hovering over the text in the screenshot and clicking copy, the following text is copied to the clipboard:
Related issues
Implementation details
Copylink afterRenamein the hover actions row, wired to the existingLspCopyTextCommandhover_plaintext()onLspHoverCommandto source plaintext directly from the LSPHover.contentsvalue, preserving spacing and line breaks without round-tripping through HTMLplaintext_from_lsp_content()to handle all three LSP content shapes (MarkedString,MarkupContent,list[MarkedString])strip_single_code_fence()to drop the surrounding```when the hover is a single fenced code block