Skip to content
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

Shows a banner if a file has too many highlighted unicode characters. #137889

Merged
merged 4 commits into from
Nov 26, 2021

Conversation

hediet
Copy link
Member

@hediet hediet commented Nov 25, 2021

recording

@hediet hediet self-assigned this Nov 25, 2021
@hediet hediet force-pushed the hediet/unicode-highlighting-banner branch from 4df5d26 to 74bc6aa Compare November 25, 2021 15:54
@hediet hediet requested a review from alexdima November 25, 2021 16:13
@hediet hediet force-pushed the hediet/unicode-highlighting-banner branch from 74bc6aa to 433b97d Compare November 25, 2021 16:13
@hediet hediet requested review from alexdima and removed request for alexdima November 25, 2021 16:17
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! 👏

src/vs/editor/browser/widget/codeEditorWidget.ts Outdated Show resolved Hide resolved
@@ -458,6 +458,7 @@ export abstract class CommonEditorConfiguration extends Disposable implements IC

protected abstract readConfiguration(styling: BareFontInfo): FontInfo;

public abstract reserveHeight(height: number): void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I realize that this method could be implemented here because it doesn't need any dom stuff (it can live in /common/ here). _computeInternalOptions could then subtract the reserved height.

src/vs/editor/test/common/mocks/testConfiguration.ts Outdated Show resolved Hide resolved
@@ -424,6 +516,78 @@ const DECORATION = ModelDecorationOptions.register({
}
});

export class DisableHighlightingOfAmbiguousCharactersAction extends EditorAction {
public static ID = 'editor.action.unicodeHighlight.disableHighlightingOfAmbiguousCharacters';
public readonly shortLabel = nls.localize('unicodeHighlight.disableHighlightingOfAmbiguousCharacters.shortLabel', '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like shortLabel here is gettting removed by the tree shaking. The reason is that if you try to find all references or rename this property, you will notice that no references are found by TypeScript, or that the rename introduces a compilation error.

The solution is to be more explicit (instead of relying 100% on duck-typing) and create a new interface IDisableUnicodeHighlightAction with the shortLabel property and then explicitly implement it.

@hediet hediet force-pushed the hediet/unicode-highlighting-banner branch from 256eae8 to 8a305e1 Compare November 26, 2021 10:00
@hediet hediet merged commit bc75bda into main Nov 26, 2021
@hediet hediet deleted the hediet/unicode-highlighting-banner branch November 26, 2021 10:39
guibber pushed a commit to guibber/vscode that referenced this pull request Nov 30, 2021
…hlighting-banner

Shows a banner if a file has too many highlighted unicode characters.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2022
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.

None yet

2 participants