Improve table indicators (#4212)

Author: mattseddon

extension/src/experiments/webview/contract.ts

@@ -97,7 +97,6 @@ export type TableData = {
   columnOrder: string[]
   columns: Column[]
   columnWidths: Record<string, number>
-  filteredCount: number
   filters: string[]
   hasBranchesToSelect: boolean
   hasCheckpoints: boolean

extension/src/experiments/webview/messages.ts

@@ -154,15 +154,15 @@ export class WebviewMessages {
         return this.focusSortsTree()
 
       case MessageFromWebviewType.OPEN_PLOTS_WEBVIEW:
-        return this.showPlotsToSide()
+        return this.showPlots()
 
       case MessageFromWebviewType.SET_EXPERIMENTS_FOR_PLOTS:
         return this.setSelectedExperiments(message.payload)
 
       case MessageFromWebviewType.SET_EXPERIMENTS_AND_OPEN_PLOTS:
         return Promise.all([
           this.setSelectedExperiments(message.payload),
-          this.showPlotsToSide()
+          this.showPlots()
         ])
 
       case MessageFromWebviewType.SET_EXPERIMENTS_HEADER_HEIGHT: {
@@ -262,7 +262,6 @@ export class WebviewMessages {
       columnOrder: this.columns.getColumnOrder(),
       columnWidths: this.columns.getColumnWidths(),
       columns: this.columns.getSelected(),
-      filteredCount: this.experiments.getFilteredCount(),
       filters: this.experiments.getFilterPaths(),
       hasBranchesToSelect:
         this.experiments.getAvailableBranchesToShow().length > 0,
@@ -424,10 +423,7 @@ export class WebviewMessages {
     )
   }
 
-  private showPlotsToSide() {
-    return commands.executeCommand(
-      RegisteredCommands.EXPERIMENT_AND_PLOTS_SHOW,
-      this.dvcRoot
-    )
+  private showPlots() {
+    return commands.executeCommand(RegisteredCommands.PLOTS_SHOW, this.dvcRoot)
   }
 }

extension/src/test/fixtures/expShow/base/tableData.ts

@@ -8,7 +8,6 @@ const tableDataFixture: TableData = {
   columnOrder: [],
   columns: columnsFixture,
   columnWidths: {},
-  filteredCount: 0,
   filters: [],
   hasBranchesToSelect: true,
   hasCheckpoints: true,

extension/src/test/suite/experiments/index.test.ts

@@ -135,7 +135,6 @@ suite('Experiments Test Suite', () => {
         columnOrder: columnsOrderFixture,
         columnWidths: {},
         columns: columnsFixture,
-        filteredCount: 0,
         filters: [],
         hasCheckpoints: true,
         hasColumns: true,
@@ -1070,7 +1069,6 @@ suite('Experiments Test Suite', () => {
         columnOrder: columnsOrderFixture,
         columnWidths: {},
         columns: [],
-        filteredCount: 0,
         filters: [],
         rows: rowsFixture,
         sorts: []
@@ -1316,6 +1314,37 @@ suite('Experiments Test Suite', () => {
       expect(mockShowPlots).to.be.calledWith(dvcDemoPath)
     }).timeout(WEBVIEW_TEST_TIMEOUT)
 
+    it('should be able to handle a message to open the plots webview', async () => {
+      stub(Setup.prototype, 'shouldBeShown').returns({
+        dvc: true,
+        experiments: true
+      })
+      const { experiments } = buildExperiments({
+        disposer: disposable
+      })
+      const mockShowPlots = stub(WorkspacePlots.prototype, 'showWebview')
+      const webviewOpened = new Promise(resolve =>
+        mockShowPlots.callsFake(() => {
+          resolve(undefined)
+          return Promise.resolve(undefined)
+        })
+      )
+
+      await experiments.isReady()
+
+      const webview = await experiments.showWebview()
+      const mockMessageReceived = getMessageReceivedEmitter(webview)
+
+      mockMessageReceived.fire({
+        type: MessageFromWebviewType.OPEN_PLOTS_WEBVIEW
+      })
+
+      await webviewOpened
+
+      expect(mockShowPlots).to.be.calledOnce
+      expect(mockShowPlots).to.be.calledWith(dvcDemoPath)
+    }).timeout(WEBVIEW_TEST_TIMEOUT)
+
     it('should handle a message to stop experiments running', async () => {
       const { experiments, dvcExecutor } = buildExperiments({
         disposer: disposable

extension/src/test/suite/experiments/model/filterBy/tree.test.ts

@@ -106,7 +106,6 @@ suite('Experiments Filter By Tree Test Suite', () => {
 
       const filteredTableData: Partial<TableData> = {
         changes: workspaceChangesFixture,
-        filteredCount: 1,
         filters: [accuracyPath],
         rows: filteredRows
       }
@@ -131,7 +130,6 @@ suite('Experiments Filter By Tree Test Suite', () => {
       const unfilteredTableData: Partial<TableData> = {
         changes: workspaceChangesFixture,
         columns: columnsFixture,
-        filteredCount: 0,
         filters: [],
         rows: [workspace, main, fe2919b, _7df876c]
       }
@@ -371,7 +369,6 @@ suite('Experiments Filter By Tree Test Suite', () => {
         changes: workspaceChangesFixture,
         columnOrder: columnsOrderFixture,
         columns: columnsFixture,
-        filteredCount: 6,
         filters: ['starred'],
         rows: filteredRows
       }

webview/src/experiments/components/App.test.tsx

@@ -774,7 +774,7 @@ describe('App', () => {
 
       for (const segment of entireColumn) {
         fireEvent.contextMenu(segment, { bubbles: true })
-        jest.advanceTimersByTime(100)
+        advanceTimersByTime(100)
         const menuitems = screen
           .getAllByRole('menuitem')
           .filter(item => !item.className.includes('disabled'))
@@ -801,7 +801,7 @@ describe('App', () => {
 
       for (const segment of entireColumn) {
         fireEvent.contextMenu(segment, { bubbles: true })
-        jest.advanceTimersByTime(100)
+        advanceTimersByTime(100)
         const menuitems = screen
           .getAllByRole('menuitem')
           .filter(item => !item.className.includes('disabled'))
@@ -821,7 +821,7 @@ describe('App', () => {
         )
         const placeholder = placeholders[0]
         fireEvent.contextMenu(placeholder, { bubbles: true })
-        jest.advanceTimersByTime(100)
+        advanceTimersByTime(100)
 
         const hideOption = screen.getByText('Hide Column')
 
@@ -1486,19 +1486,18 @@ describe('App', () => {
       renderTable({
         ...tableDataFixture
       })
+      jest.useFakeTimers()
       const selectedForPlotsIndicator =
         screen.getByLabelText('selected for plots')
       expect(selectedForPlotsIndicator).toHaveTextContent('2')
 
       expect(screen.queryByRole('tooltip')).not.toBeInTheDocument()
 
       fireEvent.mouseEnter(selectedForPlotsIndicator)
-
+      advanceTimersByTime(1000)
       const tooltip = screen.getByRole('tooltip')
 
-      expect(tooltip).toHaveTextContent(
-        '2 Experiments Selected for Plotting (Max 7)'
-      )
+      expect(tooltip).toHaveTextContent('Show Plots')
 
       setTableData({
         ...tableDataFixture,
@@ -1510,9 +1509,6 @@ describe('App', () => {
       })
 
       expect(selectedForPlotsIndicator).toHaveTextContent('')
-      expect(tooltip).toHaveTextContent(
-        'No Experiments Selected for Plotting (Max 7)'
-      )
 
       setTableData({
         ...tableDataFixture,
@@ -1533,12 +1529,10 @@ describe('App', () => {
       })
 
       expect(selectedForPlotsIndicator).toHaveTextContent('1')
-      expect(tooltip).toHaveTextContent(
-        '1 Experiment Selected for Plotting (Max 7)'
-      )
+      jest.useRealTimers()
     })
 
-    it('should show not change the plotted indicator when plotted experiments are hidden', () => {
+    it('should not change the plotted indicator when plotted experiments are hidden', () => {
       const plottedExperiment = '4fb124a'
 
       renderTable({
@@ -1547,49 +1541,30 @@ describe('App', () => {
 
       expect(screen.getByText(plottedExperiment)).toBeInTheDocument()
 
-      const selectedForPlotsIndicator =
-        screen.getByLabelText('selected for plots')
-      expect(selectedForPlotsIndicator).toHaveTextContent('2')
-
-      expect(screen.queryByRole('tooltip')).not.toBeInTheDocument()
-
-      fireEvent.mouseEnter(selectedForPlotsIndicator)
-
-      const expandedTooltip = screen.getByRole('tooltip')
-
-      expect(expandedTooltip).toHaveTextContent(
-        '2 Experiments Selected for Plotting (Max 7)'
-      )
-
-      fireEvent.mouseLeave(selectedForPlotsIndicator)
+      expect(screen.getByLabelText('selected for plots')).toHaveTextContent('2')
 
       contractRow('main')
 
-      expect(screen.queryByText(plottedExperiment)).not.toBeInTheDocument()
-      fireEvent.mouseEnter(selectedForPlotsIndicator)
-
-      const contractedTooltip = screen.getByRole('tooltip')
-
-      expect(contractedTooltip).toHaveTextContent(
-        '2 Experiments Selected for Plotting (Max 7)'
-      )
+      expect(screen.getByLabelText('selected for plots')).toHaveTextContent('2')
     })
 
     it('should show an indicator with the amount of applied sorts', () => {
       renderTable({
         ...tableDataFixture,
         sorts: []
       })
+      jest.useFakeTimers()
       const sortIndicator = screen.getByLabelText('sorts')
       expect(sortIndicator).toHaveTextContent('')
 
       expect(screen.queryByRole('tooltip')).not.toBeInTheDocument()
 
       fireEvent.mouseEnter(sortIndicator)
+      advanceTimersByTime(1000)
 
       const tooltip = screen.getByRole('tooltip')
 
-      expect(tooltip).toHaveTextContent('No Sorts Applied')
+      expect(tooltip).toHaveTextContent('Show Sorts')
 
       const { columns } = tableDataFixture
       const firstSortPath = columns[columns.length - 1].path
@@ -1599,7 +1574,6 @@ describe('App', () => {
         sorts: [{ descending: true, path: firstSortPath }]
       })
       expect(sortIndicator).toHaveTextContent('1')
-      expect(tooltip).toHaveTextContent('1 Sort Applied')
       setTableData({
         ...tableDataFixture,
         sorts: [
@@ -1608,7 +1582,7 @@ describe('App', () => {
         ]
       })
       expect(sortIndicator).toHaveTextContent('2')
-      expect(tooltip).toHaveTextContent('2 Sorts Applied')
+      jest.useRealTimers()
     })
   })
 
@@ -1617,16 +1591,18 @@ describe('App', () => {
       ...tableDataFixture,
       filters: []
     })
+    jest.useFakeTimers()
     const filterIndicator = screen.getByLabelText('filters')
     expect(filterIndicator).toHaveTextContent('')
 
     expect(screen.queryByRole('tooltip')).not.toBeInTheDocument()
 
     fireEvent.mouseEnter(filterIndicator)
+    advanceTimersByTime(1000)
 
     const tooltip = screen.getByRole('tooltip')
 
-    expect(tooltip).toHaveTextContent('No Filters Applied')
+    expect(tooltip).toHaveTextContent('Show Filters')
 
     const { columns } = tableDataFixture
     const firstFilterPath = columns[columns.length - 1].path
@@ -1636,33 +1612,89 @@ describe('App', () => {
       filters: [firstFilterPath]
     })
     expect(filterIndicator).toHaveTextContent('1')
-    expect(tooltip).toHaveTextContent('1 Filter Applied')
-    expect(tooltip).toHaveTextContent('No Experiments Filtered')
 
     setTableData({
       ...tableDataFixture,
-      filteredCount: 1,
       filters: [firstFilterPath, secondFilterPath]
     })
     expect(filterIndicator).toHaveTextContent('2')
-    expect(tooltip).toHaveTextContent('2 Filters Applied')
-    expect(tooltip).toHaveTextContent('1 Experiment Filtered')
 
     setTableData({
       ...tableDataFixture,
-      filteredCount: 10000,
       filters: [firstFilterPath, secondFilterPath]
     })
     expect(filterIndicator).toHaveTextContent('2')
-    expect(tooltip).toHaveTextContent('10000 Experiments Filtered')
 
     setTableData({
       ...tableDataFixture,
-      filteredCount: 10000,
       filters: []
     })
     expect(filterIndicator).toHaveTextContent('')
     expect(tooltip).not.toHaveTextContent('Experiment')
+    jest.useRealTimers()
+  })
+
+  it('should show a tooltip for the branches indicator', () => {
+    renderTable({
+      ...tableDataFixture
+    })
+    jest.useFakeTimers()
+    const branchesIndicator = screen.getByLabelText('branches')
+    expect(branchesIndicator).toHaveTextContent('')
+
+    expect(screen.queryByRole('tooltip')).not.toBeInTheDocument()
+
+    fireEvent.mouseEnter(branchesIndicator)
+    advanceTimersByTime(1000)
+
+    const tooltip = screen.getByRole('tooltip')
+
+    expect(tooltip).toHaveTextContent('Select Branches')
+    jest.useRealTimers()
+  })
+
+  it('should show an indicator for the number of branches selected', () => {
+    const branches = ['main', 'other', 'third']
+
+    let workspace
+    const rowsWithoutWorkspace = []
+    for (const row of tableDataFixture.rows) {
+      if (row.id !== EXPERIMENT_WORKSPACE_ID) {
+        rowsWithoutWorkspace.push(row)
+        continue
+      }
+      workspace = row
+    }
+
+    const multipleBranches = {
+      ...tableDataFixture,
+      branches,
+      hasData: true,
+      rows: [
+        workspace as Commit,
+        ...rowsWithoutWorkspace.map(row => ({
+          ...row,
+          branch: branches[0],
+          subRows: undefined
+        })),
+        ...rowsWithoutWorkspace.map(row => ({
+          ...row,
+          branch: branches[1],
+          subRows: undefined
+        })),
+        ...rowsWithoutWorkspace.map(row => ({
+          ...row,
+          branch: branches[2],
+          subRows: undefined
+        }))
+      ]
+    }
+
+    renderTable(multipleBranches)
+
+    const [indicator] = screen.getAllByLabelText('branches')
+
+    expect(indicator).toHaveTextContent(`${branches.length - 1}`)
   })
 
   it('should send a message to focus the relevant tree when clicked', () => {