fix(app, app-shell): fix python override path select on windows (#11257)

Re: RAUT-61

Co-authored-by: Vincent Abruzzo <[email protected]>
Co-authored-by: koji <[email protected]>

Author: 3 people

app-shell/src/config/index.ts

@@ -3,7 +3,6 @@
 import Store from 'electron-store'
 import get from 'lodash/get'
 import mergeOptions from 'merge-options'
-import { shell } from 'electron'
 import yargsParser from 'yargs-parser'
 
 import { UI_INITIALIZED } from '@opentrons/app/src/redux/shell/actions'
@@ -119,17 +118,3 @@ export function handleConfigChange(
 ): void {
   store().onDidChange(path, changeHandler)
 }
-
-export function registerPythonPath(): Dispatch {
-  return function handleActionForPython(action: Action) {
-    switch (action.type) {
-      case Cfg.OPEN_PYTHON_DIRECTORY: {
-        const dir = getFullConfig().python.pathToPythonOverride
-        if (dir != null) {
-          shell.openPath(dir)
-        }
-        break
-      }
-    }
-  }
-}

app-shell/src/dialogs/index.ts

@@ -1,4 +1,4 @@
-import { dialog } from 'electron'
+import { dialog, shell } from 'electron'
 import type {
   BrowserWindow,
   OpenDialogOptions,
@@ -61,3 +61,10 @@ export function showOpenFileDialog(
       return result.canceled ? [] : (result.filePaths as string[])
     })
 }
+
+export function openDirectoryInFileExplorer(
+  directory: string | null
+): Promise<string | null> {
+  if (directory == null) return Promise.resolve(null)
+  return shell.openPath(directory)
+}

app-shell/src/main.ts

@@ -4,22 +4,16 @@ import contextMenu from 'electron-context-menu'
 
 import { createUi } from './ui'
 import { initializeMenu } from './menu'
-import { initializePython } from './protocol-analysis'
 import { createLogger } from './log'
+import { registerProtocolAnalysis } from './protocol-analysis'
 import { registerDiscovery } from './discovery'
 import { registerLabware } from './labware'
 import { registerRobotLogs } from './robot-logs'
 import { registerUpdate } from './update'
 import { registerBuildrootUpdate } from './buildroot'
 import { registerSystemInfo } from './system-info'
 import { registerProtocolStorage } from './protocol-storage'
-import {
-  getConfig,
-  getStore,
-  getOverrides,
-  registerConfig,
-  registerPythonPath,
-} from './config'
+import { getConfig, getStore, getOverrides, registerConfig } from './config'
 
 import type { BrowserWindow } from 'electron'
 import type { Dispatch, Logger } from './types'
@@ -76,7 +70,6 @@ function startUp(): void {
   })
 
   initializeMenu()
-  initializePython()
 
   // wire modules to UI dispatches
   const dispatch: Dispatch = action => {
@@ -90,11 +83,11 @@ function startUp(): void {
   const actionHandlers: Dispatch[] = [
     registerConfig(dispatch),
     registerDiscovery(dispatch),
+    registerProtocolAnalysis(dispatch, mainWindow),
     registerRobotLogs(dispatch, mainWindow),
     registerUpdate(dispatch),
     registerBuildrootUpdate(dispatch),
     registerLabware(dispatch, mainWindow),
-    registerPythonPath(),
     registerSystemInfo(dispatch),
     registerProtocolStorage(dispatch),
   ]

app-shell/src/protocol-analysis/__tests__/protocolAnalysis.test.ts

@@ -1,14 +1,24 @@
 import { when, resetAllWhenMocks } from 'jest-when'
+import electron from 'electron'
+import * as ProtocolAnalysis from '@opentrons/app/src/redux/protocol-analysis'
+import * as Cfg from '@opentrons/app/src/redux/config'
 
+import * as Dialogs from '../../dialogs'
 import { Config, getConfig, handleConfigChange } from '../../config'
 import { getValidLabwareFilePaths } from '../../labware'
 import { selectPythonPath, getPythonPath } from '../getPythonPath'
 import { executeAnalyzeCli } from '../executeAnalyzeCli'
 import { writeFailedAnalysis } from '../writeFailedAnalysis'
 
-import { initializePython, analyzeProtocolSource } from '..'
+import {
+  registerProtocolAnalysis,
+  analyzeProtocolSource,
+  CONFIG_PYTHON_PATH_TO_PYTHON_OVERRIDE,
+} from '..'
+import { Dispatch } from '../../types'
 
 jest.mock('../../labware')
+jest.mock('../../dialogs')
 jest.mock('../getPythonPath')
 jest.mock('../executeAnalyzeCli')
 jest.mock('../writeFailedAnalysis')
@@ -32,21 +42,37 @@ const mockGetValidLabwareFilePaths = getValidLabwareFilePaths as jest.MockedFunc
 const mockHandleConfigChange = handleConfigChange as jest.MockedFunction<
   typeof handleConfigChange
 >
+const mockShowOpenDirectoryDialog = Dialogs.showOpenDirectoryDialog as jest.MockedFunction<
+  typeof Dialogs.showOpenDirectoryDialog
+>
+const mockOpenDirectoryInFileExplorer = Dialogs.openDirectoryInFileExplorer as jest.MockedFunction<
+  typeof Dialogs.openDirectoryInFileExplorer
+>
+
+// wait a few ticks to let the mock Promises clear
+const flush = (): Promise<void> =>
+  new Promise(resolve => setTimeout(resolve, 0))
 
 describe('analyzeProtocolSource', () => {
+  const mockMainWindow = ({
+    browserWindow: true,
+  } as unknown) as electron.BrowserWindow
+  let dispatch: jest.MockedFunction<Dispatch>
+  let handleAction: Dispatch
+
+  beforeEach(() => {
+    dispatch = jest.fn()
+    mockGetConfig.mockReturnValue({
+      python: { pathToPythonOverride: '/some/override/python' },
+    } as Config)
+    handleAction = registerProtocolAnalysis(dispatch, mockMainWindow)
+  })
+
   afterEach(() => {
     resetAllWhenMocks()
   })
 
   it('should be able to initialize the Python path', () => {
-    when(mockGetConfig)
-      .calledWith()
-      .mockReturnValue({
-        python: { pathToPythonOverride: '/some/override/python' },
-      } as Config)
-
-    initializePython()
-
     expect(mockSelectPythonPath).toHaveBeenCalledWith('/some/override/python')
     expect(mockHandleConfigChange).toHaveBeenCalledWith(
       'python.pathToPythonOverride',
@@ -111,4 +137,46 @@ describe('analyzeProtocolSource', () => {
       expect(mockWriteFailedAnalysis).toHaveBeenCalledWith(outputPath, 'oh no')
     })
   })
+
+  it('should open file picker in response to CHANGE_PYTHON_PATH_OVERRIDE and not call dispatch if no directory is returned from showOpenDirectoryDialog', () => {
+    when(mockShowOpenDirectoryDialog)
+      .calledWith(mockMainWindow)
+      .mockResolvedValue([])
+    handleAction(ProtocolAnalysis.changePythonPathOverrideConfig())
+
+    return flush().then(() => {
+      expect(mockShowOpenDirectoryDialog).toHaveBeenCalledWith(mockMainWindow)
+      expect(dispatch).not.toHaveBeenCalled()
+    })
+  })
+
+  it('should open file picker in response to CHANGE_PYTHON_PATH_OVERRIDE and call dispatch with directory returned from showOpenDirectoryDialog', () => {
+    when(mockShowOpenDirectoryDialog)
+      .calledWith(mockMainWindow)
+      .mockResolvedValue(['path/to/override'])
+    handleAction(ProtocolAnalysis.changePythonPathOverrideConfig())
+
+    return flush().then(() => {
+      expect(mockShowOpenDirectoryDialog).toHaveBeenCalledWith(mockMainWindow)
+      expect(dispatch).toHaveBeenCalledWith(
+        Cfg.updateConfigValue(
+          CONFIG_PYTHON_PATH_TO_PYTHON_OVERRIDE,
+          'path/to/override'
+        )
+      )
+    })
+  })
+
+  it('should call openDirectoryInFileExplorer in response to OPEN_PYTHON_DIRECTORY', () => {
+    when(mockOpenDirectoryInFileExplorer)
+      .calledWith('/some/override/python')
+      .mockResolvedValue(null)
+    handleAction(ProtocolAnalysis.openPythonInterpreterDirectory())
+
+    return flush().then(() => {
+      expect(mockOpenDirectoryInFileExplorer).toHaveBeenCalledWith(
+        '/some/override/python'
+      )
+    })
+  })
 })

app-shell/src/protocol-analysis/index.ts

@@ -1,20 +1,65 @@
+import * as ProtocolAnalysis from '@opentrons/app/src/redux/protocol-analysis'
+import * as Cfg from '@opentrons/app/src/redux/config'
+
 import { createLogger } from '../log'
 import { getConfig, handleConfigChange } from '../config'
-
 import { getValidLabwareFilePaths } from '../labware'
+import {
+  showOpenDirectoryDialog,
+  openDirectoryInFileExplorer,
+} from '../dialogs'
 import { selectPythonPath, getPythonPath } from './getPythonPath'
 import { executeAnalyzeCli } from './executeAnalyzeCli'
 import { writeFailedAnalysis } from './writeFailedAnalysis'
 
+import type { BrowserWindow } from 'electron'
+import type { Action, Dispatch } from '../types'
+
 const log = createLogger('protocol-analysis')
 
-export function initializePython(): void {
+export const CONFIG_PYTHON_PATH_TO_PYTHON_OVERRIDE =
+  'python.pathToPythonOverride'
+
+export function registerProtocolAnalysis(
+  dispatch: Dispatch,
+  mainWindow: BrowserWindow
+): (action: Action) => void {
   const pathToPythonOverride = getConfig().python.pathToPythonOverride
   selectPythonPath(pathToPythonOverride)
 
-  handleConfigChange('python.pathToPythonOverride', newValue => {
+  handleConfigChange(CONFIG_PYTHON_PATH_TO_PYTHON_OVERRIDE, newValue => {
     selectPythonPath(newValue)
   })
+
+  return function handleIncomingAction(action: Action): void {
+    switch (action.type) {
+      case ProtocolAnalysis.OPEN_PYTHON_DIRECTORY: {
+        const dir = getConfig().python.pathToPythonOverride
+        openDirectoryInFileExplorer(dir).catch(err => {
+          log.debug('Error opening python directory', err.message)
+        })
+        break
+      }
+      case ProtocolAnalysis.CHANGE_PYTHON_PATH_OVERRIDE: {
+        showOpenDirectoryDialog(mainWindow)
+          .then(filePaths => {
+            if (filePaths.length > 0) {
+              const nextValue = filePaths[0]
+              dispatch(
+                Cfg.updateConfigValue(
+                  CONFIG_PYTHON_PATH_TO_PYTHON_OVERRIDE,
+                  nextValue
+                )
+              )
+            }
+          })
+          .catch(err => {
+            log.debug('Error changing python path override', err.message)
+          })
+        break
+      }
+    }
+  }
 }
 
 export function analyzeProtocolSource(

app/src/pages/AppSettings/AdvancedSettings.tsx

@@ -1,6 +1,5 @@
 import * as React from 'react'
 import { useTranslation } from 'react-i18next'
-import path from 'path'
 import { useSelector, useDispatch } from 'react-redux'
 import { css } from 'styled-components'
 
@@ -27,6 +26,7 @@ import {
 } from '@opentrons/components'
 
 import * as Config from '../../redux/config'
+import * as ProtocolAnalysis from '../../redux/protocol-analysis'
 import * as Calibration from '../../redux/calibration'
 import * as CustomLabware from '../../redux/custom-labware'
 import {
@@ -60,19 +60,13 @@ const ALWAYS_TRASH: 'always-trash' = 'always-trash'
 const ALWAYS_PROMPT: 'always-prompt' = 'always-prompt'
 const REALTEK_URL = 'https://www.realtek.com/en/'
 
-const INPUT_STYLES = css`
-  position: fixed;
-  clip: rect(1px 1px 1px 1px);
-`
-
 type BlockSelection =
   | typeof ALWAYS_BLOCK
   | typeof ALWAYS_TRASH
   | typeof ALWAYS_PROMPT
 
 export function AdvancedSettings(): JSX.Element {
   const { t } = useTranslation(['app_settings', 'shared'])
-  const pythonDirectoryFileInput = React.useRef<HTMLInputElement>(null)
   const useTrashSurfaceForTipCal = useSelector((state: State) =>
     Config.getUseTrashSurfaceForTipCal(state)
   )
@@ -157,30 +151,14 @@ export function AdvancedSettings(): JSX.Element {
       )
     )
 
-  //  TODO(jr, 5/6/22): find another way to get webkitdirectory to work in the input DOM https://github.com/facebook/react/issues/3468
-  React.useEffect(() => {
-    if (pythonDirectoryFileInput.current !== null) {
-      pythonDirectoryFileInput.current.setAttribute('directory', 'true')
-      pythonDirectoryFileInput.current.setAttribute('webkitdirectory', 'true')
-    }
-  }, [pythonDirectoryFileInput])
-
   const handleClickPythonDirectoryChange: React.MouseEventHandler<HTMLButtonElement> = _event => {
-    pythonDirectoryFileInput.current?.click()
+    dispatch(ProtocolAnalysis.changePythonPathOverrideConfig())
     trackEvent({
       name: 'changePathToPythonDirectory',
       properties: {},
     })
   }
 
-  const setPythonInterpreterDirectory: React.ChangeEventHandler<HTMLInputElement> = event => {
-    const { files = [] } = event.target ?? {}
-    const dirName =
-      files?.[0]?.path != null ? path.dirname(files?.[0]?.path) : null
-    dispatch(Config.updateConfigValue('python.pathToPythonOverride', dirName))
-    event.target.value = ''
-  }
-
   const toggleDevtools = (): unknown => dispatch(Config.toggleDevtools())
   const handleChannel: React.ChangeEventHandler<HTMLSelectElement> = event =>
     dispatch(Config.updateConfigValue('update.channel', event.target.value))
@@ -558,7 +536,7 @@ export function AdvancedSettings(): JSX.Element {
                 css={TYPOGRAPHY.pRegular}
                 color={COLORS.darkBlack}
                 onClick={() =>
-                  dispatch(Config.openPythonInterpreterDirectory())
+                  dispatch(ProtocolAnalysis.openPythonInterpreterDirectory())
                 }
                 id="AdvancedSettings_sourceFolderLinkPython"
               >
@@ -592,14 +570,6 @@ export function AdvancedSettings(): JSX.Element {
               {t('add_override_path')}
             </TertiaryButton>
           )}
-          <input
-            id="AdvancedSetting_pythonPathDirectoryInput"
-            data-testid="AdvancedSetting_pythonPathDirectoryInput"
-            type="file"
-            css={INPUT_STYLES}
-            ref={pythonDirectoryFileInput}
-            onChange={setPythonInterpreterDirectory}
-          />
         </Flex>
         <Divider marginY={SPACING.spacing5} />
         <Flex alignItems={ALIGN_CENTER} justifyContent={JUSTIFY_SPACE_BETWEEN}>