-
Notifications
You must be signed in to change notification settings - Fork 183
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
Add support for remote images in hover popups #2341
Conversation
self._image_resolver = mdpopups.resolve_images( | ||
contents, mdpopups.worker_thread_resolver, partial(self._on_images_resolved, contents)) | ||
|
||
def _on_images_resolved(self, original_contents: str, contents: str) -> None: | ||
self._image_resolver = None | ||
if contents != original_contents and self.view.is_popup_visible(): | ||
update_lsp_popup(self.view, contents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this could trigger an issue like:
- the popup is shown
- images start resolving
- the original popup is closed and a different one is opened
- the original resolve completes and replaces the new popup with original contents.
Should we have some safety measures to ignore resolved results that are "outdated"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably needs a larger refactoring for tracking popups then, see also
Lines 575 to 577 in e7cd48f
# TODO: Refactor popup usage to a common class. We now have sigHelp, completionDocs, hover, and diags | |
# all using a popup. Most of these systems assume they have exclusive access to a popup, while in | |
# reality there is only one popup per view. |
But since there seems to be image caching and a size cap implemented in mdpopups, I wonder how realistic such a situation would be. I would say self.view.is_popup_visible()
should be an adequate solution for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this code will ever trigger in my usage of LSP but I imagine that if someone deals with servers or content where this would trigger then it could suddenly become a fairly easy to trigger issue.
But I'm fine with not addressing it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a simple way to test it with Pyright:
def show_cat():
""" Cute cat photo on hover.
![](https://upload.wikimedia.org/wikipedia/commons/8/8e/Hauskatze_langhaar.jpg)
"""
pass
I guess it depends on your internet connection speed, but on my end the delay is negligible, in the way that I doubt you can open another popup in the meanwhile. The image size is ~1MB.
Fixes #1363.
I have tested it with a small modification to LSP-pyright:
The following is mentioned in the mdpopups docs, but it has been published more than two years ago, so I guess it's fine to add now: