From 80b733ebe0747445840750c9a91f1b256f7c5f14 Mon Sep 17 00:00:00 2001 From: Charles Gagnon Date: Fri, 9 Jun 2023 12:48:38 -0700 Subject: [PATCH] Cleanup Notebook actions (#23368) * Cleanup Notebook actions * Comments --- .../cellViews/cellToolbar.component.ts | 50 +++++++++---------- .../notebook/browser/notebook.component.ts | 20 ++++---- .../notebook/browser/notebookActions.ts | 31 +++++++++++- .../notebookViews/notebookViews.component.ts | 6 +-- .../test/browser/notebookActions.test.ts | 31 ++++++++++-- 5 files changed, 95 insertions(+), 43 deletions(-) diff --git a/src/sql/workbench/contrib/notebook/browser/cellViews/cellToolbar.component.ts b/src/sql/workbench/contrib/notebook/browser/cellViews/cellToolbar.component.ts index 76664f5d05..a1abf1d088 100644 --- a/src/sql/workbench/contrib/notebook/browser/cellViews/cellToolbar.component.ts +++ b/src/sql/workbench/contrib/notebook/browser/cellViews/cellToolbar.component.ts @@ -11,7 +11,7 @@ import { Taskbar, ITaskbarContent } from 'sql/base/browser/ui/taskbar/taskbar'; import { IContextMenuService } from 'vs/platform/contextview/browser/contextView'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; import { DeleteCellAction, EditCellAction, CellToggleMoreActions, MoveCellAction, SplitCellAction } from 'sql/workbench/contrib/notebook/browser/cellToolbarActions'; -import { AddCellAction } from 'sql/workbench/contrib/notebook/browser/notebookActions'; +import { AddCodeCellAction, AddTextCellAction, ToggleAddCellDropdownAction } from 'sql/workbench/contrib/notebook/browser/notebookActions'; import { CellTypes } from 'sql/workbench/services/notebook/common/contracts'; import { DropdownMenuActionViewItem } from 'sql/base/browser/ui/buttonMenu/buttonMenu'; import { ICellModel } from 'sql/workbench/services/notebook/browser/models/modelInterfaces'; @@ -79,29 +79,29 @@ export class CellToolbarComponent { private setupActions(): void { this._disposableActions.clear(); - let addCellsButton = this.instantiationService.createInstance(AddCellAction, 'notebook.AddCodeCell', localize('codeCellsPreview', "Add cell"), 'masked-pseudo code'); - this._disposableActions.add(addCellsButton); + let toggleAddCellDropdown = this.instantiationService.createInstance(ToggleAddCellDropdownAction); + this._disposableActions.add(toggleAddCellDropdown); - let addCodeCellButton = this.instantiationService.createInstance(AddCellAction, 'notebook.AddCodeCell', localize('codePreview', "Code cell"), 'masked-pseudo code'); - addCodeCellButton.cellType = CellTypes.Code; - this._disposableActions.add(addCodeCellButton); + let addCodeCellAction = this.instantiationService.createInstance(AddCodeCellAction); + addCodeCellAction.cellType = CellTypes.Code; + this._disposableActions.add(addCodeCellAction); - let addTextCellButton = this.instantiationService.createInstance(AddCellAction, 'notebook.AddTextCell', localize('textPreview', "Text cell"), 'masked-pseudo markdown'); - addTextCellButton.cellType = CellTypes.Markdown; - this._disposableActions.add(addTextCellButton); + let addTextCellAction = this.instantiationService.createInstance(AddTextCellAction); + addTextCellAction.cellType = CellTypes.Markdown; + this._disposableActions.add(addTextCellAction); - let moveCellDownButton = this.instantiationService.createInstance(MoveCellAction, 'notebook.MoveCellDown', 'masked-icon move-down', this.buttonMoveDown); - let moveCellUpButton = this.instantiationService.createInstance(MoveCellAction, 'notebook.MoveCellUp', 'masked-icon move-up', this.buttonMoveUp); - this._disposableActions.add(moveCellDownButton); - this._disposableActions.add(moveCellUpButton); + let moveCellDownAction = this.instantiationService.createInstance(MoveCellAction, 'notebook.MoveCellDown', 'masked-icon move-down', this.buttonMoveDown); + let moveCellUpAction = this.instantiationService.createInstance(MoveCellAction, 'notebook.MoveCellUp', 'masked-icon move-up', this.buttonMoveUp); + this._disposableActions.add(moveCellDownAction); + this._disposableActions.add(moveCellUpAction); - let splitCellButton = this.instantiationService.createInstance(SplitCellAction, 'notebook.SplitCellAtCursor', this.buttonSplitCell, 'masked-icon icon-split-cell'); - splitCellButton.setListener(this._cellContext); - splitCellButton.enabled = this.cellModel.cellType !== 'markdown'; - this._disposableActions.add(splitCellButton); + let splitCellAction = this.instantiationService.createInstance(SplitCellAction, 'notebook.SplitCellAtCursor', this.buttonSplitCell, 'masked-icon icon-split-cell'); + splitCellAction.setListener(this._cellContext); + splitCellAction.enabled = this.cellModel.cellType !== 'markdown'; + this._disposableActions.add(splitCellAction); - let deleteButton = this.instantiationService.createInstance(DeleteCellAction, 'notebook.DeleteCell', 'masked-icon delete', this.buttonDelete); - this._disposableActions.add(deleteButton); + let deleteAction = this.instantiationService.createInstance(DeleteCellAction, 'notebook.DeleteCell', 'masked-icon delete', this.buttonDelete); + this._disposableActions.add(deleteAction); let moreActionsContainer = DOM.$('li.action-item'); this._cellToggleMoreActions = this.instantiationService.createInstance(CellToggleMoreActions); @@ -114,8 +114,8 @@ export class CellToolbarComponent { let addCellDropdownContainer = DOM.$('li.action-item'); addCellDropdownContainer.setAttribute('role', 'presentation'); let dropdownMenuActionViewItem = new DropdownMenuActionViewItem( - addCellsButton, - [addCodeCellButton, addTextCellButton], + toggleAddCellDropdown, + [addCodeCellAction, addTextCellAction], this.contextMenuService, undefined, this._actionBar.actionRunner, @@ -135,10 +135,10 @@ export class CellToolbarComponent { } taskbarContent.push( { element: addCellDropdownContainer }, - { action: moveCellDownButton }, - { action: moveCellUpButton }, - { action: splitCellButton }, - { action: deleteButton }, + { action: moveCellDownAction }, + { action: moveCellUpAction }, + { action: splitCellAction }, + { action: deleteAction }, { element: moreActionsContainer }); this._actionBar.setContent(taskbarContent); diff --git a/src/sql/workbench/contrib/notebook/browser/notebook.component.ts b/src/sql/workbench/contrib/notebook/browser/notebook.component.ts index 1abe930d44..e14c822b86 100644 --- a/src/sql/workbench/contrib/notebook/browser/notebook.component.ts +++ b/src/sql/workbench/contrib/notebook/browser/notebook.component.ts @@ -27,7 +27,7 @@ import { INotebookService, INotebookParams, INotebookEditor, INotebookSection, I import { NotebookModel } from 'sql/workbench/services/notebook/browser/models/notebookModel'; import { Deferred } from 'sql/base/common/promise'; import { ITaskbarContent, Taskbar } from 'sql/base/browser/ui/taskbar/taskbar'; -import { AddCellAction, KernelsDropdown, AttachToDropdown, TrustedAction, RunAllCellsAction, ClearAllOutputsAction, CollapseCellsAction, RunParametersAction, NotebookViewsActionProvider } from 'sql/workbench/contrib/notebook/browser/notebookActions'; +import { KernelsDropdown, AttachToDropdown, TrustedAction, RunAllCellsAction, ClearAllOutputsAction, CollapseCellsAction, RunParametersAction, NotebookViewsActionProvider, AddCodeCellAction, AddTextCellAction, ToggleAddCellDropdownAction } from 'sql/workbench/contrib/notebook/browser/notebookActions'; import { DropdownMenuActionViewItem } from 'sql/base/browser/ui/buttonMenu/buttonMenu'; import { INotebookEditOperation } from 'sql/workbench/api/common/sqlExtHostTypes'; import { IConnectionDialogService } from 'sql/workbench/services/connection/common/connectionDialogService'; @@ -517,19 +517,19 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe let spacerElement = document.createElement('li'); spacerElement.style.marginLeft = 'auto'; - let addCellsButton = this.instantiationService.createInstance(AddCellAction, 'notebook.AddCodeCell', localize('codeCellsPreview', "Add cell"), 'masked-pseudo code'); + let toggleAddCellDropdownAction = this.instantiationService.createInstance(ToggleAddCellDropdownAction); - let addCodeCellButton = this.instantiationService.createInstance(AddCellAction, 'notebook.AddCodeCell', localize('codePreview', "Code cell"), 'masked-pseudo code'); - addCodeCellButton.cellType = CellTypes.Code; + let addCodeCellAction = this.instantiationService.createInstance(AddCodeCellAction); + addCodeCellAction.cellType = CellTypes.Code; - let addTextCellButton = this.instantiationService.createInstance(AddCellAction, 'notebook.AddTextCell', localize('textPreview', "Text cell"), 'masked-pseudo markdown'); - addTextCellButton.cellType = CellTypes.Markdown; + let addTextCellAction = this.instantiationService.createInstance(AddTextCellAction); + addTextCellAction.cellType = CellTypes.Markdown; this._runAllCellsAction = this.instantiationService.createInstance(RunAllCellsAction, 'notebook.runAllCells', localize('runAllPreview', "Run all"), 'masked-pseudo start-outline'); let collapseCellsAction = this.instantiationService.createInstance(CollapseCellsAction, 'notebook.collapseCells', true); - let clearResultsButton = this.instantiationService.createInstance(ClearAllOutputsAction, 'notebook.ClearAllOutputs', true); + let clearResultsAction = this.instantiationService.createInstance(ClearAllOutputsAction, 'notebook.ClearAllOutputs', true); this._trustedAction = this.instantiationService.createInstance(TrustedAction, 'notebook.Trusted', true); this._trustedAction.enabled = false; @@ -544,8 +544,8 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe let buttonDropdownContainer = DOM.$('li.action-item'); buttonDropdownContainer.setAttribute('role', 'presentation'); let dropdownMenuActionViewItem = new DropdownMenuActionViewItem( - addCellsButton, - [addCodeCellButton, addTextCellButton], + toggleAddCellDropdownAction, + [addCodeCellAction, addTextCellAction], this.contextMenuService, undefined, this._actionBar.actionRunner, @@ -589,7 +589,7 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe { element: spacerElement }, { element: viewsDropdownContainer }, { action: collapseCellsAction }, - { action: clearResultsButton }, + { action: clearResultsAction }, { action: this._trustedAction }, { action: runParametersAction }, ]; diff --git a/src/sql/workbench/contrib/notebook/browser/notebookActions.ts b/src/sql/workbench/contrib/notebook/browser/notebookActions.ts index fbce065e33..08a2bb2aa8 100644 --- a/src/sql/workbench/contrib/notebook/browser/notebookActions.ts +++ b/src/sql/workbench/contrib/notebook/browser/notebookActions.ts @@ -59,8 +59,13 @@ export const noParameterCell: string = localize('noParametersCell', "This notebo export const noParametersInCell: string = localize('noParametersInCell', "This notebook cannot run with parameters until there are parameters added to the parameter cell. [Learn more](https://docs.microsoft.com/sql/azure-data-studio/notebooks/notebooks-parameterization)."); export const untitledNotSupported: string = localize('untitledNotSupported', "Run with parameters is not supported for Untitled notebooks. Please save the notebook before continuing. [Learn more](https://docs.microsoft.com/sql/azure-data-studio/notebooks/notebooks-parameterization)."); -// Action to add a cell to notebook based on cell type(code/markdown). -export class AddCellAction extends Action { +export class ToggleAddCellDropdownAction extends Action { + constructor() { + super('notebook.toggleAddCell', localize('codeCellsPreview', "Add cell")) + } +} + +export abstract class AddCellAction extends Action { public cellType: CellType; constructor( @@ -95,6 +100,28 @@ export class AddCellAction extends Action { } } +/** + * Action to add a new Text cell to a Notebook + */ +export class AddTextCellAction extends AddCellAction { + public override cellType: CellType = 'markdown'; + + constructor(@INotebookService notebookService: INotebookService) { + super('notebook.AddTextCell', localize('textPreview', "Text cell"), 'masked-pseudo markdown', notebookService); + } +} + +/** + * Action to add a new Code cell to a Notebook + */ +export class AddCodeCellAction extends AddCellAction { + public override cellType: CellType = 'code'; + + constructor(@INotebookService notebookService: INotebookService) { + super('notebook.AddCodeCell', localize('codePreview', "Code cell"), 'masked-pseudo code', notebookService); + } +} + export interface ITooltipState { label: string; baseClass: string; diff --git a/src/sql/workbench/contrib/notebook/browser/notebookViews/notebookViews.component.ts b/src/sql/workbench/contrib/notebook/browser/notebookViews/notebookViews.component.ts index b7c78086db..be1e3b89eb 100644 --- a/src/sql/workbench/contrib/notebook/browser/notebookViews/notebookViews.component.ts +++ b/src/sql/workbench/contrib/notebook/browser/notebookViews/notebookViews.component.ts @@ -30,7 +30,7 @@ import { NotebookViewsGridComponent } from 'sql/workbench/contrib/notebook/brows import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; import { DeleteViewAction, InsertCellAction, ViewSettingsAction } from 'sql/workbench/contrib/notebook/browser/notebookViews/notebookViewsActions'; import { DropdownMenuActionViewItem } from 'sql/base/browser/ui/buttonMenu/buttonMenu'; -import { NotebookViewsActionProvider, AddCellAction, RunAllCellsAction } from 'sql/workbench/contrib/notebook/browser/notebookActions'; +import { NotebookViewsActionProvider, RunAllCellsAction } from 'sql/workbench/contrib/notebook/browser/notebookActions'; import * as DOM from 'vs/base/browser/dom'; import { AnchorAlignment } from 'vs/base/browser/ui/contextview/contextview'; import { LabeledMenuItemActionItem } from 'sql/platform/actions/browser/menuEntryActionViewItem'; @@ -267,11 +267,11 @@ export class NotebookViewComponent extends AngularDisposable implements INoteboo let viewsContainer = document.createElement('li'); let viewsActionsProvider = new NotebookViewsActionProvider(viewsContainer, this.views, this.modelReady, this._notebookService, this._notificationService, this._instantiationService); - let viewsButton = this._instantiationService.createInstance(AddCellAction, 'notebook.OpenViews', undefined, 'notebook-button masked-pseudo code'); + let viewsAction = new Action('notebook.OpenViews'); let viewsDropdownContainer = DOM.$('li.action-item'); viewsDropdownContainer.setAttribute('role', 'presentation'); let viewsDropdownMenuActionViewItem = new DropdownMenuActionViewItem( - viewsButton, + viewsAction, viewsActionsProvider, this._contextMenuService, undefined, diff --git a/src/sql/workbench/contrib/notebook/test/browser/notebookActions.test.ts b/src/sql/workbench/contrib/notebook/test/browser/notebookActions.test.ts index 2c3c570de2..9e7e0e3bcd 100644 --- a/src/sql/workbench/contrib/notebook/test/browser/notebookActions.test.ts +++ b/src/sql/workbench/contrib/notebook/test/browser/notebookActions.test.ts @@ -7,7 +7,7 @@ import * as assert from 'assert'; import * as azdata from 'azdata'; import * as sinon from 'sinon'; import { TestConfigurationService } from 'sql/platform/connection/test/common/testConfigurationService'; -import { AddCellAction, ClearAllOutputsAction, CollapseCellsAction, CreateNotebookViewAction, DashboardViewAction, kernelNotSupported, KernelsDropdown, msgChanging, noKernelName, noParameterCell, noParametersInCell, NotebookViewAction, NotebookViewsActionProvider, RunAllCellsAction, RunParametersAction, TrustedAction, untitledNotSupported } from 'sql/workbench/contrib/notebook/browser/notebookActions'; +import { AddCodeCellAction, AddTextCellAction, ClearAllOutputsAction, CollapseCellsAction, CreateNotebookViewAction, DashboardViewAction, kernelNotSupported, KernelsDropdown, msgChanging, noKernelName, noParameterCell, noParametersInCell, NotebookViewAction, NotebookViewsActionProvider, RunAllCellsAction, RunParametersAction, TrustedAction, untitledNotSupported } from 'sql/workbench/contrib/notebook/browser/notebookActions'; import { ClientSessionStub, ContextViewProviderStub, NotebookComponentStub, NotebookModelStub, NotebookServiceStub, NotebookViewsStub, NotebookViewStub } from 'sql/workbench/contrib/notebook/test/stubs'; import { NotebookEditorStub } from 'sql/workbench/contrib/notebook/test/testCommon'; import { ICellModel, INotebookModel, ViewMode } from 'sql/workbench/services/notebook/browser/models/modelInterfaces'; @@ -126,11 +126,36 @@ suite('Notebook Actions', function (): void { mockNotebookEditor.reset(); }); - test('Add Cell Action', async function (): Promise { + test('Add Text Cell Action', async function (): Promise { + let testCellType: CellType = 'markdown' + let actualCellType: CellType; + + let action = new AddTextCellAction(mockNotebookService.object); + + mockNotebookEditor.setup(x => x.model).returns(() => testNotebookModel); + + // Normal use case + mockNotebookEditor.setup(x => x.addCell(TypeMoq.It.isAny(), TypeMoq.It.isAnyNumber())).returns((cellType, index) => { actualCellType = cellType; }); + let mockNotebookComponent = TypeMoq.Mock.ofType(NotebookComponentStub); + mockNotebookComponent.setup(c => c.addCell(TypeMoq.It.isAny(), TypeMoq.It.isAnyNumber())).returns(cellType => { + actualCellType = cellType; + }); + + assert.doesNotThrow(() => action.run(testUri)); + assert.strictEqual(actualCellType, testCellType); + + // Handle error case + mockNotebookEditor.reset(); + mockNotebookEditor.setup(x => x.model).returns(() => testNotebookModel); + mockNotebookEditor.setup(x => x.addCell(TypeMoq.It.isAny(), TypeMoq.It.isAnyNumber())).throws(new Error('Test Error')); + await assert.rejects(action.run(URI.parse('untitled'))); + }); + + test('Add Code Cell Action', async function (): Promise { let testCellType: CellType = 'code'; let actualCellType: CellType; - let action = new AddCellAction('TestId', 'TestLabel', 'TestClass', mockNotebookService.object); + let action = new AddCodeCellAction(mockNotebookService.object); action.cellType = testCellType; mockNotebookEditor.setup(x => x.model).returns(() => testNotebookModel);