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

Use httpgd NPM package #823

Merged
merged 14 commits into from
Nov 30, 2021
Merged

Use httpgd NPM package #823

merged 14 commits into from
Nov 30, 2021

Conversation

nx10
Copy link
Contributor

@nx10 nx10 commented Oct 16, 2021

What problem did you solve?

This is a first WIP attempt at replacing the client side API and connection code with the new httpgd NPM package to unify and simplify development.

This is also discussed in nx10/httpgd#79

This PR is a very early prototype, basic functionality is implemented, but there are still a lot of bugs. This is intended as a starting point for further development with @ManuelHentschel

Changes:

  • Replaced type definitions with imported types
  • Replaced API and connection handling code
  • Removed client side zooming (this is now properly handled server side)
  • Removed unique ID hack (portable SVG renderer is now used)

Bugs:

  • There seems to be a problem when more than one plot is drawn (duplicated invisible plots in history)
  • Plot view very small when opened
  • Text border is colored instead of the text itself when using theming

@ManuelHentschel
Copy link
Member

Thanks for the PR! I started working on the "Export plot as..." functionality but got stuck at the point where I actually need to get the content of the exported plot (here).

I've seen that you do this kind of "manually" here. Should I also do something like that, or is this supposed to be part of the httpgd-js api?

@ManuelHentschel
Copy link
Member

The latest push depends on the httpgd version from nx10/httpgd-js#1. To test this, I temporarily removed the httpgd dependency and use yarn link to import a local version of the package.

@ManuelHentschel
Copy link
Member

* There seems to be a problem when more than one plot is drawn (duplicated invisible plots in history)

The last commit hopefully fixes this. I think the bug became visible, because we removed the refresh timeout (the underlying bug was masked by this timeout). To avoid too frequent rerendering of the html while a new plot is drawn, I'd still recommend adding the timeout functionality again.

* Plot view very small when opened

In the .ejs files the property plot.svg needed to be renamed to plot.data, now it should look normal again.

* Text border is colored instead of the text itself when using theming

Can you please give an example for this?

@nx10
Copy link
Contributor Author

nx10 commented Oct 17, 2021

I just pushed a commit that streams the plot directly into the exported file. This should be more memory efficient and might be a bit faster. There seems to be a problem with eslint not recognizing the Response object correctly (eslint thinks this is the browser Response object and not the node-fetch one. This might be a cross-fetch problem or config problem in vscode-r I am not sure.

* Text border is colored instead of the text itself when using theming

Can you please give an example for this?

Nevermind, I think I was testing with an old version of vscode-R when I noticed this. Text looks good now.

Good job on the other two fixes as well.

Edit:

To avoid too frequent rerendering of the html while a new plot is drawn, I'd still recommend adding the timeout functionality again.

I agree.

src/plotViewer/index.ts Outdated Show resolved Hide resolved
@ManuelHentschel
Copy link
Member

Anything else that needs to be implemented/fixed, @nx10 @renkun-ken ?

@nx10
Copy link
Contributor Author

nx10 commented Nov 30, 2021

I think the restructuring went very well, thank you for your work.

At some point if somebody volunteered to add an article for the httpgd documentation would be great: https://nx10.github.io/httpgd/articles/b01_vscode.html

@renkun-ken
Copy link
Member

Thanks for the nice work!

As I test the httpgd plot viewer commands, it looks like "Toggle Preview Plot Layout" will randomly make the previews disappear rather than rearrange the layout. Not sure if you could reproduce it?

@ManuelHentschel
Copy link
Member

As I test the httpgd plot viewer commands, it looks like "Toggle Preview Plot Layout" will randomly make the previews disappear rather than rearrange the layout. Not sure if you could reproduce it?

That command cycles through three different layouts: horizontal scrolling, multi-row and hidden (i.e. the previews disappear)

@renkun-ken
Copy link
Member

That command cycles through three different layouts: horizontal scrolling, multi-row and hidden (i.e. the previews disappear)

OK. The hidden one seems not quite useful for me, as it is almost the same with full window mode.

@ManuelHentschel
Copy link
Member

OK. The hidden one seems not quite useful for me, as it is almost the same with full window mode.

I think this option was requested at some point, and it might be useful e.g. if your webview pane is rather tall and narrow, but you still don't want to see all your previous plots.

Copy link
Member

@renkun-ken renkun-ken left a comment

Choose a reason for hiding this comment

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

LGTM

@ManuelHentschel ManuelHentschel changed the title WIP use httpgd NPM package Use httpgd NPM package Nov 30, 2021
@ManuelHentschel ManuelHentschel merged commit f3194d0 into REditorSupport:master Nov 30, 2021
@nx10 nx10 deleted the httpgd branch September 21, 2022 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants