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

Intercept webbrowser open requests for local html files as well as localhost #4178

Merged
merged 4 commits into from
Jul 30, 2024

Conversation

nstrayer
Copy link
Contributor

Addresses #4118

Hooks into the new 'show-html-file' comm path to display html files opened with the webbrowser python module in the viewer pane.

Previously we overrode the webbrowser opening to forward localhost servers, so this just adds a condition to check for html files and do a similar thing.

Here's an example of it working on a bokeh plot that previously used to open in an external browser:
image

Rough edges:

  • Bokeh widget doesn't seem to care about the size of the viewer pane.
  • Not sure if how I am checking for the request being an html file is robust enough. I dont think there's an equivalent URI class in python that we can use, right?

QA Notes

@isabelizimm
Copy link
Contributor

Not sure if how I am checking for the request being an html file is robust enough. I dont think there's an equivalent URI class in python that we can use, right?

You could use urlparse from Python's urllib, which returns a named tuple, and check for the file schema and that path ends with .htm/html, but it does not do any sort of validation, if that is what you consider to be robustness.

Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

Confirmed that this works locally, nice!

  1. I agree with Isabel's recommendation for urlparse.

  2. Could you also please update the test_viewer_webbrowser_does_not_open unit test to include this case? (Maybe the test should be renamed as well.)

  3. TODO: Figure out if the file being displayed is a plot or not.

    Any ideas for how to determine this?

@nstrayer nstrayer force-pushed the send-webbrowser-file-open-to-viewer branch from c5f77c8 to 08670f8 Compare July 30, 2024 17:24
@nstrayer
Copy link
Contributor Author

@seeM ,
1 + 2 (done)
3. Not sure, I think it will depend on what we decide about how we differentiate between the two.

@nstrayer nstrayer requested a review from seeM July 30, 2024 17:27
@nstrayer
Copy link
Contributor Author

@isabelizimm , thanks! Used urlparse in f7a0011

```

Run Positron's unit tests with [pytest](https://docs.pytest.org/en/8.0.x/):

```sh
pytest python_files/positron/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, did pytest not work for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was trying to use a different python version than which python was giving me. Wasim suggested this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

My thinking was that python -m pytest should work in all cases where pytest works, and more. So it'll probably save folks some headaches.

@nstrayer nstrayer merged commit 67eb32a into main Jul 30, 2024
22 checks passed
@nstrayer nstrayer deleted the send-webbrowser-file-open-to-viewer branch July 30, 2024 21:59
@github-actions github-actions bot locked and limited conversation to collaborators Jul 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants