From 1d3debb89793ecae001bf23b990d964a2dd06b50 Mon Sep 17 00:00:00 2001 From: Maddy <12754347+MaddyDev@users.noreply.github.com> Date: Wed, 10 Nov 2021 10:59:29 -0800 Subject: [PATCH] Maddy/edit mode events cleanup (#17636) * remove multiple events * correct preview check * add test --- .../browser/cellViews/code.component.ts | 7 ++-- .../browser/cellViews/textCell.component.ts | 22 ++++++------ .../notebook/browser/notebookEditor.ts | 6 ++-- .../test/electron-browser/cell.test.ts | 36 ++++++++++++++++++- .../services/notebook/browser/models/cell.ts | 12 ------- .../browser/models/modelInterfaces.ts | 2 -- 6 files changed, 52 insertions(+), 33 deletions(-) diff --git a/src/sql/workbench/contrib/notebook/browser/cellViews/code.component.ts b/src/sql/workbench/contrib/notebook/browser/cellViews/code.component.ts index 2dec5f2d19..38a4b9da69 100644 --- a/src/sql/workbench/contrib/notebook/browser/cellViews/code.component.ts +++ b/src/sql/workbench/contrib/notebook/browser/cellViews/code.component.ts @@ -7,7 +7,7 @@ import 'vs/css!./code'; import { OnInit, Component, Input, Inject, ElementRef, ViewChild, Output, EventEmitter, OnChanges, SimpleChange, forwardRef, ChangeDetectorRef } from '@angular/core'; import { QueryTextEditor } from 'sql/workbench/browser/modelComponents/queryTextEditor'; -import { ICellModel, CellExecutionState } from 'sql/workbench/services/notebook/browser/models/modelInterfaces'; +import { ICellModel, CellExecutionState, CellEditModes } from 'sql/workbench/services/notebook/browser/models/modelInterfaces'; import { Taskbar } from 'sql/base/browser/ui/taskbar/taskbar'; import { RunCellAction, CellContext } from 'sql/workbench/contrib/notebook/browser/cellViews/codeActions'; import { NotebookModel } from 'sql/workbench/services/notebook/browser/models/notebookModel'; @@ -262,8 +262,9 @@ export class CodeComponent extends CellView implements OnInit, OnChanges { this._register(this.cellModel.onCollapseStateChanged(isCollapsed => { this.onCellCollapse(isCollapsed); })); - this._register(this.cellModel.onCellPreviewModeChanged((e) => { - if (!e && this._cellModel.cellSourceChanged) { + this._register(this.cellModel.onCurrentEditModeChanged((e) => { + let preview = e !== CellEditModes.MARKDOWN; + if (!preview && this._cellModel.cellSourceChanged) { this.updateModel(); this._cellModel.cellSourceChanged = false; } diff --git a/src/sql/workbench/contrib/notebook/browser/cellViews/textCell.component.ts b/src/sql/workbench/contrib/notebook/browser/cellViews/textCell.component.ts index 4c304fdf67..8a691f7f88 100644 --- a/src/sql/workbench/contrib/notebook/browser/cellViews/textCell.component.ts +++ b/src/sql/workbench/contrib/notebook/browser/cellViews/textCell.component.ts @@ -215,7 +215,15 @@ export class TextCellComponent extends CellView implements OnInit, OnChanges { } this._changeRef.detectChanges(); })); - this._register(this.cellModel.onCellPreviewModeChanged(preview => { + this._register(this.cellModel.onCurrentEditModeChanged(editMode => { + let markdown: boolean = editMode !== CellEditModes.WYSIWYG; + if (!markdown) { + let editorControl = this.cellEditors.length > 0 ? this.cellEditors[0].getEditor().getControl() : undefined; + if (editorControl) { + let selection = editorControl.getSelection(); + this.cellModel.markdownCursorPosition = selection?.getPosition(); + } + } // On preview mode change, get the cursor position (get the position only when the selection node is a text node) if (window.getSelection() && window.getSelection().focusNode?.nodeName === '#text' && window.getSelection().getRangeAt(0)) { let selection = window.getSelection().getRangeAt(0); @@ -247,17 +255,7 @@ export class TextCellComponent extends CellView implements OnInit, OnChanges { this.cellModel.richTextCursorPosition = cursorPosition; } } - this.previewMode = preview; - this.focusIfPreviewMode(); - })); - this._register(this.cellModel.onCellMarkdownModeChanged(markdown => { - if (!markdown) { - let editorControl = this.cellEditors.length > 0 ? this.cellEditors[0].getEditor().getControl() : undefined; - if (editorControl) { - let selection = editorControl.getSelection(); - this.cellModel.markdownCursorPosition = selection?.getPosition(); - } - } + this.previewMode = editMode !== CellEditModes.MARKDOWN; this.markdownMode = markdown; this.focusIfPreviewMode(); })); diff --git a/src/sql/workbench/contrib/notebook/browser/notebookEditor.ts b/src/sql/workbench/contrib/notebook/browser/notebookEditor.ts index fee981ca2a..6280474c6b 100644 --- a/src/sql/workbench/contrib/notebook/browser/notebookEditor.ts +++ b/src/sql/workbench/contrib/notebook/browser/notebookEditor.ts @@ -27,7 +27,7 @@ import { NotebookFindNextAction, NotebookFindPreviousAction } from 'sql/workbenc import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding'; import { IContextKeyService } from 'vs/platform/contextkey/common/contextkey'; import { IWorkbenchThemeService } from 'vs/workbench/services/themes/common/workbenchThemeService'; -import { INotebookModel } from 'sql/workbench/services/notebook/browser/models/modelInterfaces'; +import { CellEditModes, INotebookModel } from 'sql/workbench/services/notebook/browser/models/modelInterfaces'; import { INotebookFindModel } from 'sql/workbench/contrib/notebook/browser/models/notebookFindModel'; import { IDisposable, DisposableStore } from 'vs/base/common/lifecycle'; import { IModelDecorationsChangeAccessor, IModelDeltaDecoration } from 'vs/editor/common/model'; @@ -380,8 +380,8 @@ export class NotebookEditor extends EditorPane implements IFindNotebookControlle this._onFindStateChange(changeEvent).catch(onUnexpectedError); } })); - this._register(cell.onCellMarkdownModeChanged(e => { - if (e) { + this._register(cell.onCurrentEditModeChanged(editMode => { + if (editMode !== CellEditModes.WYSIWYG) { this._onFindStateChange(changeEvent).catch(onUnexpectedError); } })); diff --git a/src/sql/workbench/contrib/notebook/test/electron-browser/cell.test.ts b/src/sql/workbench/contrib/notebook/test/electron-browser/cell.test.ts index d8b77fad64..8ab3f89e20 100644 --- a/src/sql/workbench/contrib/notebook/test/electron-browser/cell.test.ts +++ b/src/sql/workbench/contrib/notebook/test/electron-browser/cell.test.ts @@ -13,7 +13,7 @@ import { CellTypes } from 'sql/workbench/services/notebook/common/contracts'; import { ModelFactory } from 'sql/workbench/services/notebook/browser/models/modelFactory'; import { NotebookModelStub, ClientSessionStub, KernelStub, FutureStub } from 'sql/workbench/contrib/notebook/test/stubs'; import { EmptyFuture } from 'sql/workbench/contrib/notebook/test/emptySessionClasses'; -import { ICellModel, ICellModelOptions, IClientSession, INotebookModel } from 'sql/workbench/services/notebook/browser/models/modelInterfaces'; +import { CellEditModes, ICellModel, ICellModelOptions, IClientSession, INotebookModel } from 'sql/workbench/services/notebook/browser/models/modelInterfaces'; import { Deferred } from 'sql/base/common/promise'; import { ServiceCollection } from 'vs/platform/instantiation/common/serviceCollection'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; @@ -1233,4 +1233,38 @@ suite('Cell Model', function (): void { cellModel.addAttachment('image/png', imageFilebase64Value, 'test.png'); assert.deepStrictEqual(cellModel.attachments, attachments, 'addAttachment should not add duplicate images'); }); + + test('cell should fire onCurrentEditModeChanged on edit', async function () { + + let notebookModel = new NotebookModelStub({ + name: '', + version: '', + mimetype: '' + }); + let contents: nb.ICellContents = { + cell_type: CellTypes.Markdown, + source: '', + metadata: {} + }; + + let cellModel = factory.createCell(contents, { notebook: notebookModel, isTrusted: false }); + + let editModeChangePromise = () => { + return new Promise((resolve, reject) => { + setTimeout(() => reject(), 2000); + cellModel.onCurrentEditModeChanged(editMode => { + resolve(editMode); + }); + }); + }; + + assert(!cellModel.isEditMode); + + let editModePromise = editModeChangePromise(); + cellModel.isEditMode = true; + + let editMode = await editModePromise; + assert(editMode); + assert.strictEqual(editMode, CellEditModes.WYSIWYG, 'Default edit mode should be WYSIWYG.'); + }); }); diff --git a/src/sql/workbench/services/notebook/browser/models/cell.ts b/src/sql/workbench/services/notebook/browser/models/cell.ts index dd61e16d67..346bb63067 100644 --- a/src/sql/workbench/services/notebook/browser/models/cell.ts +++ b/src/sql/workbench/services/notebook/browser/models/cell.ts @@ -73,8 +73,6 @@ export class CellModel extends Disposable implements ICellModel { private _isCollapsed: boolean; private _onCollapseStateChanged = new Emitter(); private _modelContentChangedEvent: IModelContentChangedEvent; - private _onCellPreviewChanged = new Emitter(); - private _onCellMarkdownChanged = new Emitter(); private _isCommandExecutionSettingEnabled: boolean = false; private _showPreview: boolean = true; private _showMarkdown: boolean = false; @@ -422,7 +420,6 @@ export class CellModel extends Disposable implements ICellModel { public set showPreview(val: boolean) { this._showPreview = val; - this._onCellPreviewChanged.fire(this._showPreview); this.doModeUpdates(); } @@ -432,7 +429,6 @@ export class CellModel extends Disposable implements ICellModel { public set showMarkdown(val: boolean) { this._showMarkdown = val; - this._onCellMarkdownChanged.fire(this._showMarkdown); this.doModeUpdates(); } @@ -454,14 +450,6 @@ export class CellModel extends Disposable implements ICellModel { this._cellSourceChanged = val; } - public get onCellPreviewModeChanged(): Event { - return this._onCellPreviewChanged.event; - } - - public get onCellMarkdownModeChanged(): Event { - return this._onCellMarkdownChanged.event; - } - public get onParameterStateChanged(): Event { return this._onParameterStateChanged.event; } diff --git a/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts b/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts index 979cd5d47d..48643f8af0 100644 --- a/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts +++ b/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts @@ -534,8 +534,6 @@ export interface ICellModel { showPreview: boolean; showMarkdown: boolean; defaultTextEditMode: string; - readonly onCellPreviewModeChanged: Event; - readonly onCellMarkdownModeChanged: Event; sendChangeToNotebook(change: NotebookChangeType): void; cellSourceChanged: boolean; readonly savedConnectionName: string | undefined;