Skip to content

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

Merged
merged 12 commits into from
May 25, 2018

Conversation

chenhowa
Copy link

@chenhowa chenhowa commented Apr 17, 2018

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

@msftclas
Copy link

msftclas commented Apr 17, 2018

CLA assistant check
All CLA requirements met.

@chenhowa
Copy link
Author

These are the tests that fail:

  1. Common Editor Config wordWrap compat true:

    AssertionError [ERR_ASSERTION]: 82 == 81

    • expected - actual

    -82
    +81

    at assertWrapping (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/common/config/commonEditorConfig.test.js:67:20)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/common/config/commonEditorConfig.test.js:83:13)

  2. Common Editor Config wordWrap on:

    AssertionError [ERR_ASSERTION]: 82 == 81

    • expected - actual

    -82
    +81

    at assertWrapping (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/common/config/commonEditorConfig.test.js:67:20)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/common/config/commonEditorConfig.test.js:89:13)

  3. Common Editor Config wordWrap on without minimap:

    AssertionError [ERR_ASSERTION]: 91 == 89

    • expected - actual

    -91
    +89

    at assertWrapping (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/common/config/commonEditorConfig.test.js:67:20)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/common/config/commonEditorConfig.test.js:98:13)

  4. Common Editor Config wordWrap on does not use wordWrapColumn:

    AssertionError [ERR_ASSERTION]: 82 == 81

    • expected - actual

    -82
    +81

    at assertWrapping (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/common/config/commonEditorConfig.test.js:67:20)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/common/config/commonEditorConfig.test.js:105:13)

  5. Workbench - TerminalConfigHelper TerminalConfigHelper - isMonospace Go:

    AssertionError [ERR_ASSERTION]: Go is not mono-spaced

    • expected - actual

    -true
    +false

    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/workbench/parts/terminal/test/electron-browser/terminalConfigHelper.test.js:132:20)

  6. Editor Controller - Indentation Rules Enter supports selection 1:

    AssertionError [ERR_ASSERTION]: [ '[5,2 -> 5,2]' ] deepEqual [ '[5,1 -> 5,1]' ]

    • expected - actual

    [

    • "[5,2 -> 5,2]"
    • "[5,1 -> 5,1]"
      ]

    at assertCursor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:125:16)
    at file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2258:17
    at usingCursor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2932:9)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2244:13)

  7. Editor Controller - Indentation Rules Enter supports selection 2:

    AssertionError [ERR_ASSERTION]: [ '[3,2 -> 3,2]' ] deepEqual [ '[3,3 -> 3,3]' ]

    • expected - actual

    [

    • "[3,2 -> 3,2]"
    • "[3,3 -> 3,3]"
      ]

    at assertCursor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:125:16)
    at file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2275:17
    at usingCursor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2932:9)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2263:13)

  8. Editor Controller - Indentation Rules Enter should adjust cursor position when press enter in the middle of leading whitespaces 2:

    AssertionError [ERR_ASSERTION]: [ '[4,4 -> 4,4]' ] deepEqual [ '[4,3 -> 4,3]' ]

    • expected - actual

    [

    • "[4,4 -> 4,4]"
    • "[4,3 -> 4,3]"
      ]

    at assertCursor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:125:16)
    at file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2449:17
    at usingCursor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2932:9)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2436:13)

  9. Editor Controller - Indentation Rules Enter should adjust cursor position when press enter in the middle of leading whitespaces 5:

    AssertionError [ERR_ASSERTION]: [ '[4,5 -> 4,5]' ] deepEqual [ '[4,3 -> 4,3]' ]

    • expected - actual

    [

    • "[4,5 -> 4,5]"
    • "[4,3 -> 4,3]"
      ]

    at assertCursor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:125:16)
    at file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2522:17
    at usingCursor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2932:9)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2507:13)

  10. Editor Controller - Indentation Rules bug Test diff editor indicators #16543: Tab should indent to correct indentation spot immediately:

    AssertionError [ERR_ASSERTION]: '\t' == '\t\t'

    • expected - actual

    at file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2619:24
    at Object.withTestCodeEditor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/testCodeEditor.js:123:9)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2615:30)

  11. Editor Controller - Indentation Rules bug When pressing Tab on white-space only lines, indent straight to the right spot (similar to empty lines) #2938 (1): When pressing Tab on white-space only lines, indent straight to the right spot (similar to empty lines):

    AssertionError [ERR_ASSERTION]: '\t\t' == '\t\t\t'

    • expected - actual

    at file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2638:24
    at Object.withTestCodeEditor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/testCodeEditor.js:123:9)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2634:30)

  12. Editor Controller - Indentation Rules bug When pressing Tab on white-space only lines, indent straight to the right spot (similar to empty lines) #2938 (2): When pressing Tab on white-space only lines, indent straight to the right spot (similar to empty lines):

    AssertionError [ERR_ASSERTION]: '\t' == '\t\t\t'

    • expected - actual

    at file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2657:24
    at Object.withTestCodeEditor (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/testCodeEditor.js:123:9)
    at Context. (file:///home/howardchen/OpenSource/vscode/out/vs/editor/test/browser/controller/cursor.test.js:2653:30)

@chenhowa
Copy link
Author

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).

@Tyriar Tyriar added this to the May 2018 milestone May 15, 2018
@Tyriar
Copy link
Member

Tyriar commented May 25, 2018

Thanks for the PR and sorry about the delay. I made some tweaks and improved the notification to have a quick fix button:

image

@Tyriar Tyriar merged commit 4b4d6e6 into microsoft:master May 25, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn users if the terminal font is not monospace
4 participants