diff --git a/extension/package.json b/extension/package.json index ed5cf923fc..56caed978c 100644 --- a/extension/package.json +++ b/extension/package.json @@ -425,7 +425,7 @@ }, { "title": "Add Plot", - "command": "dvc.addTopLevelPlot", + "command": "dvc.addPlot", "category": "DVC", "icon": "$(add)" }, @@ -601,11 +601,6 @@ "category": "DVC", "icon": "$(refresh)" }, - { - "title": "Add Custom Plot", - "command": "dvc.views.plots.addCustomPlot", - "category": "DVC" - }, { "title": "Remove Custom Plot(s)", "command": "dvc.views.plots.removeCustomPlots", @@ -910,7 +905,7 @@ "when": "dvc.commands.available && dvc.project.available" }, { - "command": "dvc.addTopLevelPlot", + "command": "dvc.addPlot", "when": "dvc.commands.available && dvc.project.available" }, { @@ -1029,10 +1024,6 @@ "command": "dvc.views.plots.refreshPlots", "when": "dvc.commands.available && dvc.project.available" }, - { - "command": "dvc.views.plots.addCustomPlot", - "when": "dvc.commands.available && dvc.project.available" - }, { "command": "dvc.views.plots.removeCustomPlots", "when": "dvc.commands.available && dvc.project.available" @@ -1425,7 +1416,7 @@ "group": "navigation@3" }, { - "command": "dvc.addTopLevelPlot", + "command": "dvc.addPlot", "when": "view == dvc.views.plotsPathsTree", "group": "navigation@0" }, diff --git a/extension/src/commands/external.ts b/extension/src/commands/external.ts index ebd61eff8e..e4e6eeb304 100644 --- a/extension/src/commands/external.ts +++ b/extension/src/commands/external.ts @@ -72,15 +72,15 @@ export enum RegisteredCommands { STOP_EXPERIMENTS = 'dvc.stopAllRunningExperiments', PIPELINE_SHOW_DAG = 'dvc.showPipelineDAG', - PIPELINE_ADD_PLOT = 'dvc.addTopLevelPlot', PLOTS_PATH_TOGGLE = 'dvc.views.plotsPathsTree.toggleStatus', PLOTS_SHOW = 'dvc.showPlots', PLOTS_SELECT = 'dvc.views.plotsPathsTree.selectPlots', PLOTS_REFRESH = 'dvc.views.plots.refreshPlots', - PLOTS_CUSTOM_ADD = 'dvc.views.plots.addCustomPlot', PLOTS_CUSTOM_REMOVE = 'dvc.views.plots.removeCustomPlots', + ADD_PLOT = 'dvc.addPlot', + EXPERIMENT_AND_PLOTS_SHOW = 'dvc.showExperimentsAndPlots', EXTENSION_CHECK_CLI_COMPATIBLE = 'dvc.checkCLICompatible', diff --git a/extension/src/extension.ts b/extension/src/extension.ts index b3cdef2a14..24e571619b 100644 --- a/extension/src/extension.ts +++ b/extension/src/extension.ts @@ -192,7 +192,12 @@ class Extension extends Disposable { this.setup ) registerPipelineCommands(this.pipelines, this.internalCommands) - registerPlotsCommands(this.plots, this.internalCommands, this.setup) + registerPlotsCommands( + this.plots, + this.internalCommands, + this.setup, + this.pipelines + ) registerSetupCommands(this.setup, this.internalCommands) this.internalCommands.registerExternalCommand( RegisteredCommands.EXPERIMENT_AND_PLOTS_SHOW, diff --git a/extension/src/pipeline/index.ts b/extension/src/pipeline/index.ts index 4ef2d5efee..d69f0944f8 100644 --- a/extension/src/pipeline/index.ts +++ b/extension/src/pipeline/index.ts @@ -112,7 +112,7 @@ export class Pipeline extends DeferredDisposable { return this.addPipeline() } - public async addTopLevelPlot() { + public async addDataSeriesPlot() { const cwd = await this.getCwd() if (!cwd) { diff --git a/extension/src/pipeline/register.ts b/extension/src/pipeline/register.ts index 468db70301..75b9580e28 100644 --- a/extension/src/pipeline/register.ts +++ b/extension/src/pipeline/register.ts @@ -1,7 +1,6 @@ import { WorkspacePipeline } from './workspace' import { RegisteredCommands } from '../commands/external' import { InternalCommands } from '../commands/internal' -import { Context, getDvcRootFromContext } from '../vscode/context' export const registerPipelineCommands = ( pipelines: WorkspacePipeline, @@ -11,10 +10,4 @@ export const registerPipelineCommands = ( RegisteredCommands.PIPELINE_SHOW_DAG, () => pipelines.showDag() ) - - internalCommands.registerExternalCommand( - RegisteredCommands.PIPELINE_ADD_PLOT, - (context: Context) => - pipelines.addTopLevelPlot(getDvcRootFromContext(context)) - ) } diff --git a/extension/src/pipeline/workspace.ts b/extension/src/pipeline/workspace.ts index 6829583d68..d940075872 100644 --- a/extension/src/pipeline/workspace.ts +++ b/extension/src/pipeline/workspace.ts @@ -63,14 +63,14 @@ export class WorkspacePipeline extends BaseWorkspace { ) } - public async addTopLevelPlot(overrideRoot?: string) { + public async addDataSeriesPlot(overrideRoot?: string) { const cwd = overrideRoot || (await this.getCwd()) if (!cwd) { return } - void this.getRepository(cwd).addTopLevelPlot() + void this.getRepository(cwd).addDataSeriesPlot() } private getCwd() { diff --git a/extension/src/plots/commands/register.ts b/extension/src/plots/commands/register.ts index beed77877e..f271cb7de0 100644 --- a/extension/src/plots/commands/register.ts +++ b/extension/src/plots/commands/register.ts @@ -1,15 +1,22 @@ +import { commands } from 'vscode' import { RegisteredCommands } from '../../commands/external' import { InternalCommands } from '../../commands/internal' import { showSetupOrExecuteCommand } from '../../commands/util' import { Setup } from '../../setup' import { Context, getDvcRootFromContext } from '../../vscode/context' import { WorkspacePlots } from '../workspace' +import { WorkspacePipeline } from '../../pipeline/workspace' export const registerPlotsCommands = ( plots: WorkspacePlots, internalCommands: InternalCommands, - setup: Setup + setup: Setup, + pipelines: WorkspacePipeline ) => { + commands.registerCommand(RegisteredCommands.ADD_PLOT, (context: Context) => + plots.addPlot(pipelines, getDvcRootFromContext(context)) + ) + internalCommands.registerExternalCommand( RegisteredCommands.PLOTS_SHOW, showSetupOrExecuteCommand(setup, context => @@ -27,11 +34,6 @@ export const registerPlotsCommands = ( (context: Context) => plots.refresh(getDvcRootFromContext(context)) ) - internalCommands.registerExternalCommand( - RegisteredCommands.PLOTS_CUSTOM_ADD, - (context: Context) => plots.addCustomPlot(getDvcRootFromContext(context)) - ) - internalCommands.registerExternalCommand( RegisteredCommands.PLOTS_CUSTOM_REMOVE, (context: Context) => diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index 41c31ff5a6..ae7ac0ddc7 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -223,7 +223,8 @@ export class Plots extends BaseRepository { errors, experiments, () => this.getWebview(), - () => this.selectPlots() + () => this.selectPlots(), + () => this.addCustomPlot() ) this.dispose.track( this.onDidReceivedWebviewMessage(message => diff --git a/extension/src/plots/quickPick.test.ts b/extension/src/plots/quickPick.test.ts new file mode 100644 index 0000000000..7e22b95dee --- /dev/null +++ b/extension/src/plots/quickPick.test.ts @@ -0,0 +1,39 @@ +import { pickPlotType } from './quickPick' +import { quickPickValue } from '../vscode/quickPick' +import { Title } from '../vscode/title' + +jest.mock('../vscode/quickPick') + +const mockedQuickPickValue = jest.mocked(quickPickValue) + +beforeEach(() => { + jest.resetAllMocks() +}) + +describe('pickPlotType', () => { + it('should call a quick pick with possible plot types', async () => { + mockedQuickPickValue.mockResolvedValueOnce(undefined) + + await pickPlotType() + + expect(mockedQuickPickValue).toHaveBeenCalledWith( + [ + { + description: + 'Create a data series plot definition by selecting data from a file', + label: 'Data Series', + value: 'data-series' + }, + { + description: + 'Create an extension-only plot by selecting one metric and one param from the experiments table', + label: 'Custom', + value: 'custom' + } + ], + { + title: Title.SELECT_PLOT_TYPE + } + ) + }) +}) diff --git a/extension/src/plots/quickPick.ts b/extension/src/plots/quickPick.ts new file mode 100644 index 0000000000..5c00508247 --- /dev/null +++ b/extension/src/plots/quickPick.ts @@ -0,0 +1,28 @@ +import { quickPickValue } from '../vscode/quickPick' +import { Title } from '../vscode/title' + +export const enum PLOT_TYPE { + CUSTOM = 'custom', + DATA_SERIES = 'data-series' +} + +export const pickPlotType = () => + quickPickValue( + [ + { + description: + 'Create a data series plot definition by selecting data from a file', + label: 'Data Series', + value: PLOT_TYPE.DATA_SERIES + }, + { + description: + 'Create an extension-only plot by selecting one metric and one param from the experiments table', + label: 'Custom', + value: PLOT_TYPE.CUSTOM + } + ], + { + title: Title.SELECT_PLOT_TYPE + } + ) diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index d4b14e99a2..dddc510cc2 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -41,6 +41,7 @@ export class WebviewMessages { private readonly getWebview: () => BaseWebview | undefined private readonly selectPlots: () => Promise + private readonly addCustomPlot: () => Promise constructor( dvcRoot: string, @@ -49,7 +50,8 @@ export class WebviewMessages { errors: ErrorsModel, experiments: Experiments, getWebview: () => BaseWebview | undefined, - selectPlots: () => Promise + selectPlots: () => Promise, + addCustomPlot: () => Promise ) { this.dvcRoot = dvcRoot this.paths = paths @@ -58,6 +60,7 @@ export class WebviewMessages { this.experiments = experiments this.getWebview = getWebview this.selectPlots = selectPlots + this.addCustomPlot = addCustomPlot } public async sendWebviewMessage() { @@ -71,16 +74,13 @@ export class WebviewMessages { public handleMessageFromWebview(message: MessageFromWebview) { switch (message.type) { - case MessageFromWebviewType.ADD_CUSTOM_PLOT: - return commands.executeCommand( - RegisteredCommands.PLOTS_CUSTOM_ADD, - this.dvcRoot - ) - case MessageFromWebviewType.ADD_PIPELINE_PLOT: + case MessageFromWebviewType.ADD_PLOT: return commands.executeCommand( - RegisteredCommands.PIPELINE_ADD_PLOT, + RegisteredCommands.ADD_PLOT, this.dvcRoot ) + case MessageFromWebviewType.ADD_CUSTOM_PLOT: + return this.addCustomPlotFromWebview() case MessageFromWebviewType.EXPORT_PLOT_DATA_AS_CSV: return this.exportPlotDataAsCsv(message.payload) case MessageFromWebviewType.EXPORT_PLOT_DATA_AS_TSV: @@ -309,6 +309,11 @@ export class WebviewMessages { ) } + private addCustomPlotFromWebview() { + void this.addCustomPlot() + sendTelemetryEvent(EventName.VIEWS_PLOTS_CUSTOM_ADD, undefined, undefined) + } + private setExperimentStatus(id: string) { this.experiments.toggleExperimentStatus(id) sendTelemetryEvent( diff --git a/extension/src/plots/workspace.ts b/extension/src/plots/workspace.ts index 296b10cbd9..8145db9cd4 100644 --- a/extension/src/plots/workspace.ts +++ b/extension/src/plots/workspace.ts @@ -1,9 +1,14 @@ import { EventEmitter } from 'vscode' import { Plots } from '.' import { PlotsData } from './webview/contract' +import { PLOT_TYPE, pickPlotType } from './quickPick' import { ResourceLocator } from '../resourceLocator' import { BaseWorkspaceWebviews } from '../webview/workspace' import { WorkspaceExperiments } from '../experiments/workspace' +import { WorkspacePipeline } from '../pipeline/workspace' +import { sendTelemetryEvent } from '../telemetry' +import { RegisteredCommands } from '../commands/external' +import { StopWatch } from '../util/time' export class WorkspacePlots extends BaseWorkspaceWebviews { public readonly pathsChanged = this.dispose.track(new EventEmitter()) @@ -74,6 +79,25 @@ export class WorkspacePlots extends BaseWorkspaceWebviews { return this.getRepository(dvcRoot).removeCustomPlot() } + public async addPlot(pipelines: WorkspacePipeline, overrideRoot?: string) { + const stopWatch = new StopWatch() + const response: PLOT_TYPE | undefined = await pickPlotType() + + if (response === PLOT_TYPE.CUSTOM) { + await this.addCustomPlot(overrideRoot) + } + + if (response === PLOT_TYPE.DATA_SERIES) { + await pipelines.addDataSeriesPlot(overrideRoot) + } + + sendTelemetryEvent( + RegisteredCommands.ADD_PLOT, + { type: response }, + { duration: stopWatch.getElapsedTime() } + ) + } + public getFocusedOrOnlyOrPickProject() { return this.focusedWebviewDvcRoot || this.getOnlyOrPickProject() } diff --git a/extension/src/telemetry/constants.ts b/extension/src/telemetry/constants.ts index ff8a1c9a5f..ff5bcff127 100644 --- a/extension/src/telemetry/constants.ts +++ b/extension/src/telemetry/constants.ts @@ -86,6 +86,7 @@ export const EventName = Object.assign( VIEWS_PLOTS_COMPARISON_ROWS_REORDERED: 'views.plots.comparisonRowsReordered', VIEWS_PLOTS_CREATED: 'views.plots.created', + VIEWS_PLOTS_CUSTOM_ADD: 'views.plots.addCustomPlot', VIEWS_PLOTS_EXPERIMENT_TOGGLE: 'views.plots.toggleExperimentStatus', VIEWS_PLOTS_EXPORT_PLOT_DATA_AS_CSV: 'views.plots.exportPlotDataAsCsv', VIEWS_PLOTS_EXPORT_PLOT_DATA_AS_JSON: 'views.plots.exportPlotDataAsJson', @@ -195,15 +196,15 @@ export interface IEventNamePropertyMapping { [EventName.STOP_EXPERIMENTS]: { stopped: boolean; wasRunning: boolean } [EventName.PIPELINE_SHOW_DAG]: undefined - [EventName.PIPELINE_ADD_PLOT]: undefined [EventName.PLOTS_PATH_TOGGLE]: undefined [EventName.PLOTS_SHOW]: undefined [EventName.PLOTS_SELECT]: undefined [EventName.PLOTS_REFRESH]: undefined - [EventName.PLOTS_CUSTOM_ADD]: undefined [EventName.PLOTS_CUSTOM_REMOVE]: undefined + [EventName.ADD_PLOT]: { type: string | undefined } + [EventName.ADD_TARGET]: undefined [EventName.CHECKOUT_TARGET]: undefined [EventName.CHECKOUT]: undefined @@ -287,6 +288,7 @@ export interface IEventNamePropertyMapping { [EventName.VIEWS_PLOTS_CLOSED]: undefined [EventName.VIEWS_PLOTS_CREATED]: undefined + [EventName.VIEWS_PLOTS_CUSTOM_ADD]: undefined [EventName.VIEWS_PLOTS_FOCUS_CHANGED]: WebviewFocusChangedProperties [EventName.VIEWS_PLOTS_REVISIONS_REORDERED]: undefined [EventName.VIEWS_PLOTS_COMPARISON_ROWS_REORDERED]: undefined diff --git a/extension/src/test/e2e/util.ts b/extension/src/test/e2e/util.ts index dd67c309b4..ed37beaf82 100644 --- a/extension/src/test/e2e/util.ts +++ b/extension/src/test/e2e/util.ts @@ -185,7 +185,9 @@ export const closeAllEditors = async (): Promise => { export const createCustomPlot = async (): Promise => { const workbench = await browser.getWorkbench() - const addCustomPlot = await workbench.executeCommand('DVC: Add Custom Plot') + const addCustomPlot = await workbench.executeCommand('DVC: Add Plot') + await browser.waitUntil(() => addCustomPlot.elem.isDisplayed()) + await browser.keys(['ArrowDown', 'Enter']) await browser.waitUntil(() => addCustomPlot.elem.isDisplayed()) await browser.keys('Enter') await browser.waitUntil(() => addCustomPlot.elem.isDisplayed()) diff --git a/extension/src/test/suite/pipeline/index.test.ts b/extension/src/test/suite/pipeline/index.test.ts index d96f6ca341..bde79ed3ff 100644 --- a/extension/src/test/suite/pipeline/index.test.ts +++ b/extension/src/test/suite/pipeline/index.test.ts @@ -289,7 +289,7 @@ suite('Pipeline Test Suite', () => { }) }) - it('should add a top-level plot', async () => { + it('should add a data series plot', async () => { const { pipeline } = buildPipeline({ disposer: disposable, dvcRoot: dvcDemoPath @@ -304,7 +304,7 @@ suite('Pipeline Test Suite', () => { mockPickPlotConfiguration.onFirstCall().resolves(undefined) - await pipeline.addTopLevelPlot() + await pipeline.addDataSeriesPlot() expect(mockPickPlotConfiguration).to.be.calledOnce expect(mockAddPlotToDvcFile).not.to.be.called @@ -316,7 +316,7 @@ suite('Pipeline Test Suite', () => { y: 'acc' }) - await pipeline.addTopLevelPlot() + await pipeline.addDataSeriesPlot() expect(mockPickPlotConfiguration).to.be.calledTwice expect(mockAddPlotToDvcFile).to.be.called diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index c2f5a6810e..a8e8f11fdf 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -1107,20 +1107,23 @@ suite('Plots Test Suite', () => { }).timeout(WEBVIEW_TEST_TIMEOUT) it('should handle an add custom plot message from the webview', async () => { - const { mockMessageReceived } = await buildPlotsWebview({ + const { mockMessageReceived, plots } = await buildPlotsWebview({ disposer: disposable, plotsDiff: plotsDiffFixture }) - const executeCommandSpy = spy(commands, 'executeCommand') + const mockAddCustomPlot = stub(plots, 'addCustomPlot') + const mockSendTelemetryEvent = stub(Telemetry, 'sendTelemetryEvent') mockMessageReceived.fire({ type: MessageFromWebviewType.ADD_CUSTOM_PLOT }) - expect(executeCommandSpy).to.be.calledWithExactly( - RegisteredCommands.PLOTS_CUSTOM_ADD, - dvcDemoPath + expect(mockAddCustomPlot).to.be.calledOnce + expect(mockSendTelemetryEvent).to.be.calledWithExactly( + EventName.VIEWS_PLOTS_CUSTOM_ADD, + undefined, + undefined ) }) @@ -1209,7 +1212,7 @@ suite('Plots Test Suite', () => { ) }) - it('should handle an add pipeline plot message from the webview', async () => { + it('should handle an add plot message from the webview', async () => { const { mockMessageReceived } = await buildPlotsWebview({ disposer: disposable, plotsDiff: plotsDiffFixture @@ -1218,11 +1221,11 @@ suite('Plots Test Suite', () => { const mockExecuteCommand = stub(commands, 'executeCommand') mockMessageReceived.fire({ - type: MessageFromWebviewType.ADD_PIPELINE_PLOT + type: MessageFromWebviewType.ADD_PLOT }) expect(mockExecuteCommand).to.be.calledWithExactly( - RegisteredCommands.PIPELINE_ADD_PLOT, + RegisteredCommands.ADD_PLOT, dvcDemoPath ) }) diff --git a/extension/src/test/suite/plots/workspace.test.ts b/extension/src/test/suite/plots/workspace.test.ts index b2219923db..f9ff5ab148 100644 --- a/extension/src/test/suite/plots/workspace.test.ts +++ b/extension/src/test/suite/plots/workspace.test.ts @@ -9,6 +9,8 @@ import { closeAllEditors } from '../util' import * as customPlotQuickPickUtil from '../../../plots/model/quickPick' import { customPlotsOrderFixture } from '../../fixtures/expShow/base/customPlots' import { RegisteredCommands } from '../../../commands/external' +import * as quickPickUtil from '../../../plots/quickPick' +import { WorkspacePipeline } from '../../../pipeline/workspace' suite('Workspace Plots Test Suite', () => { const disposable = Disposable.fn() @@ -23,13 +25,32 @@ suite('Workspace Plots Test Suite', () => { return closeAllEditors() }) - describe('dvc.views.plots.addCustomPlot', () => { + describe('dvc.addPlot', () => { + it('should be able to add a data series plot', async () => { + const mockPickPlotType = stub(quickPickUtil, 'pickPlotType') + const mockAddDataSeriesPlot = stub( + WorkspacePipeline.prototype, + 'addDataSeriesPlot' + ) + + mockPickPlotType + .onFirstCall() + .resolves(quickPickUtil.PLOT_TYPE.DATA_SERIES) + mockAddDataSeriesPlot.onFirstCall().resolves(undefined) + + await commands.executeCommand(RegisteredCommands.ADD_PLOT) + + expect(mockAddDataSeriesPlot).to.be.calledOnce + }) + it('should be able to add a custom plot', async () => { const { plotsModel } = await buildPlots({ disposer: disposable, plotsDiff: plotsDiffFixture }) + const mockPickPlotType = stub(quickPickUtil, 'pickPlotType') + const mockGetMetricAndParam = stub( customPlotQuickPickUtil, 'pickMetricAndParam' @@ -40,12 +61,13 @@ suite('Workspace Plots Test Suite', () => { param: 'params.yaml:log_file' } + mockPickPlotType.onFirstCall().resolves(quickPickUtil.PLOT_TYPE.CUSTOM) mockGetMetricAndParam.onFirstCall().resolves(mockMetricVsParamOrderValue) const mockSetCustomPlotsOrder = stub(plotsModel, 'setCustomPlotsOrder') mockSetCustomPlotsOrder.returns(undefined) - await commands.executeCommand(RegisteredCommands.PLOTS_CUSTOM_ADD) + await commands.executeCommand(RegisteredCommands.ADD_PLOT) expect(mockSetCustomPlotsOrder).to.be.calledWith([ ...customPlotsOrderFixture, @@ -53,7 +75,26 @@ suite('Workspace Plots Test Suite', () => { ]) }) - it('should not add a custom plot if user cancels', async () => { + it('should not add a plot if users cancels', async () => { + const mockPickPlotType = stub(quickPickUtil, 'pickPlotType') + const mockAddDataSeriesPlot = stub( + WorkspacePipeline.prototype, + 'addDataSeriesPlot' + ) + const mockGetMetricAndParam = stub( + customPlotQuickPickUtil, + 'pickMetricAndParam' + ) + + mockPickPlotType.onFirstCall().resolves(undefined) + + await commands.executeCommand(RegisteredCommands.ADD_PLOT) + + expect(mockAddDataSeriesPlot).not.to.be.called + expect(mockGetMetricAndParam).not.to.be.called + }) + + it('should not add a custom plot if user cancels during custom plot creation', async () => { const { plotsModel } = await buildPlots({ disposer: disposable, plotsDiff: plotsDiffFixture @@ -62,13 +103,15 @@ suite('Workspace Plots Test Suite', () => { customPlotQuickPickUtil, 'pickMetricAndParam' ) + const mockPickPlotType = stub(quickPickUtil, 'pickPlotType') + mockPickPlotType.onFirstCall().resolves(quickPickUtil.PLOT_TYPE.CUSTOM) mockGetMetricAndParam.onFirstCall().resolves(undefined) const mockSetCustomPlotsOrder = stub(plotsModel, 'setCustomPlotsOrder') mockSetCustomPlotsOrder.returns(undefined) - await commands.executeCommand(RegisteredCommands.PLOTS_CUSTOM_ADD) + await commands.executeCommand(RegisteredCommands.ADD_PLOT) expect(mockSetCustomPlotsOrder).to.not.be.called }) diff --git a/extension/src/vscode/title.ts b/extension/src/vscode/title.ts index b015c66e5c..d9fbbf486b 100644 --- a/extension/src/vscode/title.ts +++ b/extension/src/vscode/title.ts @@ -41,6 +41,7 @@ export enum Title { SELECT_SORTS_TO_REMOVE = 'Select Sort(s) to Remove', SELECT_TRAINING_SCRIPT = 'Select your Training Script', SELECT_PLOT_DATA = 'Select Data to Plot', + SELECT_PLOT_TYPE = 'Select Plot Type', SETUP_WORKSPACE = 'Setup the Workspace', SET_EXPERIMENTS_HEADER_HEIGHT = 'Set the Maximum Experiment Table Header Height', SET_REMOTE_AS_DEFAULT = 'Set Default Remote', diff --git a/extension/src/webview/contract.ts b/extension/src/webview/contract.ts index ed3708d586..5138717d3b 100644 --- a/extension/src/webview/contract.ts +++ b/extension/src/webview/contract.ts @@ -16,8 +16,8 @@ export enum MessageFromWebviewType { ADD_CONFIGURATION = 'add-configuration', APPLY_EXPERIMENT_TO_WORKSPACE = 'apply-experiment-to-workspace', ADD_STARRED_EXPERIMENT_FILTER = 'add-starred-experiment-filter', + ADD_PLOT = 'add-plot', ADD_CUSTOM_PLOT = 'add-custom-plot', - ADD_PIPELINE_PLOT = 'add-pipeline-plot', CREATE_BRANCH_FROM_EXPERIMENT = 'create-branch-from-experiment', COPY_TO_CLIPBOARD = 'copy-to-clipboard', COPY_STUDIO_LINK = 'copy-studio-link', @@ -112,7 +112,7 @@ export type MessageFromWebview = type: MessageFromWebviewType.ADD_CUSTOM_PLOT } | { - type: MessageFromWebviewType.ADD_PIPELINE_PLOT + type: MessageFromWebviewType.ADD_PLOT } | { type: MessageFromWebviewType.COPY_TO_CLIPBOARD diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index d1a08094e0..194d9b9475 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -336,7 +336,7 @@ describe('App', () => { expect(loading).toHaveLength(3) }) - it('should render only get started (buttons: select plots, add experiments, add custom plots) when there are some selected exps, all unselected plots, and no custom plots', async () => { + it('should render only get started (buttons: select plots, add experiments, add plot) when there are some selected exps, all unselected plots, and no custom plots', async () => { renderAppWithOptionalData({ hasPlots: true, hasUnselectedPlots: true, @@ -344,11 +344,11 @@ describe('App', () => { }) const addExperimentsButton = await screen.findByText('Add Experiments') const selectPlotsButton = await screen.findByText('Select Plots') - const addCustomPlotsButton = await screen.findByText('Add Custom Plot') + const addPlotsButton = await screen.findByText('Add Plot') expect(addExperimentsButton).toBeInTheDocument() expect(selectPlotsButton).toBeInTheDocument() - expect(addCustomPlotsButton).toBeInTheDocument() + expect(addPlotsButton).toBeInTheDocument() expect(screen.queryByTestId('section-container')).not.toBeInTheDocument() mockPostMessage.mockReset() @@ -368,10 +368,10 @@ describe('App', () => { }) mockPostMessage.mockReset() - fireEvent.click(addCustomPlotsButton) + fireEvent.click(addPlotsButton) expect(mockPostMessage).toHaveBeenCalledWith({ - type: MessageFromWebviewType.ADD_CUSTOM_PLOT + type: MessageFromWebviewType.ADD_PLOT }) mockPostMessage.mockReset() }) @@ -391,7 +391,7 @@ describe('App', () => { const refreshButton = await screen.findByText('Refresh') expect(addExperimentsButton).toBeInTheDocument() - expect(screen.queryByText('Add Plots')).not.toBeInTheDocument() + expect(screen.queryByText('Add Plot')).not.toBeInTheDocument() expect(addCustomPlotsButton).toBeInTheDocument() expect(errorIcon).toBeInTheDocument() expect(screen.queryByTestId('section-container')).not.toBeInTheDocument() @@ -427,7 +427,7 @@ describe('App', () => { mockPostMessage.mockReset() }) - it('should render get started (buttons: select plots, add experiments) and custom section when there are some selected exps, all unselected plots, and added custom plots', async () => { + it('should render get started (buttons: select plots, add experiments, add plot) and custom section when there are some selected exps, all unselected plots, and added custom plots', async () => { renderAppWithOptionalData({ custom: customPlotsFixture, hasPlots: true, @@ -436,12 +436,12 @@ describe('App', () => { }) const addExperimentsButton = await screen.findByText('Add Experiments') const selectPlotsButton = await screen.findByText('Select Plots') - const addCustomPlotsButton = screen.queryByText('Add Custom Plot') + const addPlotButton = await screen.findByText('Add Plot') const customSection = await screen.findByTestId('section-container') expect(addExperimentsButton).toBeInTheDocument() expect(selectPlotsButton).toBeInTheDocument() - expect(addCustomPlotsButton).not.toBeInTheDocument() + expect(addPlotButton).toBeInTheDocument() expect(customSection).toBeInTheDocument() mockPostMessage.mockReset() @@ -460,9 +460,15 @@ describe('App', () => { type: MessageFromWebviewType.SELECT_PLOTS }) mockPostMessage.mockReset() + + fireEvent.click(addPlotButton) + + expect(mockPostMessage).toHaveBeenCalledWith({ + type: MessageFromWebviewType.ADD_PLOT + }) }) - it('should render only get started (buttons: add experiments, add custom plots, add plots) when there are no selected exps and no custom plots', async () => { + it('should render only get started (buttons: add experiments, add plot) when there are no selected exps and no custom plots', async () => { renderAppWithOptionalData({ custom: null, hasPlots: true, @@ -470,13 +476,11 @@ describe('App', () => { selectedRevisions: undefined }) const addExperimentsButton = await screen.findByText('Add Experiments') - const addPlotsButton = await screen.findByText('Add Plot') - const addCustomPlotsButton = await screen.findByText('Add Custom Plot') + const addPlotButton = await screen.findByText('Add Plot') const customSection = screen.queryByTestId('section-container') expect(addExperimentsButton).toBeInTheDocument() - expect(addCustomPlotsButton).toBeInTheDocument() - expect(addPlotsButton).toBeInTheDocument() + expect(addPlotButton).toBeInTheDocument() expect(customSection).not.toBeInTheDocument() mockPostMessage.mockReset() @@ -486,18 +490,12 @@ describe('App', () => { type: MessageFromWebviewType.SELECT_EXPERIMENTS }) - fireEvent.click(addCustomPlotsButton) + fireEvent.click(addPlotButton) expect(mockPostMessage).toHaveBeenCalledWith({ - type: MessageFromWebviewType.ADD_CUSTOM_PLOT + type: MessageFromWebviewType.ADD_PLOT }) mockPostMessage.mockReset() - - fireEvent.click(addPlotsButton) - - expect(mockPostMessage).toHaveBeenCalledWith({ - type: MessageFromWebviewType.ADD_PIPELINE_PLOT - }) }) it('should render get started (buttons: add experiments, add plots) and custom section when there are no selected exps and added custom plots', async () => { @@ -509,11 +507,9 @@ describe('App', () => { }) const addExperimentsButton = await screen.findByText('Add Experiments') const addPlotsButton = await screen.findByText('Add Plot') - const addCustomPlotsButton = screen.queryByText('Add Custom Plot') const customSection = await screen.findByTestId('section-container') expect(addExperimentsButton).toBeInTheDocument() - expect(addCustomPlotsButton).not.toBeInTheDocument() expect(addPlotsButton).toBeInTheDocument() expect(customSection).toBeInTheDocument() @@ -528,7 +524,7 @@ describe('App', () => { fireEvent.click(addPlotsButton) expect(mockPostMessage).toHaveBeenCalledWith({ - type: MessageFromWebviewType.ADD_PIPELINE_PLOT + type: MessageFromWebviewType.ADD_PLOT }) }) @@ -2364,7 +2360,12 @@ describe('App', () => { const revisionBlocks = within(ribbon).getAllByRole('listitem') return revisionBlocks .map(item => item.textContent) - .filter(text => !text?.includes(' of ') && text !== 'Refresh All') + .filter( + text => + !text?.includes(' of ') && + text !== 'Refresh All' && + text !== 'Add Plot' + ) } it('should show the revisions at the top', () => { @@ -2412,6 +2413,25 @@ describe('App', () => { ).toBeInTheDocument() }) + it('should send a message to add a plot when clicking the add plot button', () => { + renderAppWithOptionalData({ + comparison: comparisonTableFixture, + selectedRevisions: plotsRevisionsFixture + }) + + const addPlotButton = within(screen.getByTestId('ribbon')).getAllByRole( + 'button' + )[0] + + mockPostMessage.mockReset() + fireEvent.click(addPlotButton) + + expect(mockPostMessage).toHaveBeenCalledTimes(1) + expect(mockPostMessage).toHaveBeenCalledWith({ + type: MessageFromWebviewType.ADD_PLOT + }) + }) + it('should send a message to select the revisions when clicking the filter button', () => { renderAppWithOptionalData({ comparison: comparisonTableFixture, @@ -2420,7 +2440,7 @@ describe('App', () => { const filterButton = within(screen.getByTestId('ribbon')).getAllByRole( 'button' - )[0] + )[1] fireEvent.click(filterButton) @@ -2432,13 +2452,12 @@ describe('App', () => { it('should send a message to refresh each revision when clicking the refresh all button', () => { renderAppWithOptionalData({ comparison: comparisonTableFixture, - selectedRevisions: plotsRevisionsFixture }) const refreshAllButton = within( screen.getByTestId('ribbon') - ).getAllByRole('button')[1] + ).getAllByRole('button')[2] mockPostMessage.mockReset() fireEvent.click(refreshAllButton) diff --git a/webview/src/plots/components/emptyState/AddPlots.tsx b/webview/src/plots/components/emptyState/AddPlots.tsx index e162f59f7b..5ddcc8b2f1 100644 --- a/webview/src/plots/components/emptyState/AddPlots.tsx +++ b/webview/src/plots/components/emptyState/AddPlots.tsx @@ -1,48 +1,32 @@ import React from 'react' -import { - addCustomPlot, - addPipelinePlot, - selectPlots, - selectRevisions -} from '../../util/messages' +import { addPlot, selectPlots, selectRevisions } from '../../util/messages' import { StartButton } from '../../../shared/components/button/StartButton' type AddPlotsProps = { - hasCustomPlots: boolean hasUnselectedPlots: boolean } export const AddPlots: React.FC = ({ - hasCustomPlots, hasUnselectedPlots }: AddPlotsProps) => (

No Plots to Display

- {hasUnselectedPlots ? ( + {hasUnselectedPlots && ( - ) : ( - - )} - {!hasCustomPlots && ( - )} +
) diff --git a/webview/src/plots/components/emptyState/GetStarted.tsx b/webview/src/plots/components/emptyState/GetStarted.tsx index 2cd70757e0..0f507ee3f4 100644 --- a/webview/src/plots/components/emptyState/GetStarted.tsx +++ b/webview/src/plots/components/emptyState/GetStarted.tsx @@ -18,10 +18,7 @@ export const GetStarted: React.FC = ({ }) => ( {hasPlots ? ( - + ) : ( )} diff --git a/webview/src/plots/components/emptyState/Welcome.tsx b/webview/src/plots/components/emptyState/Welcome.tsx index 52e8e3aa81..eb2cbfb196 100644 --- a/webview/src/plots/components/emptyState/Welcome.tsx +++ b/webview/src/plots/components/emptyState/Welcome.tsx @@ -1,12 +1,12 @@ import React from 'react' import { StartButton } from '../../../shared/components/button/StartButton' -import { addPipelinePlot, selectRevisions } from '../../util/messages' +import { addPlot, selectRevisions } from '../../util/messages' export const Welcome: React.FC = () => (

No Plots Detected.

- + { data-testid="ribbon" className={cx(styles.list, needsShadow && styles.withShadow)} > +
  • + +
  • diff --git a/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx b/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx index 471878fedb..daf879e6f3 100644 --- a/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx +++ b/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx @@ -5,7 +5,6 @@ import { TemplatePlots } from './TemplatePlots' import { changeSize } from './templatePlotsSlice' import { PlotsContainer } from '../PlotsContainer' import { PlotsState } from '../../store' -import { addPipelinePlot } from '../../util/messages' export const TemplatePlotsWrapper: React.FC = () => { const { nbItemsPerRow, isCollapsed, height, hasItems } = useSelector( @@ -21,7 +20,6 @@ export const TemplatePlotsWrapper: React.FC = () => { changeSize={changeSize} hasItems={hasItems} height={height} - addPlotsButton={{ onClick: addPipelinePlot }} > diff --git a/webview/src/plots/util/messages.ts b/webview/src/plots/util/messages.ts index 506903eabc..0b5ac4ba31 100644 --- a/webview/src/plots/util/messages.ts +++ b/webview/src/plots/util/messages.ts @@ -3,16 +3,15 @@ import { PlotsSection } from 'dvc/src/plots/webview/contract' import { sendMessage } from '../../shared/vscode' import { PlotGroup } from '../components/templatePlots/templatePlotsSlice' -export const addCustomPlot = () => +export const addPlot = () => sendMessage({ - type: MessageFromWebviewType.ADD_CUSTOM_PLOT + type: MessageFromWebviewType.ADD_PLOT }) -export const addPipelinePlot = () => { +export const addCustomPlot = () => sendMessage({ - type: MessageFromWebviewType.ADD_PIPELINE_PLOT + type: MessageFromWebviewType.ADD_CUSTOM_PLOT }) -} export const exportPlotDataAsCsv = (id: string) => { sendMessage({