Skip to content

Commit 3d30620

Browse files
colin-grant-workcolin-grant-work
authored
Improve dirty diff & avoid instantiation service leakage (#15238)
* Ensures that a valid InstantiationService is available for hover delegate creation * Improves dirty diff functionality to work when editor modified
1 parent 59ea154 commit 3d30620

File tree

7 files changed

+138
-36
lines changed

7 files changed

+138
-36
lines changed

‎packages/monaco/src/browser/monaco-editor-peek-view-widget.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,14 @@ export class MonacoEditorPeekViewWidget {
9090
return this._actionbarWidget;
9191
}
9292

93+
fillContainer(container: HTMLElement): void {
94+
super._fillContainer(container);
95+
}
96+
97+
protected override _fillContainer(container: HTMLElement): void {
98+
that.fillContainer(container);
99+
}
100+
93101
fillHead(container: HTMLElement, noCloseAction?: boolean): void {
94102
super._fillHead(container, noCloseAction);
95103
}
@@ -137,6 +145,26 @@ export class MonacoEditorPeekViewWidget {
137145
protected override revealRange(range: monaco.Range, isLastLine: boolean): void {
138146
that.doRevealRange(that.editor['m2p'].asRange(range), isLastLine);
139147
}
148+
149+
getBodyElement(): HTMLDivElement | undefined {
150+
return this._bodyElement;
151+
}
152+
153+
setBodyElement(element: HTMLDivElement | undefined): void {
154+
this._bodyElement = element;
155+
}
156+
157+
getHeadElement(): HTMLDivElement | undefined {
158+
return this._headElement;
159+
}
160+
161+
setHeadElement(element: HTMLDivElement | undefined): void {
162+
this._headElement = element;
163+
}
164+
165+
override setCssClass(className: string, classToReplace?: string | undefined): void {
166+
super.setCssClass(className, classToReplace);
167+
}
140168
}(
141169
editor.getControl() as unknown as ICodeEditor,
142170
Object.assign(<IPeekViewOptions>{}, options, this.convertStyles(styles)),
@@ -185,6 +213,10 @@ export class MonacoEditorPeekViewWidget {
185213
return action;
186214
}
187215

216+
protected fillContainer(container: HTMLElement): void {
217+
this.delegate.fillContainer(container);
218+
}
219+
188220
protected fillHead(container: HTMLElement, noCloseAction?: boolean): void {
189221
this.delegate.fillHead(container, noCloseAction);
190222
}
@@ -209,6 +241,26 @@ export class MonacoEditorPeekViewWidget {
209241
this.delegate.doRevealRange(this.editor['p2m'].asRange(range), isLastLine);
210242
}
211243

244+
protected get bodyElement(): HTMLDivElement | undefined {
245+
return this.delegate.getBodyElement();
246+
}
247+
248+
protected set bodyElement(element: HTMLDivElement | undefined) {
249+
this.delegate.setBodyElement(element);
250+
}
251+
252+
protected get headElement(): HTMLDivElement | undefined {
253+
return this.delegate.getHeadElement();
254+
}
255+
256+
protected set headElement(element: HTMLDivElement | undefined) {
257+
this.delegate.setHeadElement(element);
258+
}
259+
260+
protected setCssClass(className: string, classToReplace?: string | undefined): void {
261+
this.delegate.setCssClass(className, classToReplace);
262+
}
263+
212264
private convertStyles(styles: MonacoEditorPeekViewWidget.Styles): IPeekViewStyles {
213265
return {
214266
frameColor: this.convertColor(styles.frameColor),

‎packages/monaco/src/browser/monaco-editor-provider.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -424,7 +424,7 @@ export class MonacoEditorProvider {
424424
fixedOverflowWidgets: true,
425425
minimap: { enabled: false },
426426
renderSideBySide: false,
427-
readOnly: true,
427+
readOnly: false,
428428
renderIndicators: false,
429429
diffAlgorithm: 'advanced',
430430
stickyScroll: { enabled: false },

‎packages/monaco/src/browser/monaco-editor.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,8 @@ import { ILanguageConfigurationService } from '@theia/monaco-editor-core/esm/vs/
6363
import { ILanguageFeaturesService } from '@theia/monaco-editor-core/esm/vs/editor/common/services/languageFeatures';
6464
import * as objects from '@theia/monaco-editor-core/esm/vs/base/common/objects';
6565
import { Selection } from '@theia/editor/lib/browser/editor';
66-
import { IHoverService } from '@theia/monaco-editor-core/esm/vs/platform/hover/browser/hover';
66+
import { IHoverService, WorkbenchHoverDelegate } from '@theia/monaco-editor-core/esm/vs/platform/hover/browser/hover';
67+
import { setHoverDelegateFactory } from '@theia/monaco-editor-core/esm/vs/base/browser/ui/hover/hoverDelegateFactory';
6768
import { MonacoTextModelService } from './monaco-text-model-service';
6869

6970
export type ServicePair<T> = [ServiceIdentifier<T>, T];
@@ -155,6 +156,11 @@ export class MonacoEditor extends MonacoEditorServices implements TextEditor {
155156
...MonacoEditor.createReadOnlyOptions(document.readOnly),
156157
...options
157158
}, override));
159+
// Ensure that a valid InstantiationService is responsible for creating hover delegates when the InstantiationService for this widget is disposed.
160+
// Cf. https://github.com/eclipse-theia/theia/issues/15102
161+
this.toDispose.push(Disposable.create(() => setHoverDelegateFactory((placement, enableInstantHover) =>
162+
StandaloneServices.get(IInstantiationService).createInstance(WorkbenchHoverDelegate, placement, enableInstantHover, {})
163+
)));
158164
this.addHandlers(this.editor);
159165
}
160166

‎packages/scm/src/browser/decorations/scm-decorations-service.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ export class ScmDecorationsService {
8080
const previousLines = ContentLines.fromString(previousContent);
8181
const currentLines = ContentLines.fromTextEditorDocument(editor.document);
8282
const dirtyDiff = this.diffComputer.computeDirtyDiff(ContentLines.arrayLike(previousLines), ContentLines.arrayLike(currentLines));
83-
const update = <DirtyDiffUpdate>{ editor, previousRevisionUri: uri, ...dirtyDiff };
83+
const update = { editor, previousRevisionUri: uri, ...dirtyDiff } satisfies DirtyDiffUpdate;
8484
this.decorator.applyDecorations(update);
8585
this.onDirtyDiffUpdateEmitter.fire(update);
8686
} finally {

‎packages/scm/src/browser/dirty-diff/dirty-diff-navigator.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,10 @@ export class DirtyDiffController implements Disposable {
135135

136136
handleDirtyDiffUpdate(dirtyDiff: DirtyDiffUpdate): void {
137137
if (dirtyDiff.editor === this.editor) {
138-
this.closeWidget();
139138
this.dirtyDiff = dirtyDiff;
139+
if (this.widget) {
140+
this.widget.changes = dirtyDiff.changes ;
141+
}
140142
}
141143
}
142144

@@ -210,7 +212,8 @@ export class DirtyDiffController implements Disposable {
210212
protected createWidget(): DirtyDiffWidget | undefined {
211213
const { widgetFactory, editor, changes, previousRevisionUri } = this;
212214
if (widgetFactory && editor instanceof MonacoEditor && changes?.length && previousRevisionUri) {
213-
const widget = widgetFactory({ editor, previousRevisionUri, changes });
215+
const widget = widgetFactory({ editor, previousRevisionUri });
216+
widget.changes = changes;
214217
widget.onDidClose(() => {
215218
this.widget = undefined;
216219
});

‎packages/scm/src/browser/dirty-diff/dirty-diff-widget.ts‎

Lines changed: 70 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ export const DirtyDiffWidgetProps = Symbol('DirtyDiffWidgetProps');
3636
export interface DirtyDiffWidgetProps {
3737
readonly editor: MonacoEditor;
3838
readonly previousRevisionUri: URI;
39-
readonly changes: readonly Change[];
4039
}
4140

4241
export const DirtyDiffWidgetFactory = Symbol('DirtyDiffWidgetFactory');
@@ -48,8 +47,9 @@ export class DirtyDiffWidget implements Disposable {
4847
private readonly onDidCloseEmitter = new Emitter<unknown>();
4948
readonly onDidClose: Event<unknown> = this.onDidCloseEmitter.event;
5049
protected index: number = -1;
51-
private peekView?: DirtyDiffPeekView;
52-
private diffEditorPromise?: Promise<MonacoDiffEditor>;
50+
private peekView: DirtyDiffPeekView;
51+
private diffEditorPromise: Promise<MonacoDiffEditor>;
52+
protected _changes?: readonly Change[];
5353

5454
constructor(
5555
@inject(DirtyDiffWidgetProps) protected readonly props: DirtyDiffWidgetProps,
@@ -66,6 +66,14 @@ export class DirtyDiffWidget implements Disposable {
6666
this.diffEditorPromise = this.peekView.create();
6767
}
6868

69+
get changes(): readonly Change[] {
70+
return this._changes ?? [];
71+
}
72+
73+
set changes(changes: readonly Change[]) {
74+
this.handleChangedChanges(changes);
75+
}
76+
6977
get editor(): MonacoEditor {
7078
return this.props.editor;
7179
}
@@ -78,10 +86,6 @@ export class DirtyDiffWidget implements Disposable {
7886
return this.props.previousRevisionUri;
7987
}
8088

81-
get changes(): readonly Change[] {
82-
return this.props.changes;
83-
}
84-
8589
get currentChange(): Change | undefined {
8690
return this.changes[this.index];
8791
}
@@ -90,8 +94,30 @@ export class DirtyDiffWidget implements Disposable {
9094
return this.index;
9195
}
9296

93-
showChange(index: number): void {
94-
this.checkCreated();
97+
protected handleChangedChanges(updated: readonly Change[]): void {
98+
if (!updated.length) {
99+
return this.dispose();
100+
}
101+
if (this.currentChange) {
102+
const { previousRange: { start, end } } = this.currentChange;
103+
// Same change or first after it.
104+
const newIndex = updated.findIndex(candidate => (candidate.previousRange.start === start && candidate.previousRange.end === end)
105+
|| candidate.previousRange.start > start);
106+
if (newIndex !== -1) {
107+
this.index = newIndex;
108+
} else {
109+
this.index = Math.min(this.index, updated.length - 1);
110+
}
111+
this.showCurrentChange();
112+
} else {
113+
this.index = -1;
114+
}
115+
this._changes = updated;
116+
this.updateHeading();
117+
}
118+
119+
async showChange(index: number): Promise<void> {
120+
await this.checkCreated();
95121
if (index >= 0 && index < this.changes.length) {
96122
this.index = index;
97123
this.showCurrentChange();
@@ -119,7 +145,7 @@ export class DirtyDiffWidget implements Disposable {
119145
}
120146

121147
async getContentWithSelectedChanges(predicate: (change: Change, index: number, changes: readonly Change[]) => boolean): Promise<string> {
122-
this.checkCreated();
148+
await this.checkCreated();
123149
const changes = this.changes.filter(predicate);
124150
const { diffEditor } = await this.diffEditorPromise!;
125151
const diffEditorModel = diffEditor.getModel()!;
@@ -132,11 +158,11 @@ export class DirtyDiffWidget implements Disposable {
132158
}
133159

134160
protected showCurrentChange(): void {
135-
this.peekView!.setTitle(this.computePrimaryHeading(), this.computeSecondaryHeading());
161+
this.updateHeading();
136162
const { previousRange, currentRange } = this.changes[this.index];
137-
this.peekView!.show(Position.create(LineRange.getEndPosition(currentRange).line, 0),
163+
this.peekView.show(Position.create(LineRange.getEndPosition(currentRange).line, 0),
138164
this.computeHeightInLines());
139-
this.diffEditorPromise!.then(({ diffEditor }) => {
165+
this.diffEditorPromise.then(({ diffEditor }) => {
140166
let startLine = LineRange.getStartPosition(currentRange).line;
141167
let endLine = LineRange.getEndPosition(currentRange).line;
142168
if (LineRange.isEmpty(currentRange)) { // the change is a removal
@@ -151,6 +177,10 @@ export class DirtyDiffWidget implements Disposable {
151177
this.editor.focus();
152178
}
153179

180+
protected updateHeading(): void {
181+
this.peekView.setTitle(this.computePrimaryHeading(), this.computeSecondaryHeading());
182+
}
183+
154184
protected computePrimaryHeading(): string {
155185
return this.uri.path.base;
156186
}
@@ -174,10 +204,8 @@ export class DirtyDiffWidget implements Disposable {
174204
return Math.min(changeHeightInLines + /* padding */ 8, Math.floor(editorHeightInLines / 3));
175205
}
176206

177-
protected checkCreated(): void {
178-
if (!this.peekView) {
179-
throw new Error('create() method needs to be called first.');
180-
}
207+
protected async checkCreated(): Promise<MonacoDiffEditor> {
208+
return this.diffEditorPromise;
181209
}
182210
}
183211

@@ -250,7 +278,7 @@ function applyChanges(changes: readonly Change[], original: monaco.editor.ITextM
250278

251279
class DirtyDiffPeekView extends MonacoEditorPeekViewWidget {
252280

253-
private diffEditorPromise?: Promise<MonacoDiffEditor>;
281+
private diffEditor?: MonacoDiffEditor;
254282
private height?: number;
255283

256284
constructor(readonly widget: DirtyDiffWidget) {
@@ -259,12 +287,16 @@ class DirtyDiffPeekView extends MonacoEditorPeekViewWidget {
259287

260288
override async create(): Promise<MonacoDiffEditor> {
261289
try {
290+
this.bodyElement = document.createElement('div');
291+
this.bodyElement.classList.add('body');
292+
const diffEditor = await this.widget.editorProvider.createEmbeddedDiffEditor(this.editor, this.bodyElement, this.widget.previousRevisionUri);
293+
this.diffEditor = diffEditor;
294+
this.toDispose.push(diffEditor);
262295
super.create();
263-
const diffEditor = await this.diffEditorPromise!;
264296
return new Promise(resolve => {
265-
// setTimeout is needed here because the non-side-by-side diff editor might still not have created the view zones;
266-
// otherwise, the first change shown might not be properly revealed in the diff editor.
267-
// see also https://github.com/microsoft/vscode/blob/b30900b56c4b3ca6c65d7ab92032651f4cb23f15/src/vs/workbench/contrib/scm/browser/dirtydiffDecorator.ts#L248
297+
// setTimeout is needed here because the non-side-by-side diff editor might still not have created the view zones;
298+
// otherwise, the first change shown might not be properly revealed in the diff editor.
299+
// see also https://github.com/microsoft/vscode/blob/b30900b56c4b3ca6c65d7ab92032651f4cb23f15/src/vs/workbench/contrib/scm/browser/dirtydiffDecorator.ts#L248
268300
const disposable = diffEditor.diffEditor.onDidUpdateDiff(() => setTimeout(() => {
269301
resolve(diffEditor);
270302
disposable.dispose();
@@ -313,8 +345,10 @@ class DirtyDiffPeekView extends MonacoEditorPeekViewWidget {
313345
if (item instanceof ActionMenuNode) {
314346
const { command, id, label, icon, when } = item;
315347
if (icon && menuCommandExecutor.isVisible(menuPath, command, this.widget) && (!when || contextKeyService.match(when))) {
348+
// Close editor on successful contributed action.
349+
// https://github.com/microsoft/vscode/blob/11b1500e0a2e8b5ba12e98a3905f9d120b8646a0/src/vs/workbench/contrib/scm/browser/quickDiffWidget.ts#L356-L361
316350
this.addAction(id, label, icon, menuCommandExecutor.isEnabled(menuPath, command, this.widget), () => {
317-
menuCommandExecutor.executeCommand(menuPath, command, this.widget);
351+
menuCommandExecutor.executeCommand(menuPath, command, this.widget).then(() => this.dispose());
318352
});
319353
}
320354
}
@@ -329,15 +363,20 @@ class DirtyDiffPeekView extends MonacoEditorPeekViewWidget {
329363
() => this.dispose());
330364
}
331365

332-
protected override fillHead(container: HTMLElement): void {
333-
super.fillHead(container, true);
366+
protected override fillContainer(container: HTMLElement): void {
367+
this.setCssClass('peekview-widget');
368+
369+
this.headElement = document.createElement('div');
370+
this.headElement.classList.add('head');
371+
372+
container.appendChild(this.headElement);
373+
container.appendChild(this.bodyElement!);
374+
375+
this.fillHead(this.headElement);
334376
}
335377

336-
protected override fillBody(container: HTMLElement): void {
337-
this.diffEditorPromise = this.widget.editorProvider.createEmbeddedDiffEditor(this.editor, container, this.widget.previousRevisionUri).then(diffEditor => {
338-
this.toDispose.push(diffEditor);
339-
return diffEditor;
340-
});
378+
protected override fillHead(container: HTMLElement): void {
379+
super.fillHead(container, true);
341380
}
342381

343382
protected override doLayoutBody(height: number, width: number): void {
@@ -355,7 +394,7 @@ class DirtyDiffPeekView extends MonacoEditorPeekViewWidget {
355394
}
356395

357396
private layout(height: number, width: number): void {
358-
this.diffEditorPromise?.then(({ diffEditor }) => diffEditor.layout({ height, width }));
397+
this.diffEditor?.diffEditor.layout({ height, width });
359398
}
360399

361400
protected override doRevealRange(range: Range): void {

‎packages/scm/src/browser/scm-contribution.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,7 @@ export class ScmContribution extends AbstractViewContribution<ScmWidget> impleme
204204
execute: widget => {
205205
if (widget instanceof EditorWidget && widget.editor instanceof MonacoDiffEditor) {
206206
widget.editor.diffNavigator.next();
207+
widget.activate();
207208
} else {
208209
this.dirtyDiffNavigator.gotoNextChange();
209210
}
@@ -219,6 +220,7 @@ export class ScmContribution extends AbstractViewContribution<ScmWidget> impleme
219220
execute: widget => {
220221
if (widget instanceof EditorWidget && widget.editor instanceof MonacoDiffEditor) {
221222
widget.editor.diffNavigator.previous();
223+
widget.activate();
222224
} else {
223225
this.dirtyDiffNavigator.gotoPreviousChange();
224226
}

0 commit comments

Comments
 (0)