Skip to content

Commit c9b52e6

Browse files
DonJayamanneDonJayamanne
authored
Fixes to allowing users to open a diff view for ipynb files (#8081)
* Fixes to allowing users to open a diff view * Updated code review comments
1 parent 14de5ff commit c9b52e6

2 files changed

Lines changed: 87 additions & 18 deletions

File tree

‎src/client/datascience/interactive-ipynb/nativeEditorProvider.ts‎

Lines changed: 76 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
'use strict';
44
import { inject, injectable } from 'inversify';
55
import * as path from 'path';
6-
import { TextDocument, Uri } from 'vscode';
6+
import { TextDocument, TextEditor, Uri } from 'vscode';
77

88
import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../common/application/types';
99
import { JUPYTER_LANGUAGE } from '../../common/constants';
@@ -21,7 +21,6 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
2121
private executedEditors: Set<string> = new Set<string>();
2222
private notebookCount: number = 0;
2323
private openedNotebookCount: number = 0;
24-
2524
constructor(
2625
@inject(IServiceContainer) private serviceContainer: IServiceContainer,
2726
@inject(IAsyncDisposableRegistry) asyncRegistry: IAsyncDisposableRegistry,
@@ -45,16 +44,14 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
4544
findFilesPromise.then(r => this.notebookCount += r.length);
4645
}
4746

48-
// Listen to document open commands. We use this to launch an ipynb editor
49-
const disposable = this.documentManager.onDidOpenTextDocument(this.onOpenedDocument);
50-
this.disposables.push(disposable);
47+
this.disposables.push(this.documentManager.onDidChangeActiveTextEditor(this.onDidChangeActiveTextEditorHandler.bind(this)));
5148

5249
// Since we may have activated after a document was opened, also run open document for all documents.
5350
// This needs to be async though. Iterating over all of these in the .ctor is crashing the extension
5451
// host, so postpone till after the ctor is finished.
5552
setTimeout(() => {
5653
if (this.documentManager.textDocuments && this.documentManager.textDocuments.forEach) {
57-
this.documentManager.textDocuments.forEach(this.onOpenedDocument);
54+
this.documentManager.textDocuments.forEach(doc => this.openNotebookAndCloseEditor(doc, false));
5855
}
5956
}, 0);
6057

@@ -132,6 +129,19 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
132129
purpose: Identifiers.HistoryPurpose // Share the same one as the interactive window. Just need a new session
133130
};
134131
}
132+
/**
133+
* Open ipynb files when user opens an ipynb file.
134+
*
135+
* @private
136+
* @memberof NativeEditorProvider
137+
*/
138+
private onDidChangeActiveTextEditorHandler(editor?: TextEditor){
139+
// I we're a source control diff view, then ignore this editor.
140+
if (!editor || this.isEditorPartOfDiffView(editor)){
141+
return;
142+
}
143+
this.openNotebookAndCloseEditor(editor.document, true).ignoreErrors();
144+
}
135145

136146
private async create(file: Uri, contents: string): Promise<INotebookEditor> {
137147
const editor = this.serviceContainer.get<INotebookEditor>(INotebookEditor);
@@ -178,27 +188,79 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
178188
return Uri.file(`${localize.DataScience.untitledNotebookFileName()}-${number}`);
179189
}
180190

181-
private onOpenedDocument = async (document: TextDocument) => {
191+
private openNotebookAndCloseEditor = async (document: TextDocument, closeDocumentBeforeOpeningNotebook: boolean) => {
182192
// See if this is an ipynb file
183-
if (this.isNotebook(document) && this.configuration.getSettings().datascience.useNotebookEditor) {
193+
if (this.isNotebook(document) && this.configuration.getSettings().datascience.useNotebookEditor &&
194+
!this.activeEditors.has(document.uri.fsPath)) {
184195
try {
185-
const contents = document.getText();
196+
const contents = document. getText();
186197
const uri = document.uri;
198+
const closeActiveEditorCommand = 'workbench.action.closeActiveEditor';
199+
200+
if (closeDocumentBeforeOpeningNotebook) {
201+
if (!this.documentManager.activeTextEditor || this.documentManager.activeTextEditor.document !== document){
202+
await this.documentManager.showTextDocument(document);
203+
}
204+
await this.cmdManager.executeCommand(closeActiveEditorCommand);
205+
}
187206

188207
// Open our own editor.
189208
await this.open(uri, contents);
190209

191-
// Then switch back to the ipynb and close it.
192-
// If we don't do it in this order, the close will switch to the wrong item
193-
await this.documentManager.showTextDocument(document);
194-
const command = 'workbench.action.closeActiveEditor';
195-
await this.cmdManager.executeCommand(command);
210+
if (!closeDocumentBeforeOpeningNotebook){
211+
// Then switch back to the ipynb and close it.
212+
// If we don't do it in this order, the close will switch to the wrong item
213+
await this.documentManager.showTextDocument(document);
214+
await this.cmdManager.executeCommand(closeActiveEditorCommand);
215+
}
196216
} catch (e) {
197217
return this.dataScienceErrorHandler.handleError(e);
198218
}
199219
}
200220
}
221+
/**
222+
* Check if user is attempting to compare two ipynb files.
223+
* If yes, then return `true`, else `false`.
224+
*
225+
* @private
226+
* @param {TextEditor} editor
227+
* @memberof NativeEditorProvider
228+
*/
229+
private isEditorPartOfDiffView(editor?: TextEditor){
230+
if (!editor){
231+
return false;
232+
}
233+
// There's no easy way to determine if the user is openeing a diff view.
234+
// One simple way is to check if there are 2 editor opened, and if both editors point to the same file
235+
// One file with the `file` scheme and the other with the `git` scheme.
236+
if (this.documentManager.visibleTextEditors.length <= 1){
237+
return false;
238+
}
239+
240+
// If we have both `git` & `file` schemes for the same file, then we're most likely looking at a diff view.
241+
// Also ensure both editors are in the same view column.
242+
// Possible we have a git diff view (with two editors git and file scheme), and we open the file view
243+
// on the side (different view column).
244+
const gitSchemeEditor = this.documentManager.visibleTextEditors.find(editorUri =>
245+
editorUri.document.uri.scheme === 'git' &&
246+
this.fileSystem.arePathsSame(editorUri.document.uri.fsPath, editor.document.uri.fsPath));
247+
248+
if (!gitSchemeEditor){
249+
return false;
250+
}
201251

252+
const fileSchemeEditor = this.documentManager.visibleTextEditors.find(editorUri =>
253+
editorUri.document.uri.scheme === 'file' &&
254+
this.fileSystem.arePathsSame(editorUri.document.uri.fsPath, editor.document.uri.fsPath) &&
255+
editorUri.viewColumn === gitSchemeEditor.viewColumn);
256+
if (!fileSchemeEditor){
257+
return false;
258+
}
259+
260+
// Also confirm the document we have passed in, belongs to one of the editors.
261+
// If its not, then its another document (that is not in the diff view).
262+
return gitSchemeEditor === editor || fileSchemeEditor === editor;
263+
}
202264
private isNotebook(document: TextDocument) {
203265
// Only support file uris (we don't want to automatically open any other ipynb file from another resource as a notebook).
204266
// E.g. when opening a document for comparison, the scheme is `git`, in live share the scheme is `vsls`.

‎src/test/datascience/interactive-ipynb/nativeEditorProvider.unit.test.ts‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import { expect } from 'chai';
99
import { instance, mock, when } from 'ts-mockito';
1010
import * as typemoq from 'typemoq';
11-
import { EventEmitter, TextDocument, Uri } from 'vscode';
11+
import { EventEmitter, TextDocument, TextEditor, Uri } from 'vscode';
1212
import { CommandManager } from '../../../client/common/application/commandManager';
1313
import { DocumentManager } from '../../../client/common/application/documentManager';
1414
import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../../client/common/application/types';
@@ -65,11 +65,17 @@ suite('Data Science - Native Editor Provider', () => {
6565
textDocument.setup(t => t.getText()).returns(() => content);
6666
return textDocument.object;
6767
}
68+
function createTextEditor(doc: TextDocument) {
69+
const textEditor = typemoq.Mock.ofType<TextEditor>();
70+
textEditor.setup(e => e.document).returns(() => doc);
71+
return textEditor.object;
72+
}
6873
async function testAutomaticallyOpeningNotebookEditorWhenOpeningFiles(uri: Uri, shouldOpenNotebookEditor: boolean) {
69-
const eventEmitter = new EventEmitter<TextDocument>();
74+
const eventEmitter = new EventEmitter<TextEditor>();
7075
const editor = typemoq.Mock.ofType<INotebookEditor>();
7176
when(configService.getSettings()).thenReturn({ datascience: { useNotebookEditor: true } } as any);
72-
when(doctManager.onDidOpenTextDocument).thenReturn(eventEmitter.event);
77+
when(doctManager.onDidChangeActiveTextEditor).thenReturn(eventEmitter.event);
78+
when(doctManager.visibleTextEditors).thenReturn([]);
7379
editor.setup(e => e.closed).returns(() => new EventEmitter<INotebookEditor>().event);
7480
editor.setup(e => e.executed).returns(() => new EventEmitter<INotebookEditor>().event);
7581
editor.setup(e => (e as any).then).returns(() => undefined);
@@ -90,7 +96,8 @@ suite('Data Science - Native Editor Provider', () => {
9096

9197
// Open a text document.
9298
const textDoc = createTextDocument(uri, 'hello');
93-
eventEmitter.fire(textDoc);
99+
const textEditor = createTextEditor(textDoc);
100+
eventEmitter.fire(textEditor);
94101

95102
// wait for callbacks to get executed.
96103
await sleep(1);

0 commit comments

Comments
 (0)