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

Gets rid of registerDecorationType in inlay hints controller #136517

Merged
merged 2 commits into from
Nov 8, 2021

Conversation

hediet
Copy link
Member

@hediet hediet commented Nov 5, 2021

See #132537.

The utility introduced by this PR can be made a general utility later.

@hediet hediet requested a review from jrieken November 5, 2021 12:59
Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

Left some, for future please don't do "unrelated" change renaming variables because that makes review unnecessary complex.

Tho, not sure what the overall motivation is? Is before/afterInjectedText going aways?

@@ -209,13 +208,14 @@ export class InlayHintsController implements IEditorContribution {
return result;
}

private readonly ruleFactory = new DynamicCssRules(this._editor);
Copy link
Member

Choose a reason for hiding this comment

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

nit - _ prefix for privates, also move atop ctor

.replace(/([A-Z])/g, ([letter]) => `-${letter.toLowerCase()}`);
}

function themeColorToCssVar(themeColor: ThemeColor): string {
Copy link
Member

Choose a reason for hiding this comment

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

* Reference counting and delayed garbage collection ensure that no rules leak.
*/
export class DynamicCssRules {
private counter = 0;
Copy link
Member

Choose a reason for hiding this comment

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

nit - more _ for privates

@hediet
Copy link
Member Author

hediet commented Nov 5, 2021

Tho, not sure what the overall motivation is? Is before/afterInjectedText going aways?

Yes. Ideally, everything related to "decoration types" is going away, as its implementation is very complicated.

@hediet hediet force-pushed the hediet/inlay-hints-controller-no-decoration-type branch from 051dbe7 to 39c7f7b Compare November 8, 2021 08:36
@hediet hediet merged commit fbb774e into main Nov 8, 2021
@hediet hediet deleted the hediet/inlay-hints-controller-no-decoration-type branch November 8, 2021 14:13
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2021
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.

2 participants