Fix problems that can occur when we can't find the theme json by rchiodo · Pull Request #5177 · microsoft/vscode-python

rchiodo · 2019-04-09T18:04:59Z

For #5136

Modify our react code to have a 'StyleInjector' that pulls our css values from the extension instead of having the extension generate them on startup. This is necessary because in the non theme case, we can't tell if we're dark or light. However VS Code knows and will tell us.

When no json is found we use the 'vscode-light' or 'vscode-dark' to decide on a base theme.

This has the side effect of also making our window update when the user updates the theme. No more reopening the window.

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
Title summarizes what is changing
Has a news entry file (remember to thank yourself!)
Has sufficient logging.
Has telemetry for enhancements.
Unit tests & system/integration tests are added/updated
Test plan is updated as appropriate
package-lock.json has been regenerated by running npm install (if dependencies have changed)
The wiki is updated with any design decisions/details.

Add support for when theme files aren't found.

rchiodo · 2019-04-09T18:12:34Z


 @injectable()
-export class DataViewer implements IDataViewer, IAsyncDisposable {
+export class DataViewer extends WebViewHost<IDataViewerMapping> implements IDataViewer, IDisposable {


WebViewHost [](start = 32, length = 11)

History and DataViewer now derive from the WebViewHost. This simplifies the css handling and other common stuff (like putting up the webview) #Resolved

rchiodo · 2019-04-09T20:11:15Z

+export declare function acquireVsCodeApi(): IVsCodeApi;
+
+// tslint:disable-next-line: no-unnecessary-class
+export class PostOffice {


PostOffice [](start = 13, length = 10)

I changed this class to be static. Otherwise I was passing refs to components around (the styleInjector needs a post office too). Seems like it didn't need to be a component at all as it doesn't render anything. #Resolved

rchiodo · 2019-04-09T20:13:05Z

            this.wrapper = undefined;
        }
+
+        PostOffice.resetApi();


PostOffice [](start = 8, length = 10)

Because the postoffice isn't a component anymore, we need to cleanup after every test. #Resolved

IanMatthewHuff · 2019-04-10T16:43:03Z

@@ -0,0 +1,2 @@
+Default colors when theme.json cannot be found.
+Fix python interactive window to update when theme changes.


This seems like a good candidate for a manual test case in our CTI test plan file. #Resolved

I agree. Is that in our source somewhere? I can update it here.

In reply to: 274056601 [](ancestors = 274056601)

Yup. .github/test_plan.md

In reply to: 274056754 [](ancestors = 274056754,274056601)

IanMatthewHuff · 2019-04-10T16:50:14Z

-            const theme = ignoreTheme ? DefaultTheme : workbench.get<string>('colorTheme');
-            const terminalCursor = workbench.get<string>('terminal.integrated.cursorStyle', 'block');
+            theme = ignoreTheme ? DefaultTheme : theme;
+            const terminalCursor = workbench ? workbench.get<string>('terminal.integrated.cursorStyle', 'block') : 'block';


Not directly part of this checkin, but the default for cursorStyle for VSCode is line, not block. #ByDesign

That's not what my settings say.

In reply to: 274059636 [](ancestors = 274059636)

Maybe you're thinking of editor cursor? Not terminal cursor?

In reply to: 274060268 [](ancestors = 274060268,274059636)

IanMatthewHuff · 2019-04-10T16:52:30Z

+                    css = this.generateCss(theme, tokenColors, font, fontSize, terminalCursor, ignoreTheme ? LightTheme : undefined);
+                } else if (tokenColors === null && font && fontSize) {
+                    // No colors found. See if we can figure out what type of theme we have
+                    const style = (theme && isDark) ? DarkTheme : LightTheme ;


we are already inside a if(theme) block here #Resolved

Oh yeah. True.

In reply to: 274060589 [](ancestors = 274060589)

IanMatthewHuff · 2019-04-10T16:58:22Z

    }

-    private getScopeStyle = (tokenColors: JSONArray, scope: string, secondary?: string): { color: string; fontStyle: string } => {
+    private getScopeStyle = (tokenColors: JSONArray | null, scope: string, secondary: string, defaultStyle: string | undefined): { color: string; fontStyle: string } => {


More of a style issue, but for X | undefined I'd usually just use ? notation, especially as a parameter. #ByDesign

There's a difference though. x? notation means the caller doesn't have to send that parameter. X | undefined means they have to be explicit. This change was to force it to be explicit.

In reply to: 274062994 [](ancestors = 274062994)

IanMatthewHuff · 2019-04-10T17:19:08Z

+
+    // Post a message to our webpanel and update our new datascience settings
+    private onPossibleThemeChange = (event: ConfigurationChangeEvent) => {
+        if (event.affectsConfiguration('workbench')) {


Can we get more specific here? affectsConfiguration supports dotted names (I'm not totally sure what section the theme color is in). Could save us some unneeded checks. #Resolved

I think "workbench.colorTheme" should work. Not 100% sure.

In reply to: 274070721 [](ancestors = 274070721)

Sure I'll try that.

In reply to: 274071214 [](ancestors = 274071214,274070721)

Thanks. It worked.

In reply to: 274071761 [](ancestors = 274071761,274071214,274070721)

IanMatthewHuff · 2019-04-10T17:34:08Z

+        }
+    }
+
+    private computeKnownDark() : boolean {


[](start = 30, length = 1)

In jupyterExecution you are removing all these spaces before the return type. #WontFix

I probably did 'Format Document' in the other file. Tslint doesn't complain, so I don't think it really matters.

In reply to: 274076718 [](ancestors = 274076718)

IanMatthewHuff

…theme

rchiodo added 5 commits April 5, 2019 17:40

Fix linter messages

4663793

Add support for when theme files aren't found.

Refactor common parts out of history and dataviewer

7406259

Handle refresh of theme completely.

99b4f71

Fix tests to pass

d70098e

Fix dark/light detection to use body class

deb3594

rchiodo self-assigned this Apr 9, 2019

rchiodo requested a review from IanMatthewHuff April 9, 2019 18:05

Add news entry

90c4bbe

rchiodo commented Apr 9, 2019

View reviewed changes

rchiodo added 2 commits April 9, 2019 12:36

Fix unit tests and dataViewer

ad28754

Fix font size in variable explore

9e6009b

rchiodo commented Apr 9, 2019

View reviewed changes

IanMatthewHuff reviewed Apr 10, 2019

View reviewed changes

Add manual tests for themes

71b7592

IanMatthewHuff reviewed Apr 10, 2019

View reviewed changes

Review feedback

4c4ce55

IanMatthewHuff reviewed Apr 10, 2019

View reviewed changes

Be more specific on configuration changes

c1938c4

IanMatthewHuff reviewed Apr 10, 2019

View reviewed changes

IanMatthewHuff approved these changes Apr 10, 2019

View reviewed changes

rchiodo added 2 commits April 10, 2019 10:38

Merge remote-tracking branch 'origin/master' into rchiodo/fix_unknown…

f1d8382

…theme

Fix merge from master

88b8e49

rchiodo merged commit deb3fa7 into master Apr 10, 2019

rchiodo deleted the rchiodo/fix_unknowntheme branch April 10, 2019 19:30

lock Bot locked as resolved and limited conversation to collaborators Jul 30, 2019

		@@ -0,0 +1,2 @@
		Default colors when theme.json cannot be found.
		Fix python interactive window to update when theme changes. No newline at end of file

Conversation

rchiodo commented Apr 9, 2019

Uh oh!

rchiodo Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Apr 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Apr 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Apr 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Apr 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Apr 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Apr 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rchiodo Apr 9, 2019 •

edited

Loading

rchiodo Apr 9, 2019 •

edited

Loading

rchiodo Apr 9, 2019 •

edited

Loading

IanMatthewHuff Apr 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff Apr 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff Apr 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff Apr 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff Apr 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff Apr 10, 2019 •

edited by rchiodo

Loading