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.

Sign up for GitHub

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

Jump to bottom

Add support for remote images in hover popups #2341

Merged
merged 2 commits into from
Oct 18, 2023
Merged

Add support for remote images in hover popups #2341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a69dfcc
Add support for remote images in hover popups
jwortmann Oct 17, 2023
eb9f1da
Fix indentation
jwortmann Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions plugin/hover.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
from .session_view import HOVER_HIGHLIGHT_KEY
from functools import partial
import html
import mdpopups
import sublime


Expand Down Expand Up @@ -99,6 +100,7 @@ class LspHoverCommand(LspTextCommand):
def __init__(self, view: sublime.View) -> None:
super().__init__(view)
self._base_dir = None # type: Optional[str]
self._image_resolver = None

def run(
self,
Expand Down Expand Up @@ -311,6 +313,13 @@ def _show_hover(self, listener: AbstractViewListener, point: int, only_diagnosti
location=point,
on_navigate=lambda href: self._on_navigate(href, point),
on_hide=lambda: self.view.erase_regions(HOVER_HIGHLIGHT_KEY))
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)
Comment on lines +316 to +322
Copy link
Member

@rchl rchl Oct 18, 2023 •
edited
Loading

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"?

Copy link
Member Author

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

LSP/plugin/documents.py

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.

Copy link
Member

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.

Copy link
Member Author

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.


def _on_navigate(self, href: str, point: int) -> None:
if href.startswith("subl:"):
Expand Down
13 changes: 13 additions & 0 deletions stubs/mdpopups.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,16 @@ def scope2style(
selected: bool = False,
explicit_background: bool = False
) -> str: ...


def worker_thread_resolver(
url: str,
done: Callable[[bytes], None]
) -> None: ...


def resolve_images(
minihtml: str,
resolver: Callable[[str, Callable[[bytes], None]], None],
on_done: Callable[[str], None]
) -> Optional[object]: ...