nx10 / httpgd

Asynchronous http server graphics device for R.
https://nx10.github.io/httpgd
GNU General Public License v2.0
377 stars 19 forks source link

Integrate with vscode-R #63

Open ManuelHentschel opened 3 years ago

ManuelHentschel commented 3 years ago

Hi @nx10 , thanks for this great package! @renkun-ken amazing to find yet another R repo that you are involved with :D

Would you be interested in working together to integrate an httpgd-based viewer in vscode-R?

In https://github.com/Ikuyadeu/vscode-R/pull/614 I managed to match a single plot to the color theme/font of the editor, but integrating the viewer provided by this package in a similar fashion seems to be a bit more difficult:

So far, I think the best solution would be to run the "brains" of the viewer (i.e. inst/www/httpgd.ts) inside the vscode-R extension and only have the js code required to make the viewer interactive run inside the webview. Some of the buttons (e.g. to export the plot as a file) could also be moved from the webview to a vscode-menu. In order to match the plots' theme to vscode, it might also be helpful to have a "minimal-styling" mode in the httpgd-package and leave the styling to the webview.

See also: https://github.com/Ikuyadeu/vscode-R/pull/620

nx10 commented 3 years ago

Hi @ManuelHentschel yes, I had quick look at your PR yesterday already, thanks for all of your work so far! I am a bit busy this week, so it took me some time to look into it further.

Would you be interested in working together to integrate an httpgd-based viewer in vscode-R?

Absolutely!

In Ikuyadeu/vscode-R#614 I managed to match a single plot to the color theme/font of the editor, [...]

Your approach of styling the SVGs with CSS to match the editor theme is very interesting. The extra_css attribute was originally intended to embed or link web fonts in the SVGs, but I left it open as a plain string to allow for exactly such experiments as your editor theme matching. However, I am unsure how consistent this will work with colorful plots, and more complex plots (eg. ggplot2). If the main reason for styling is the dark theme, maybe a filter will work better. (Something like: html { filter: invert(100%) hue-rotate(180deg) } to invert the luminescence could be interesting.) The "cleaner" (a bit less flexible) solution would be to set the background color and font with the base R graphics functions (par(...)). This would also allow users to override the settings from within R.

[...] So far, I think the best solution would be to run the "brains" of the viewer (i.e. inst/www/httpgd.ts) inside the vscode-R extension and only have the js code required to make the viewer interactive run inside the webview. Some of the buttons (e.g. to export the plot as a file) could also be moved from the webview to a vscode-menu. In order to match the plots' theme to vscode, it might also be helpful to have a "minimal-styling" mode in the httpgd-package and leave the styling to the webview.

I think you are right. The httpgd.ts/js was intended for this exact use case, but the index.html grew a lot in the last couple of updates with the development of the plot history sidebar, and other UI improvements. I think it would feel a lot more "vscode-native" if we moved most/all of the menu to a vscode menu. Have you tried setting the hgd(..., cors=T) attribute? That might help with resource permissions.

I am very excited this is finally happening. As I said this week is very busy for me, but after the weekend I will have a look at the integration. I am unfamiliar with the vscode plugin API, so its great to have you on board. Feel free to ask if you run into any problems with the httpgd code. I will keep this issue open for discussion and to track our progress.

ManuelHentschel commented 3 years ago

However, I am unsure how consistent this will work with colorful plots, and more complex plots (eg. ggplot2). If the main reason for styling is the dark theme, maybe a filter will work better. (Something like: html { filter: invert(100%) hue-rotate(180deg) } to invert the luminescence could be interesting.) The "cleaner" (a bit less flexible) solution would be to set the background color and font with the base R graphics functions (par(...)). This would also allow users to override the settings from within R.

I definitely agree on this one. I think the CSS styles look quite nice for simple plots (and e.g. the responsiveness to color theme changes looks cool), but for people who want to draw more complex, reproducible plots, a filter would work better. The best option is probably to just make this configurable by the user.

Have you tried setting the hgd(..., cors=T) attribute? That might help with resource permissions.

Yes, that setting actually resolved one of the three different error messages I was getting. But overall everything seems to work better when keeping the webview rather simple, so I'd stick to that approach (e.g. debugging js code inside a webview can be quite the pain).

ManuelHentschel commented 3 years ago

In https://github.com/nx10/httpgd/pull/64 I made some provisional (!) changes that ease the styling of SVG plots in vscode. Feel free to commit directly to that branch, if you want to change things.

If you want to make changes to the vscode-R code at some point, feel free to just clone my PR, make the changes, and submit a new PR. If you leave the corresponding box checked, that would give us both (and all other vscode-R collaborators) write access to the branch.

It might also make sense to draft a new PR and leave the current one up as proof of concept for anyone interested.

ManuelHentschel commented 3 years ago

Regarding implementation details, I have a couple of questions on my mind. Any clarification/opinion would be helpful, but don't worry if you have other things to do at the moment.

With respect to the different "abstraction levels" of plotting with R and httpgd in particular, where would the vscode-R-viewer fit in best? E.g. would it make sense to have one viewer per R session? Or one per "device"? Can there be multiple httpgd-websockets for one R session, or is there at most one at any time? To e.g. compare plots side-by-side it probably make sense to decouple the webview from the "viewer" itself, so that the same plots can be opened in different panels.

Also, how much "two way communication" is there between the plot-view and R itself? E.g. can a plot be changed after another plot has been drawn? Does closing a plot in the web view have any effect on the graphics device in R?

I noticed that in the current PR it seems to be impossible to get the plot viewer back after closing the corresponding editor tab (i.e. calling plot() again doesn't trigger a new webview to be created). That should definitely be implemented differently as well...

@renkun-ken Any thoughts on this? You've implemented most of the communication between R and vscode-R so far, so you're probably most qualified in this area!

nx10 commented 3 years ago

With respect to the different "abstraction levels" of plotting with R and httpgd in particular, where would the vscode-R-viewer fit in best? ...

The http/websocket API is intended to be a public API to which requests can be made from anywhere. I would also consider it stable in the sense that its endpoints and returned data structures will only be added to and not changed or removed for a long time. I added a short but complete API documentation vignette in the last release. It can also be accessed here

The JS/HTML client is basically a reference implementation that shows how you can consume the API. To make it easier for developers (and myself) I tried to separate most the connection and client state logic in the typescript file (httpgd.ts), and the interface logic in the html/css files. I am experimenting a bit with the client and httpgd.ts and would not consider their API stable at the moment. (I am currently adding an image export dialog for example which will need changes in both files)

Personally I think there are two routes we can take:

Either way shipping the client with vscode-R and only calling the httpgd API is probably the best thing to do. That way we can easily add vscode specific code and optimizations (without having to also ensure cross browser compatibility and bloat the reference implementation).

... E.g. would it make sense to have one viewer per R session? Or one per "device"? Can there be multiple httpgd-websockets for one R session, or is there at most one at any time? To e.g. compare plots side-by-side it probably make sense to decouple the webview from the "viewer" itself, so that the same plots can be opened in different panels. Also, how much "two way communication" is there between the plot-view and R itself? E.g. can a plot be changed after another plot has been drawn? Does closing a plot in the web view have any effect on the graphics device in R?

Some technical clarification to graphics devices upfront: The device level is defined by R. A "GraphicsDevice" is basically a library that consumes a number of basic C-level drawing functions that are called by the R "GraphicsEngine". There can be multiple devices attached in a R session, but only a single "active" device will receive the graphics/draw calls. There can be multiple instances/devices of httpgd running, but only one will be active. That also means that the inactive devices will not be able to receive new plots / change plots or resize plots. I can not think of any advantage that starting multiple instances/devices of httpgd has (unless having multiple plot histories between which you have to manually switch, but that seems a bit complicated).

httpgd is designed to be fully asynchronous and allow multiple connections (http and ws) to a single graphics device / instance. It will also ensure that you can add to the newest plot, even if you viewed or resized a plot from the history. (However it will only cache each plot in the last resolution is was drawn, so if you have two viewers with different sizes, it will have to call into R to redraw it which is always a bit slower.) You can test this by opening multiple browser windows or vscode panels with Kun Ren's method.

I noticed that in the current PR it seems to be impossible to get the plot viewer back after closing the corresponding editor tab (i.e. calling plot() again doesn't trigger a new webview to be created). That should definitely be implemented differently as well...

For that we can maybe have a look at how Rstudio handles this. I think their GraphicsDevice automatically opens when none other exists and a plotting function is called. If I remember right the current plotting implementation with the "session watcher" in vscode also intercepts draw calls already to open the window etc. (A hook to plot.new() may suffice)

Edit: Just a small addition: Complete bi-directional communication between http/ws and R is possible/used. The API is intended to be easy to understand and use and to figure out the optimal/fast way in C/C++ without having to expose all the complexity (and peculiarities) of the R graphics API.

ManuelHentschel commented 3 years ago

Some technical clarification to graphics devices upfront: ...

Thanks for the explanation! If I understood everything correctly, I'd stick to a similar scope as the included viewer and use one internal "viewer" instance per httpgd-device. Each internal viewer would then have (at most) one webview associated with it. If the user closes the webview, drawing a new plot (or some vscode command) should trigger the viewer-instance to create a new webpanel and restore the plots.

If we later add the option to open the same device in multiple viewers, the overhad caused by this internal viewer class should be neglibible, compared to the redrawing/rendering of the images which happens anyways when they are resized.

Copy parts of httpgd.ts and build an UI with the vscode APIs around it Downsides:

* Probably a bit more work

I'd also prefer this option. I'm somewhat familiar with the vscode api, so for me personally this is probably no more work than properly understanding the included httpgd-viewer.

The JS/HTML client is basically a reference implementation that shows how you can consume the API. To make it easier for developers (and myself) I tried to separate most the connection and client state logic in the typescript file (httpgd.ts), and the interface logic in the html/css files. I am experimenting a bit with the client and httpgd.ts and would not consider their API stable at the moment.

Those files are structured very well, putting together the vscode proof of concept was way easier than expected, using the included typescript code! I'm still a bit unsure about the technical details, though, so I tried to put together a somewhat simplified declaration file of the api and viewer in vscode-R@httpgdWIP. The HttpgdApiAndOrConnection class roughly corresponds to the R-facing part of the viewer and HttpgdViewer to the vscode-facing part.

Do these classes correctly capture the basics of the viewer api and leave room for sensible extension?

If yes, I could start working on an implementation of the HttpgdViewer class. Implementing the HttpgdApiAndOrConnection part would be a bit more difficult for me, if you're interested in taking this on would be great!

nx10 commented 3 years ago

I just had a look at your implementation and using something like this:

#svgDiv > svg {
    width: 100%;
    height: auto;
}

#plotDiv {
    width: 20%;
    height: 20%;
    float: left;
}

#plotDiv > svg {
    width: 100%;
    height: auto;
}

will scale the SVG without any modifications to httpgd.

Also I saw that, you currently load the SVG directly inside the HTML, in the viewer I wrote I rust use an <img src="..."> tag. With that the implementation might be a bit easier as the HTML engine will handle loading by itself, but the ability to style the plot with CSS directly is lost. (Which as we discussed is probably not ideal anyway and should be done with the R APIs) But I do not have a strong preference either way.

The declarations in httpgdTypes.d.ts look very good so far, I will do some modifications and try to write the implementation shortly. To which fork/branch should I ideally send PRs?

ManuelHentschel commented 3 years ago

Also I saw that, you currently load the SVG directly inside the HTML, in the viewer I wrote I rust use an <img src="..."> tag. With that the implementation might be a bit easier as the HTML engine will handle loading by itself, [...]

If possible, I'd try to avoid relying on the HTML engine to load other resources, since the webview is quite limited compared to a proper browser. E.g. in order to access local files, each accessed path needs to be explicitly converted to a WebviewUri inside the vscode-R extension. Accessing local ports is possible in principal, but can cause problems e.g. when using a remote session. And relative links usually don't work properly, since they are resolved relative to some dummy URL like vscode-webview://65c538bb-6fe3-47ed-a71e-3b1cfdb569b6.... (I'm sure these issues can all be solved somehow, but directly supplying the full HTML feels a lot easier).

[...] but the ability to style the plot with CSS directly is lost. (Which as we discussed is probably not ideal anyway and should be done with the R APIs) But I do not have a strong preference either way.

I'd definitely try to keep this feature. For simple plots, I really like the seamless integration into the editor's color theme. Though I agree that for more complicated plots, it will usually be better to use R side styling. This should be easy to implement (e.g. by adding an optional "no-styling" argument to hhpgd::hgd() and falling back to a simpler CSS file in vscode-R, if styling on the plot is detected).

To which fork/branch should I ideally send PRs?

I'd suggest sending the PRs directly to vscode-R, that way it's easier for other maintainers to weigh in.

Edit: The latest commits include some rather unnecessary hovers/animations, these are definitely not meant to be stay, I was just playing around a bit to see what's possible with CSS.

nx10 commented 3 years ago

If possible, I'd try to avoid relying on the HTML engine to load other resources, since the webview is quite limited compared to a proper browser. [...]

That makes a lot of sense.

I'd definitely try to keep this feature. For simple plots, I really like the seamless integration into the editor's color theme. Though I agree that for more complicated plots, it will usually be better to use R side styling. This should be easy to implement (e.g. by adding an optional "no-styling" argument to hhpgd::hgd() and falling back to a simpler CSS file in vscode-R, if styling on the plot is detected).

I understand. Still, we could keep this by functionality without having to change the renderer with pure CSS:

.httpgd {
    width: 100%;
    height: auto;
}

.httpgd > rect:first-of-type {
    fill: none !important;
}

.httpgd text {
    font-family: var(--vscode-editor-font-family) !important;
    fill: var(--vscode-foreground);
}

.httpgd line, .httpgd polyline, .httpgd polygon, .httpgd path, .httpgd rect, .httpgd circle {
    stroke: var(--vscode-foreground) !important;
}

I am a bit reluctant to add special cases to the renderer, as I want each SVG to be as close to the representation dictated by the R graphics engine as possible, and feel like theming should be done on top of that.

The latest commits include some rather unnecessary hovers/animations, these are definitely not meant to be stay, I was just playing around a bit to see what's possible with CSS.

I had the same idea multiple times while writing the renderer, even though this should not be the default, I think animated plots could be quite fun.

ManuelHentschel commented 3 years ago

I am a bit reluctant to add special cases to the renderer, as I want each SVG to be as close to the representation dictated by the R graphics engine as possible, and feel like theming should be done on top of that.

Definitely makes sense. I'm just trying to keep the viewer code as clean/structured as possible, and would prefer avoiding !important as it can make the CSS rather hard to maintain (see e.g. https://www.w3schools.com/css//css_important.asp). But I can work around that, either by using !important or by simply removing the general styling from the svg before passing it to the webview.

(Athough, to me, having an option in httpgd to produce minimal styling sounds useful in other scenarios, as well, e.g. to include the produced plots in a website, or match them to some corporate theme. So let me know in case you do change your mind!)

nx10 commented 3 years ago

Definitely makes sense. I'm just trying to keep the viewer code as clean/structured as possible, and would prefer avoiding !important as it can make the CSS rather hard to maintain (see e.g. https://www.w3schools.com/css//css_important.asp). But I can work around that, either by using !important or by simply removing the general styling from the svg before passing it to the webview.

Yes, I think !important could be acceptable in that case, as there will probably only ever be one style sheet overriding the plot theme, but I absolutely understand your reservations.

(Athough, to me, having an option in httpgd to produce minimal styling sounds useful in other scenarios, as well, e.g. to include the produced plots in a website, or match them to some corporate theme. So let me know in case you do change your mind!)

I agree, but when I implement this in httpgd, I would want to solve this generally to allow theming of plots regardless of complexity. I had a quick look at how precedence is handled in CSS/SVGs and in the future I will investigate a rendering mode that properly splits the SVG and all CSS styles (including style attributes) in two separate files. Until then I believe procedurally removing the styling information with JS or overriding it with !important should suffice.

Edit: I have opened an issue (https://github.com/nx10/httpgd/issues/65) for this.

nx10 commented 3 years ago

I just finished my draft for the public client API, but I have trouble adding to your PR. I think I either do not understand how to do this in git or I need to be a maintainer to be allowed to do so.

ManuelHentschel commented 3 years ago

You could simply open a new PR at vscode-R, I'll be able to add to that. I can close the other PR or we just leave it up as Proof of concept.

nx10 commented 3 years ago

I just opened the PR (https://github.com/Ikuyadeu/vscode-R/pull/620). Let me know if you need some additional functionality of the interface or have other questions.

Edit: Exporting plots is currently client side only, there are functions for drawing the SVG to a canvas and downloading/copying it to clipboard here https://github.com/nx10/httpgd/blob/8ea7a0d749bbcc0bceed1647229ecccf57122ae7/inst/www/httpgd.ts#L532-L566

nx10 commented 3 years ago

I noticed the field changedPlots?: PlotId[]; in your last commit. Some notes regarding changes in plots:

Edit: In the original viewer I implemented this by removing (and adding) plots from the history if they are not in the current plot list and re-requesting the newest plot whenever the server notifies changes.

I hope this helps.

ManuelHentschel commented 3 years ago

Thanks for the clarification, that was something I wanted to ask about as well. In the latest commit, I tried to adjust the refreshPlots and checkState methods to only request the plots that might have changed.

Regarding the removing of plots, how can this be triggered? Only through the viewer or through R as well? Would it make sense to handle this in the viewer? We would not re-request old plots anyways, and simply marking a plot as "closed" would give us the possibility to undo closing a plot or "resetting" the view.

nx10 commented 3 years ago

Plots can be removed from both R and HTTP see. The viewer could implement "hiding" plots, but this will keep the plot in memory, it also would need to keep track of which plots are hidden by itself. Maybe "hiding" could be implemented at a later date so that the last deletion can be reverted, but as plots can usually be re-generated very quickly, I am not sure how important it is.

ManuelHentschel commented 3 years ago

Distinguishing between "closing" and "hiding" feels like a sensible approach. In the last commits, I added commands for both. The "x" Button in the title bar hides a plot, holding ALT closes the plot. Hiding is implemented by keeping a list of hidden plotIds which each plot.id is checked again before fetching its svg.

ManuelHentschel commented 3 years ago

Regarding the dynamic resizing of plots: Do plots have a natural/preferred size or aspect ratio, independent of the viewer? If yes, is this individual to the plot or the same for the entire device?

Inside vscode, it probably makes sense to e.g. keep the aspect ratio fix, since the size of the webview panel is usually not ideal for plotting

nx10 commented 3 years ago

There is no preferred size or ratio. They can be freely resized.

Maybe it would be possible to allow resizing the panel in both directions or add a movable divider between the history and viewed plot?

ManuelHentschel commented 3 years ago

I added a movable divider below the plot, as you suggested. Getting this to work smoothly was a bit of a pain, but for now I think the resizing feels more or less natural. I used preserveAspectRatio="none" and width: 100%; height: 100%; on the main svg element to enable smooth resizing without having to call the httpgd-api all the time.

Some (re)sizing related issues that still need to be implemented/discussed:

Also, the design of the entire viewer is still a bit provisional since I've never done web development/design before. If you have any experience from the integrated httpgd-viewer, feel free to modify the webview to your liking.

Any feedback on the currently implemented features is of course welcome as well :)

nx10 commented 3 years ago

The viewer seems to be coming along quite nicely, well done! I tried the last couple iterations of your dynamic resizing and agree, now it "feels" quite good.

Here are a couple of notes:

Great work so far!

Edit: I am not very much of a front end developer myself, so don't worry. I think if we create a performant viewer that integrates and works well, design can be iterated on later (possibly with the help and feedback of others).

ManuelHentschel commented 3 years ago

Thanks for the feedback!

  • In my opinion the horizontal padding of the iframe is a bit to much. setting body { padding: 0; } looks better in my opinion. what do you think?

Agree. I set it to 1px to avoid hiding the border of the svg when there is one.

  • The UI buttons seem to be broken for me in the last couple commits, I am getting an error, e.g.: "Running the contributed command: 'r.httpgd.toggleStyle' failed." when I click them.

No idea what's causing this. You could try setting a breakpoint here to see what's going on...

  • I currently do zoom in the httpgd-viewer by just scaling a smaller plot with the SVG width and height attributes, with the idea that zoom = 1/scale.

Thanks for the example, I roughly implemented the mechanism in the latest commit and it seems to work as expected.

  • I currently have the Plot history items centered and fit to a fixed aspect ratio with css in the httpgd-viewer. That way they sort and display nicely regardles of changes in the main plot size. We could also consider rendering these in a fixed resolution at the expenso of an additional R prerender step.

Sounds good. We could also use preserveAspectRatio="none" to just stretch them to a fixed aspect ratio without needing to rerender them (imo, the stretching looks ok when resizing the plot and probably isn't a big deal in the small preview).

  • I have begun work on an export dialog in the httpgd-viewer that lets you enter a specific resolution and scale to export plots. I have paused development on that because I am working on modular rendering + server side renderers for PNG and other formats now.

Apart from just dumping the current SVG to a file I haven't worked on this yet. The export dialog in vscode should probably be done using the input methods provided by the extension api. A server side export mechanism would be nice to have, since it means less work to be done in the extension, but I'd assume this is possible inside the extension as well. Are you also planning to implement R functions to export plots? If so, it might make sense to allow handling some user interaction by the client (either the integrated on or vscode), e.g. by calling hgd_export() and having the UI ask for format, size etc.

  • I think the plot history should be a scrollable div, so you can traverse it without having to scroll away from the main plot.

Makes sense, I'll have to look into how to implement that...

nx10 commented 3 years ago

No idea what's causing this. You could try setting a breakpoint here to see what's going on...

The regex in getPanelPath() returns undefined, because dummyUri.toString() looks like this 'vscode-webview-resource://589df09e-e701-49a4-9aa9-5afeb09a321fc/file///'.

Sounds good. We could also use preserveAspectRatio="none" to just stretch them to a fixed aspect ratio without needing to rerender them (imo, the stretching looks ok when resizing the plot and probably isn't a big deal in the small preview).

Agreed.

Are you also planning to implement R functions to export plots? If so, it might make sense to allow handling some user interaction by the client (either the integrated on or vscode), e.g. by calling hgd_export() and having the UI ask for format, size etc.

All http API functions have equivalent bindings in R. There is already hgd_svg(file="plot1.svg"). There will be an API endpoint that gives you a list of renderers that looks something like this:

{  "renderers": [ 
  { "id": "svg", "extension": ".svg", "mime": "image/svg+xml", "name": "SVG"},  
  { "id": "png", "extension": ".png", "mime": "image/png", "name": "PNG"},  
  ...
]

Keeping this dynamic will allow adding renderers in the backend without having to update the client code and some renderers will only be conditionally unavailable depending on the host system (e.g.: on linux cairo will need to be installed to use PNG).

Edit: https://github.com/Ikuyadeu/vscode-R/pull/620/commits/64197521b9195f0a0aaaad250722dc71c98eb736 fixed the problem thanks!

ManuelHentschel commented 3 years ago

The regex in getPanelPath() returns undefined, because dummyUri.toString() looks like this 'vscode-webview-resource://589df09e-e701-49a4-9aa9-5afeb09a321fc/file///'. [...] Edit: Ikuyadeu/vscode-R@6419752 fixed the problem thanks!

Glad the fix works, though I'm a bit surprised that these Uris look different on different machines/OSs.

Sounds good. We could also use preserveAspectRatio="none" to just stretch them to a fixed aspect ratio without needing to rerender them (imo, the stretching looks ok when resizing the plot and probably isn't a big deal in the small preview).

Agreed.

I implemented this now and it looks ok imo, but there is a weird bug when making the main plot smaller: image Note that there isn't a single cutoff height, but each svg element seems to be cut off relative to its total height. I have no idea what's causing this (and the problem exists with different sizing of the small plots as well), so I'll just leave it for now...

There will be an API endpoint that gives you a list of renderers that looks something like this:

Looks good! Regardless of the front-end it might make sense to include the unavailable renderers as well, with a note explaining how to enable them/why they are not available.

ManuelHentschel commented 3 years ago

Note that there isn't a single cutoff height, but each svg element seems to be cut off relative to its total height. I have no idea what's causing this (and the problem exists with different sizing of the small plots as well), so I'll just leave it for now...

I think I figured it out: The SVGs use the same ids in the clipPath elements (e.g. <clipPath id="c0"> and later <g clip-path="url(#c0)">), since they are not originally intended to be part of the same HTML document. When rendering the page, the engine probably just uses the first matching id it can find in the HTML, which is not correct if the main plot and the small plots have different sizes.

In general, ids are supposed to be used only once per document anyways, so I guess the best solution for this would be to use unique ids for the clipPaths. This could probably be done serverside, e.g. by prepending the plotId, but that decision is up to you @nx10. If you prefer not to change the Ids, i can probably just search/replace them before passing the svg to the plotviewer.

nx10 commented 3 years ago

You are right, I will think of something that solves this server side. Maybe https://github.com/nx10/httpgd/issues/65 should be a "portable SVG" renderer that avoids any page wide/global settings (at the expense of slightly increased file sizes).

Edit: If it's not too much work I would suggest replacing them for now as you said. Maybe with a regex right after fetching the svg.

ManuelHentschel commented 3 years ago

I added a function to replace the ids using regexes for now, which seems to be working fine. But since this mechanism is quite error-prone, this should at some point be replaced by a server-side solution or a proper implementation using an HTML parser/editor (e.g. cheerio).

ManuelHentschel commented 3 years ago

I've made a number of changes recently, let me know if you have any feedback, questions, or find some bugs :)

nx10 commented 3 years ago

Looks very good so far. It seems like it's nearing a releasable state. Have you had a look at how setting the startup options could be done via the extension settings? I should have a prototype of the modular server side rendering API ready very soon.

ManuelHentschel commented 3 years ago

Looks very good so far. It seems like it's nearing a releasable state.

Yeah, I guess there's no harm in merging it in an opt-in fashion, that way we'd hopefully receive some more feedback, bug reports etc.

Have you had a look at how setting the startup options could be done via the extension settings?

With most other features, configuration like this is handled using option(...) in R itself (requiring the user to set these options in their .Rprofile). So I'd go about this by using something like getOption("vsc.httpgd.initialWidth", 720). Though it might make sense to include these options directly in httpgd, that way users can specify their preferred startup options independently from vscode.

nx10 commented 3 years ago

I agree that default options for httpgd should exist. There is already https://github.com/nx10/httpgd/issues/12, which was opened when the API was changing a lot, but should be straight forward to implement now.

I am specifically talking about having to set

options(vsc.plot = FALSE)
options(device = function(...) {
  httpgd::hgd(
    silent = TRUE
  )
  .vsc$request('httpgd', url=httpgd::hgd_url())
})

As far as I understand, the users currently do not have to edit their .Rprofile anymore to get vscode-R running. I think having users edit a file should be avoided, but I know too little about the current implementation to know if this is feasible.

ManuelHentschel commented 3 years ago

Ah, that step is done here for the current plot viewer anyways, so this should be quick to modify to optionally use httpgd.

Edit: With the latest commit it's only required to set options(vsc.useHttpgd = TRUE) in .Rprofile to use the httpgd-based plot viewer.

ManuelHentschel commented 3 years ago

I'm not aware of any "breaking" bugs at the moment, so I'd propose merging the viewer as opt-in feature to get some feedback. Ok?

renkun-ken commented 3 years ago

Thanks for the amazing work on this. I suggest that we keep the naming convention consistent with the existing options such as vsc.use_rstudioapi, i.e. options(vsc.use_httpgd = TRUE).

nx10 commented 3 years ago

What is your opinion on the default selection of the dynamic vscode theme? While I see its benefits (and personally prefer dark themes), I feel like the plots should have the expected style dictated by the R graphics engine per default and dynamic styling should be opt-in. I think it might confuse new users why their graphics settings get overriden.

ManuelHentschel commented 3 years ago

Definitely makes sense. I set it to the modified style, mostly to make sure that users notice this feature when trying out the PR. But I don't mind changing it to the unmodified style before merging

Edit: The default setting should be original styling now

renkun-ken commented 3 years ago

and dynamic styling should be opt-in

I agree. In my use cases, I'll mostly keep the styling by the R graphics engine so that it is more straightforward to share the plots with others with expected coloring.

nx10 commented 3 years ago

@ManuelHentschel The modular rendering branch is ready for testing. There is still quite a lot of busy work left, but all the "hard problems" are solved.

I am sorry that the development is a bit slower from my side, but I am quite busy with my thesis paper at the moment.

ManuelHentschel commented 3 years ago

I played around a bit with the branch already and the renderers work quite nicely! Especially looking forward to the Tikz renderer.

For now I'm just planning to get a basic viewer merged, we can then add renderers etc. iteratively, so don't worry the development speed here.