-
Notifications
You must be signed in to change notification settings - Fork 31.9k
Attempts to make described workaround from issue #45226 #48028
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
Conversation
These are the tests that fail:
|
Should I just amend the other tests so that they pass? It looks like the notification message just causes many off-by-one errors. Although I'm not sure why '\t' became '\t\t\t' in 12). |
Fixes #45226
Creates a notification for when the user sets editor.fontFamily or terminal.integrated.fontFamily to a non-monospace font like Go or Verdana.
Although the notification mostly works, there are a couple issues I definitely need help with.
First issue is making my tests run. I tried to add two small unit tests, but the second one fails, even though the tested behavior succeeds when I exercise it within a running instance of VSCode.
https://github.com/chenhowa/vscode/blob/c2d50f7ce2e4ef7e6ea1819e14d878ecc07d1993/src/vs/workbench/parts/terminal/test/electron-browser/terminalConfigHelper.test.ts#L144-L154
Second issue is that my changes cause around 11 other unit tests to fail. I believe I've localized the cause to the call to this._notificationService.notify() -- when I comment out the call, the tests pass.
https://github.com/chenhowa/vscode/blob/c2d50f7ce2e4ef7e6ea1819e14d878ecc07d1993/src/vs/workbench/parts/terminal/electron-browser/terminalPanel.ts#L83-L93