Default plots to PNG output, and support showing PNGs or SVGs in the existing PlotViewer control #7140
Conversation
| @@ -1809,8 +1809,8 @@ | |||
| }, | |||
| "jupyter.enablePlotViewer": { | |||
| "type": "boolean", | |||
| "default": true, | |||
| "description": "Modify plot output so that it can be expanded into a plot viewer window.", | |||
| "default": false, | |||
There was a problem hiding this comment.
We might need to discuss this. I feel that customers might still want the old behavior, so I left the setting in, but defaulted it to false instead of true, so they can opt into getting the extra SVGs if they want. My code will prefer SVG output to any PNG output if present.
| export const MatplotLibInitSvg = `import matplotlib\n%matplotlib inline\n${Identifiers.MatplotLibDefaultParams} = dict(matplotlib.rcParams)\n%config InlineBackend.figure_formats = ['svg', 'png']`; | ||
| export const MatplotLibInitPng = `import matplotlib\n%matplotlib inline\n${Identifiers.MatplotLibDefaultParams} = dict(matplotlib.rcParams)\n%config InlineBackend.figure_formats = ['png']`; | ||
| export const ConfigSvg = `%config InlineBackend.figure_formats = ['svg', 'png']`; | ||
| export const ConfigPng = `%config InlineBackend.figure_formats = ['png']`; |
There was a problem hiding this comment.
Looks like this has been broken for a long time / forever? For multiples it seems to take {} or [], but for the singular it can't:
Docs are a little funny on this, they say set, but the example uses a list?
Guessing we didn't notice this before as png is Jupyter default I'm pretty sure. So it would fail to run, but then just get PNG anyways from the Jupyter default.
| @@ -821,6 +823,16 @@ export class JupyterNotebookBase implements INotebook { | |||
| // get a request to update style | |||
| await this.executeSilently(matplobInit, cancelToken); | |||
|
|
|||
| const useDark = this.applicationService.activeColorTheme.kind === ColorThemeKind.Dark; | |||
There was a problem hiding this comment.
This code was in the kernel.ts version, but not here so I added it as it looks correct for native theming.
| !isResourceNativeNotebook(this._resource, this.vscNotebook, this.fs) | ||
| ? CodeSnippets.ConfigSvg | ||
| : CodeSnippets.ConfigPng; | ||
| settings && settings.enablePlotViewer ? CodeSnippets.ConfigSvg : CodeSnippets.ConfigPng; |
There was a problem hiding this comment.
Removed the !isResourceNativeNotebook as Native notebooks can now use the setting to opt into getting the SVGs if they want.
This code also doesn't look to have worked before. The check for isNativeNotebook was here, but not in the kernel.ts version. So this init would run first and set to [png] then the init in the native kernel.ts code would run and set to [png, svg] so native notebooks were always getting the SVGs unless the setting was off.
Codecov Report
@@ Coverage Diff @@
## main #7140 +/- ##
=====================================
- Coverage 64% 64% -1%
=====================================
Files 362 362
Lines 22887 22879 -8
Branches 3430 3431 +1
=====================================
- Hits 14825 14777 -48
- Misses 6751 6799 +48
+ Partials 1311 1303 -8
|
| @@ -2095,6 +2095,7 @@ | |||
| "glob": "^7.1.2", | |||
| "hash.js": "^1.1.7", | |||
| "iconv-lite": "^0.4.21", | |||
| "image-size": "^1.0.0", | |||
There was a problem hiding this comment.
@DonJayamanne image-size was added here.
| @@ -287,93 +283,6 @@ export class JupyterNotebookBase implements INotebook { | |||
| return this.session ? this.session.waitForIdle(timeoutMs) : Promise.resolve(); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Intialize code (working dir / startup commands / plot init) now all moved from Notebook into Kernel.
There was a problem hiding this comment.
Gave a quick peek with notebooks and IW. Startup command, new working directory, and plots all looked good after a notebook start as well as after a kernel restart.
There was a problem hiding this comment.
@DonJayamanne could you take a re-look? It's still blocked on the change requested.
|
@DavidKutu @rchiodo Tagged for a re-review since I made a big bigger change to move init code only into Kernel versus Notebook. |
For #6913
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).