astoff / comint-mime

Display graphics and other MIME attachments in Emacs shells
67 stars 6 forks source link

Add support for SVG plots using IPython/Matplotlib #21

Closed jabirali closed 6 months ago

jabirali commented 6 months ago

This pull request resolves issue #20:

For comparison with #20, the same example looks like this after this PR:

Screenshot 2024-04-24 at 17 44 33
jabirali commented 6 months ago

Additional effect of using SVG for Matplotlib plots: I just discovered that the text color appears to follow the current Emacs theme. So if you use a Matplotlib style sheet with a transparent background (or set rcParams["figure.facecolor"] = (0, 0, 0, 0)), then the plot text automatically changes colors when Emacs does (e.g. useful with auto-dark).

Screenshot 2024-04-24 at 22 21 24
astoff commented 6 months ago

Thanks for the PR. The first change we should certainly merge. The second probably as well, but I would like to understand the consequences of enabling SVG output. Are there any downsides? Also: InlineBackend.figure_formats is a list, what happens when there's more than one element in it?

jabirali commented 6 months ago

Thanks for the quick response!

Thanks for the PR. The first change we should certainly merge.

Great! That one is the most important, since the second change doesn't necessarily have to be in comint-mime itself.

The second probably as well, but I would like to understand the consequences of enabling SVG output.

This is a valid concern, and I agree that it's good to think through the default carefully. I think we have three options:

After thinking a bit about it (see also the comments below and the revised PR), the last option would perhaps be ideal?

Are there any downsides?

I did a bit of research, and it seems that there are two downsides:

I now think that SVG might not be a great default, especially since it would certainly be surprising to a user if Emacs spent gigabytes of memory on showing a few plots. As a user, I would however still enjoy an option like e.g. comint-mime-image-scalable to easily toggle it on, as the SVG looks much significantly better here. I remember trying the SVG support in VSCode before and didn't have any issues, but it probably depends on e.g. the data density, plot type, and hardware.

Also: InlineBackend.figure_formats is a list, what happens when there's more than one element in it?

Based on this, it appears that it is a set and not a list, and that the allowed values in that set are png, retina, jpeg, svg, pdf.

I did a bit of testing on my machine as to what these do:

I think the conclusion is the only values that are useful in the context of comint-mime would be the default ['png'] for raster images or ['svg'] for scalable images. Instead of a general comint-mime-image-format type option to choose between these, it might then make sense to have a more specific comint-mime-image-scalable option that toggles whether we should prefer SVG output from REPLs when supported (with currently only Python implemented of course).

jabirali commented 6 months ago

Based on the considerations above, I have now updated the pull request as follows (see c7af585):

I think this might perhaps be a better way to implement it than my original PR :).

astoff commented 6 months ago

Thanks for the detailed information. I agree with your conclusion, so I will merge your suggestion. I just have two small requests.

  1. How about renaming the variable to comint-mime-prefer-svg?
  2. There are some tab characters, could you untabify (replace by spaces)?

I don't need any paperwork to merge this small change, but, because this package is on GNU ELPA, you would need to sign the FSF copyright assignment papers if you want to continue contributing patches.

Also so you know: I will add similar changes in drepl, a new package I'm developing.

jabirali commented 6 months ago

Thanks for the detailed information. I agree with your conclusion, so I will merge your suggestion.

Great, thanks!

I just have two small requests.

  1. How about renaming the variable to comint-mime-prefer-svg?

  2. There are some tab characters, could you untabify (replace by spaces)?

Done and done: 589ce84. For consistency, I also renamed the corresponding Python variable to prefer_svg.

I don't need any paperwork to merge this small change, but, because this package is on GNU ELPA, you would need to sign the FSF copyright assignment papers if you want to continue contributing patches.

Understood! I don't mind signing the FSF papers, but I haven't done it before so I would have to familiarize myself with the process in that case. I will keep it in mind if I have further suggestions for improvements.

Also so you know: I will add similar changes in drepl, a new package I'm developing.

Sounds good! I'll keep an eye on that project, that sounds interesting as well.

astoff commented 1 month ago

@jabirali FYI, for the reasons explained in #23, I removed the line of your MR that added a workaround for dark theme users; unfortunately it seems in this case the user has to fix the problem on their own.

Thankfully that's not too hard. I added the following explanation to comint-mime-prefer-svg, which you might want to apply:

Using a theme with dark background can render some SVGs unreadable. If you experience this, try setting `comint-mime-image-props' to (:foreground "black" :background "white"). Alternatively, configure the SVG creator to produce images that play well with your theme.

jabirali commented 1 month ago

That sound reasonable. Thanks for the heads up! :)