Add missing "Hide Column" option to the other headers (#2593)

* Add missing options to the other headers

* Remove timestamp restriction

* Refactor context-menu options in table header (#2643)

* Move context menu stuff lower level component

* Refactor TableHeaderCell

* Extend sorting options to placeholders

* Add test for placeholder options

* Fix the options in the Experiment columns

* Add a failling test for the parent/child mismatch

* Add PathSelectionModel::unselect()

* Unselect, rather than toggleStatus, when hiding a colum

* Send the leaf column id for ctx options

* Fix sorting from placeholder

* Fix merge conflict

* Revert demo

Co-authored-by: Stephanie Roy <[email protected]>
Co-authored-by: Stephanie Roy <[email protected]>

* Reduce number of lines

Co-authored-by: Stephanie Roy <[email protected]>
Co-authored-by: Stephanie Roy <[email protected]>

Author: 3 people

extension/src/experiments/columns/model.test.ts

@@ -364,4 +364,74 @@ describe('ColumnsModel', () => {
       expect(model.getColumnWidths()[changedColumnId]).toBe(expectedWidth)
     })
   })
+
+  describe('columns selection', () => {
+    const expectStatus = (
+      columns: Array<{ status: Status }>,
+      expectedStatus: Status
+    ) => {
+      for (const { status } of columns) {
+        expect(status).toStrictEqual(expectedStatus)
+      }
+    }
+
+    it('should unselect the children of a unselected column', async () => {
+      const mockMemento = buildMockMemento({
+        [PersistenceKey.METRICS_AND_PARAMS_STATUS + exampleDvcRoot]: {}
+      })
+      const model = new ColumnsModel(
+        exampleDvcRoot,
+        mockMemento,
+        mockedColumnsOrderOrStatusChanged
+      )
+      await model.transformAndSet(outputFixture)
+      expect(model.getSelected()).toStrictEqual(columnsFixture)
+
+      const parentPath = 'params:params.yaml:process'
+      model.toggleStatus(parentPath)
+      const children = model.getChildren(parentPath)
+      expectStatus(children, Status.UNSELECTED)
+    })
+
+    it('should select the children of a selected column', async () => {
+      const parentPath = 'params:params.yaml:process'
+      const mockMemento = buildMockMemento({
+        [PersistenceKey.METRICS_AND_PARAMS_STATUS + exampleDvcRoot]: {
+          [parentPath]: Status.UNSELECTED
+        }
+      })
+      const model = new ColumnsModel(
+        exampleDvcRoot,
+        mockMemento,
+        mockedColumnsOrderOrStatusChanged
+      )
+      await model.transformAndSet(outputFixture)
+      expect(model.getSelected()).not.toStrictEqual(columnsFixture)
+
+      model.toggleStatus(parentPath)
+
+      const children = model.getChildren(parentPath)
+      expectStatus(children, Status.SELECTED)
+    })
+
+    it('should unselect the children of an indeterminate column when it is unselected', async () => {
+      const mockMemento = buildMockMemento({
+        [PersistenceKey.METRICS_AND_PARAMS_STATUS + exampleDvcRoot]: {}
+      })
+      const model = new ColumnsModel(
+        exampleDvcRoot,
+        mockMemento,
+        mockedColumnsOrderOrStatusChanged
+      )
+      await model.transformAndSet(outputFixture)
+      expect(model.getSelected()).toStrictEqual(columnsFixture)
+
+      const parentPath = 'metrics:summary.json'
+      model.toggleStatus('metrics:summary.json:val_loss')
+      model.unselect(parentPath)
+
+      const children = model.getChildren(parentPath)
+      expectStatus(children, Status.UNSELECTED)
+    })
+  })
 })

extension/src/experiments/webview/messages.ts

@@ -276,7 +276,7 @@ export class WebviewMessages {
   }
 
   private hideTableColumn(path: string) {
-    this.columns.toggleStatus(path)
+    this.columns.unselect(path)
 
     this.notifyChanged()
 

extension/src/path/selection/model.ts

@@ -51,13 +51,11 @@ export abstract class PathSelectionModel<
 
   public toggleStatus(path: string) {
     const status = this.getNextStatus(path)
-    this.status[path] = status
-    this.setAreChildrenSelected(path, status)
-    this.setAreParentsSelected(path)
-    this.persistStatus()
-    this.statusChanged?.fire()
+    return this.setStatus(path, status)
+  }
 
-    return this.status[path]
+  public unselect(path: string) {
+    return this.setStatus(path, Status.UNSELECTED)
   }
 
   public getTerminalNodeStatuses(parentPath?: string): Status[] {
@@ -90,6 +88,16 @@ export abstract class PathSelectionModel<
     }
   }
 
+  private setStatus(path: string, status: Status) {
+    this.status[path] = status
+    this.setAreChildrenSelected(path, status)
+    this.setAreParentsSelected(path)
+    this.persistStatus()
+    this.statusChanged?.fire()
+
+    return this.status[path]
+  }
+
   private setAreChildrenSelected(path: string, status: Status) {
     return this.getChildren(path)?.map(element => {
       const path = element.path

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

@@ -355,7 +355,7 @@ suite('Experiments Test Suite', () => {
     it('should be able to handle a message to hide a table column', async () => {
       const { experiments, columnsModel } = buildExperiments(disposable)
 
-      const mockToggleStatus = stub(columnsModel, 'toggleStatus')
+      const mockUnselect = stub(columnsModel, 'unselect')
       const mockSendTelemetryEvent = stub(Telemetry, 'sendTelemetryEvent')
       const webview = await experiments.showWebview()
       const mockMessageReceived = getMessageReceivedEmitter(webview)
@@ -366,8 +366,8 @@ suite('Experiments Test Suite', () => {
         type: MessageFromWebviewType.HIDE_EXPERIMENTS_TABLE_COLUMN
       })
 
-      expect(mockToggleStatus).to.be.calledOnce
-      expect(mockToggleStatus).to.be.calledWithExactly(mockColumnId)
+      expect(mockUnselect).to.be.calledOnce
+      expect(mockUnselect).to.be.calledWithExactly(mockColumnId)
 
       expect(mockSendTelemetryEvent).to.be.calledWithExactly(
         EventName.VIEWS_EXPERIMENTS_TABLE_HIDE_COLUMN,

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

@@ -760,11 +760,82 @@ describe('App', () => {
       advanceTimersByTime(100)
 
       const menuitems = screen.getAllByRole('menuitem')
-      expect(menuitems).toHaveLength(2)
+      expect(menuitems).toHaveLength(3)
 
       fireEvent.keyDown(paramsFileHeader, { bubbles: true, key: 'Escape' })
       expect(screen.queryAllByRole('menuitem')).toHaveLength(0)
     })
+
+    it('should have the same options in the empty placeholders', () => {
+      renderTableWithPlaceholder()
+      const header = screen.getByTestId('header-Created')
+      const placeholders = screen.getAllByTestId(/header-Created.+placeholder/)
+      const entireColumn = [header, ...placeholders]
+
+      expect(entireColumn).toHaveLength(5)
+
+      for (const segment of entireColumn) {
+        fireEvent.contextMenu(segment, { bubbles: true })
+        jest.advanceTimersByTime(100)
+        const menuitems = screen
+          .getAllByRole('menuitem')
+          .map(item => item.textContent)
+
+        expect(menuitems).toStrictEqual([
+          'Hide Column',
+          'Set Max Header Height',
+          'Sort Ascending',
+          'Sort Descending'
+        ])
+
+        fireEvent.keyDown(segment, { bubbles: true, key: 'Escape' })
+      }
+    })
+
+    it('should have the same options in the empty placeholders of the Experiment column', () => {
+      renderTableWithPlaceholder()
+      const header = screen.getByTestId('header-id')
+      const placeholders = screen.getAllByTestId(/header-id.+placeholder/)
+      const entireColumn = [header, ...placeholders]
+
+      expect(entireColumn).toHaveLength(5)
+
+      for (const segment of entireColumn) {
+        fireEvent.contextMenu(segment, { bubbles: true })
+        jest.advanceTimersByTime(100)
+        const menuitems = screen
+          .getAllByRole('menuitem')
+          .map(item => item.textContent)
+
+        expect(menuitems).toStrictEqual(['Set Max Header Height'])
+
+        fireEvent.keyDown(segment, { bubbles: true, key: 'Escape' })
+      }
+    })
+
+    describe('Hiding a column from its empty placeholder', () => {
+      it('should send the column id and not the placeholder id as the message payload', () => {
+        renderTableWithPlaceholder()
+        const placeholders = screen.getAllByTestId(
+          /header-Created.+placeholder/
+        )
+        const placeholder = placeholders[0]
+        fireEvent.contextMenu(placeholder, { bubbles: true })
+        jest.advanceTimersByTime(100)
+
+        const hideOption = screen.getByText('Hide Column')
+
+        mockPostMessage.mockClear()
+
+        fireEvent.click(hideOption)
+
+        expect(mockPostMessage).toHaveBeenCalledTimes(1)
+        expect(mockPostMessage).toHaveBeenCalledWith({
+          payload: 'Created',
+          type: MessageFromWebviewType.HIDE_EXPERIMENTS_TABLE_COLUMN
+        })
+      })
+    })
   })
 
   describe('Row Context Menu', () => {

webview/src/experiments/components/table/Table.test.tsx

@@ -18,9 +18,9 @@ import { MessageFromWebviewType } from 'dvc/src/webview/contract'
 import React from 'react'
 import { TableInstance } from 'react-table'
 import tableDataFixture from 'dvc/src/test/fixtures/expShow/base/tableData'
-import { SortOrder } from './header/TableHeader'
 import { Table } from './Table'
 import styles from './styles.module.scss'
+import { SortOrder } from './header/ContextMenuContent'
 import { ExperimentsTable } from '../Experiments'
 import * as ColumnOrder from '../../hooks/useColumnOrder'
 import { vsCodeApi } from '../../../shared/api'