Skip to content

Ensure valid editor constructed before attempting to use editor services #15238

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.

Sign up for GitHub

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
colin-grant-work merged 6 commits into from
Mar 28, 2025
Merged

Ensure valid editor constructed before attempting to use editor services #15238

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e0dc27d
Ensure editor created before services used
colin-grant-work Mar 20, 2025
6d35a44
Lint
colin-grant-work Mar 25, 2025
118daf9
Close editor on successful action, not any change
colin-grant-work Mar 25, 2025
e2cf80c
Update DirtyDiffWidget change model when changes change
colin-grant-work Mar 25, 2025
d809c93
Update headings; jump in main editor; close if no changes
colin-grant-work Mar 26, 2025
fc8aa2d
Use the global instantiation service on disposal
colin-grant-work Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Ensure editor created before services used
  • Loading branch information
@colin-grant-work
colin-grant-work committed Mar 26, 2025
commit e0dc27d4b7b963a87214f8e0d47b466891775d6e
52 changes: 52 additions & 0 deletions packages/monaco/src/browser/monaco-editor-peek-view-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,14 @@ export class MonacoEditorPeekViewWidget {
return this._actionbarWidget;
}

fillContainer(container: HTMLElement): void {
super._fillContainer(container);
}

protected override _fillContainer(container: HTMLElement): void {
that.fillContainer(container);
}

fillHead(container: HTMLElement, noCloseAction?: boolean): void {
super._fillHead(container, noCloseAction);
}
Expand Down Expand Up @@ -137,6 +145,26 @@ export class MonacoEditorPeekViewWidget {
protected override revealRange(range: monaco.Range, isLastLine: boolean): void {
that.doRevealRange(that.editor['m2p'].asRange(range), isLastLine);
}

getBodyElement(): HTMLDivElement | undefined {
return this._bodyElement;
}

setBodyElement(element: HTMLDivElement | undefined): void {
this._bodyElement = element;
}

getHeadElement(): HTMLDivElement | undefined {
return this._headElement;
}

setHeadElement(element: HTMLDivElement | undefined): void {
this._headElement = element;
}

override setCssClass(className: string, classToReplace?: string | undefined): void {
super.setCssClass(className, classToReplace);
}
}(
editor.getControl() as unknown as ICodeEditor,
Object.assign(<IPeekViewOptions>{}, options, this.convertStyles(styles)),
Expand Down Expand Up @@ -185,6 +213,10 @@ export class MonacoEditorPeekViewWidget {
return action;
}

protected fillContainer(container: HTMLElement): void {
this.delegate.fillContainer(container)
}

protected fillHead(container: HTMLElement, noCloseAction?: boolean): void {
this.delegate.fillHead(container, noCloseAction);
}
Expand All @@ -209,6 +241,26 @@ export class MonacoEditorPeekViewWidget {
this.delegate.doRevealRange(this.editor['p2m'].asRange(range), isLastLine);
}

protected get bodyElement(): HTMLDivElement | undefined {
return this.delegate.getBodyElement();
}

protected set bodyElement(element: HTMLDivElement | undefined) {
this.delegate.setBodyElement(element);
}

protected get headElement(): HTMLDivElement | undefined {
return this.delegate.getHeadElement();
}

protected set headElement(element: HTMLDivElement | undefined) {
this.delegate.setHeadElement(element);
}

protected setCssClass(className: string, classToReplace?: string | undefined): void {
this.delegate.setCssClass(className, classToReplace);
}

private convertStyles(styles: MonacoEditorPeekViewWidget.Styles): IPeekViewStyles {
return {
frameColor: this.convertColor(styles.frameColor),
Expand Down
63 changes: 35 additions & 28 deletions packages/scm/src/browser/dirty-diff/dirty-diff-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ export class DirtyDiffWidget implements Disposable {
private readonly onDidCloseEmitter = new Emitter<unknown>();
readonly onDidClose: Event<unknown> = this.onDidCloseEmitter.event;
protected index: number = -1;
private peekView?: DirtyDiffPeekView;
private diffEditorPromise?: Promise<MonacoDiffEditor>;
private peekView: DirtyDiffPeekView;
private diffEditorPromise: Promise<MonacoDiffEditor>;

constructor(
@inject(DirtyDiffWidgetProps) protected readonly props: DirtyDiffWidgetProps,
Expand Down Expand Up @@ -90,16 +90,16 @@ export class DirtyDiffWidget implements Disposable {
return this.index;
}

showChange(index: number): void {
this.checkCreated();
async showChange(index: number): Promise<void> {
await this.checkCreated();
if (index >= 0 && index < this.changes.length) {
this.index = index;
this.showCurrentChange();
}
}

showNextChange(): void {
this.checkCreated();
async showNextChange(): Promise<void> {
await this.checkCreated();
const index = this.index;
const length = this.changes.length;
if (length > 0 && (index < 0 || length > 1)) {
Expand All @@ -108,8 +108,8 @@ export class DirtyDiffWidget implements Disposable {
}
}

showPreviousChange(): void {
this.checkCreated();
async showPreviousChange(): Promise<void> {
await this.checkCreated();
const index = this.index;
const length = this.changes.length;
if (length > 0 && (index < 0 || length > 1)) {
Expand All @@ -119,7 +119,7 @@ export class DirtyDiffWidget implements Disposable {
}

async getContentWithSelectedChanges(predicate: (change: Change, index: number, changes: readonly Change[]) => boolean): Promise<string> {
this.checkCreated();
await this.checkCreated();
const changes = this.changes.filter(predicate);
const { diffEditor } = await this.diffEditorPromise!;
const diffEditorModel = diffEditor.getModel()!;
Expand All @@ -134,9 +134,9 @@ export class DirtyDiffWidget implements Disposable {
protected showCurrentChange(): void {
this.peekView!.setTitle(this.computePrimaryHeading(), this.computeSecondaryHeading());
const { previousRange, currentRange } = this.changes[this.index];
this.peekView!.show(Position.create(LineRange.getEndPosition(currentRange).line, 0),
this.peekView.show(Position.create(LineRange.getEndPosition(currentRange).line, 0),
this.computeHeightInLines());
this.diffEditorPromise!.then(({ diffEditor }) => {
this.diffEditorPromise.then(({ diffEditor }) => {
let startLine = LineRange.getStartPosition(currentRange).line;
let endLine = LineRange.getEndPosition(currentRange).line;
if (LineRange.isEmpty(currentRange)) { // the change is a removal
Expand Down Expand Up @@ -174,10 +174,8 @@ export class DirtyDiffWidget implements Disposable {
return Math.min(changeHeightInLines + /* padding */ 8, Math.floor(editorHeightInLines / 3));
}

protected checkCreated(): void {
if (!this.peekView) {
throw new Error('create() method needs to be called first.');
}
protected async checkCreated(): Promise<void> {
await this.diffEditorPromise;
}
}

Expand Down Expand Up @@ -250,7 +248,7 @@ function applyChanges(changes: readonly Change[], original: monaco.editor.ITextM

class DirtyDiffPeekView extends MonacoEditorPeekViewWidget {

private diffEditorPromise?: Promise<MonacoDiffEditor>;
private diffEditor?: MonacoDiffEditor;
private height?: number;

constructor(readonly widget: DirtyDiffWidget) {
Expand All @@ -259,12 +257,16 @@ class DirtyDiffPeekView extends MonacoEditorPeekViewWidget {

override async create(): Promise<MonacoDiffEditor> {
try {
this.bodyElement = document.createElement('div');
this.bodyElement.classList.add('body');
const diffEditor = await this.widget.editorProvider.createEmbeddedDiffEditor(this.editor, this.bodyElement, this.widget.previousRevisionUri);
this.diffEditor = diffEditor;
this.toDispose.push(diffEditor);
super.create();
const diffEditor = await this.diffEditorPromise!;
return new Promise(resolve => {
// setTimeout is needed here because the non-side-by-side diff editor might still not have created the view zones;
// otherwise, the first change shown might not be properly revealed in the diff editor.
// see also https://github.com/microsoft/vscode/blob/b30900b56c4b3ca6c65d7ab92032651f4cb23f15/src/vs/workbench/contrib/scm/browser/dirtydiffDecorator.ts#L248
// setTimeout is needed here because the non-side-by-side diff editor might still not have created the view zones;
// otherwise, the first change shown might not be properly revealed in the diff editor.
// see also https://github.com/microsoft/vscode/blob/b30900b56c4b3ca6c65d7ab92032651f4cb23f15/src/vs/workbench/contrib/scm/browser/dirtydiffDecorator.ts#L248
const disposable = diffEditor.diffEditor.onDidUpdateDiff(() => setTimeout(() => {
resolve(diffEditor);
disposable.dispose();
Expand Down Expand Up @@ -329,15 +331,20 @@ class DirtyDiffPeekView extends MonacoEditorPeekViewWidget {
() => this.dispose());
}

protected override fillHead(container: HTMLElement): void {
super.fillHead(container, true);
protected override fillContainer(container: HTMLElement): void {
this.setCssClass('peekview-widget');

this.headElement = document.createElement('div');
this.headElement.classList.add('head');

container.appendChild(this.headElement);
container.appendChild(this.bodyElement!);

this.fillHead(this.headElement);
}

protected override fillBody(container: HTMLElement): void {
this.diffEditorPromise = this.widget.editorProvider.createEmbeddedDiffEditor(this.editor, container, this.widget.previousRevisionUri).then(diffEditor => {
this.toDispose.push(diffEditor);
return diffEditor;
});
protected override fillHead(container: HTMLElement): void {
super.fillHead(container, true);
}

protected override doLayoutBody(height: number, width: number): void {
Expand All @@ -355,7 +362,7 @@ class DirtyDiffPeekView extends MonacoEditorPeekViewWidget {
}

private layout(height: number, width: number): void {
this.diffEditorPromise?.then(({ diffEditor }) => diffEditor.layout({ height, width }));
this.diffEditor?.diffEditor.layout({ height, width });
}

protected override doRevealRange(range: Range): void {
Expand Down