Conversation
There was a problem hiding this comment.
I dislike these Provider and ProviderManager classes.
I think the rewrite should be more functional. Any file is a module, and any module can be used like a class.
And yes... you repeated the Git bug issue; looks like the bike is still being invented. Except now, I really don't know how to fix it with this pattern. This pattern actively makes our world a worse place. / hj
| import { TextEditor, window } from "vscode"; | ||
| import { Provider } from "./provider"; | ||
|
|
||
| export class TextEditorBasedProvider extends Provider { | ||
| protected get textEditor() { | ||
| return window.activeTextEditor; | ||
| } |
There was a problem hiding this comment.
TextEditor is unused.
| import { TextEditor, window } from "vscode"; | |
| import { Provider } from "./provider"; | |
| export class TextEditorBasedProvider extends Provider { | |
| protected get textEditor() { | |
| return window.activeTextEditor; | |
| } | |
| import { type TextEditor, window } from "vscode"; | |
| import { Provider } from "./provider"; | |
| export class TextEditorBasedProvider extends Provider { | |
| protected get textEditor(): TextEditor | undefined { | |
| return window.activeTextEditor; | |
| } |
| } | ||
|
|
||
| public override shouldSkip(): boolean { | ||
| return !!!this.textEditor; |
There was a problem hiding this comment.
| return !!!this.textEditor; | |
| return !this.textEditor; |
| } | ||
|
|
||
| public override shouldSkip(): boolean { | ||
| return !!!this.notebookEditor; |
There was a problem hiding this comment.
| return !!!this.notebookEditor; | |
| return !this.notebookEditor; |
| export class Provider extends Base { | ||
| public activated = false; | ||
| private variables = new Map<string, () => Promise<string | undefined>>(); | ||
|
|
||
| constructor( | ||
| extension: Extension, | ||
| public id = "base", | ||
| public priority = 0 | ||
| ) { | ||
| super(extension); | ||
| this.registerVariables(); | ||
| } | ||
|
|
||
| public subscribe() { | ||
| return; | ||
| } | ||
|
|
||
| public shouldSkip(): boolean { | ||
| return true; | ||
| } | ||
|
|
||
| public hasVariable(name: string): boolean { | ||
| return this.variables.has(name); | ||
| } | ||
|
|
||
| public async resolveVariable(name: string): Promise<string | undefined> { | ||
| return this.variables.has(name) ? await this.variables.get(name)!() : undefined; | ||
| } | ||
|
|
||
| protected registerVariables() {} | ||
|
|
||
| protected async provide(name: string, value: () => Promise<string | undefined>) { | ||
| this.variables.set(name, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
The abstract keyword exists and you should use it here (probably). But again, I really dislike the whole idea.
| import { Provider } from "./provider"; | ||
| import { window } from "vscode"; | ||
|
|
||
| export class NotebookBasedProvider extends Provider { | ||
| protected get notebookEditor() { | ||
| return window.activeNotebookEditor; | ||
| } |
There was a problem hiding this comment.
| import { Provider } from "./provider"; | |
| import { window } from "vscode"; | |
| export class NotebookBasedProvider extends Provider { | |
| protected get notebookEditor() { | |
| return window.activeNotebookEditor; | |
| } | |
| import { Provider } from "./provider"; | |
| import { type NotebookEditor, window } from "vscode"; | |
| export class NotebookBasedProvider extends Provider { | |
| protected get notebookEditor(): NotebookEditor | undefined { | |
| return window.activeNotebookEditor; | |
| } |
|
|
||
| protected override onExtensionActivate(): void { | ||
| this.gitApi = extensions | ||
| .getExtension<GitExtension>("vscode.git") |
There was a problem hiding this comment.
Looks like the Git bug made a comeback. Please call .activate(): Promise<GitExtension> and await it.
Actually, see #402 (comment).
| ); | ||
|
|
||
| if (Object.values(this.extensionState).some((x) => !x)) this.onExtensionDeactivate(); | ||
| else this.onExtensionActivate(); |
There was a problem hiding this comment.
You cannot use await for Git here though (async constructor).
| else this.onExtensionActivate(); | |
| else await this.onExtensionActivate(); // useless suggestion |
|
|
||
| const isAnyInactive = filtered.some((x) => !x.isActive); | ||
| if (isAnyInactive) this.onExtensionDeactivate(); | ||
| else this.onExtensionActivate(); |
There was a problem hiding this comment.
| else this.onExtensionActivate(); | |
| await else this.onExtensionActivate(); |
| extensions.onDidChange(() => { | ||
| this.updateState(); | ||
| }) |
There was a problem hiding this comment.
updateState waits for the Git extension to load, which is incorrect because Git should be loaded only once. This is the Git bug for code placed after updateState.
| extensions.onDidChange(() => { | |
| this.updateState(); | |
| }) | |
| extensions.onDidChange(async () => { | |
| await this.updateState(); | |
| }) |
| public addProvider(provider: Provider) { | ||
| this.providers.push(provider); | ||
| this.providers.sort((a, b) => a.priority - b.priority); | ||
| if (this.extension.activated) provider.subscribe(); | ||
| } |
There was a problem hiding this comment.
It can be faster by using a proper insert function, this code reminds me of microsoft/vscode#272155.
|
hey @xhayper can you continue this pr? tysm |
3 participants
No description provided.