Use builtin vscode.open command in tracked explorer tree (#807)

* use builtin command to open files instead of hand rolled

* wrap open file command

* pull out test utils

* annotate existing test

extension/package.json

@@ -281,12 +281,6 @@
           "type": "string",
           "default": null
         },
-        "dvc.views.trackedExplorerTree.noOpenUnsupported": {
-          "title": "%config.noOpenUnsupported.title%",
-          "description": "%config.noOpenUnsupported.description%",
-          "type": "boolean",
-          "default": false
-        },
         "dvc.views.trackedExplorerTree.noPromptPullMissing": {
           "title": "%config.noPromptPullMissing.title%",
           "description": "%config.noPromptPullMissing.description%",

extension/src/fileSystem/tree.ts

@@ -25,6 +25,7 @@ import { RegisteredCliCommands, RegisteredCommands } from '../commands/external'
 import { sendViewOpenedTelemetryEvent } from '../telemetry'
 import { EventName } from '../telemetry/constants'
 import { getInput } from '../vscode/inputBox'
+import { openFile } from '../vscode/commands'
 
 export class TrackedExplorerTree implements TreeDataProvider<string> {
   public dispose = Disposable.fn()
@@ -42,9 +43,6 @@ export class TrackedExplorerTree implements TreeDataProvider<string> {
 
   private doNotShowAgainText = "Don't Show Again"
 
-  private noOpenUnsupportedOption =
-    'dvc.views.trackedExplorerTree.noOpenUnsupported'
-
   private noPromptPullMissingOption =
     'dvc.views.trackedExplorerTree.noPromptPullMissing'
 
@@ -118,20 +116,6 @@ export class TrackedExplorerTree implements TreeDataProvider<string> {
     return treeItem
   }
 
-  private handleOpenUnsupportedError = async (relPath: string) => {
-    if (getConfigValue(this.noOpenUnsupportedOption)) {
-      return
-    }
-    const response = await window.showInformationMessage(
-      `Cannot open ${relPath}. File is unsupported and cannot be opened as text.`,
-      this.doNotShowAgainText
-    )
-
-    if (response) {
-      return setConfigValue(this.noOpenUnsupportedOption, true)
-    }
-  }
-
   private openPullPrompt = async (path: string) => {
     if (getConfigValue(this.noPromptPullMissingOption)) {
       return
@@ -176,26 +160,12 @@ export class TrackedExplorerTree implements TreeDataProvider<string> {
 
   private openResource = (resource: Uri) => {
     const path = resource.fsPath
-    const dvcRoot = this.pathRoots[path]
-    const relPath = relative(dvcRoot, path)
 
     if (!exists(path)) {
       return this.openPullPrompt(path)
     }
 
-    return window.showTextDocument(resource).then(
-      textEditor => textEditor,
-      error => {
-        if (
-          error.message.includes(
-            'File seems to be binary and cannot be opened as text'
-          )
-        ) {
-          return this.handleOpenUnsupportedError(relPath)
-        }
-        return window.showInformationMessage(error.message)
-      }
-    )
+    return openFile(resource)
   }
 
   private getDataPlaceholder(path: string): string {

extension/src/test/suite/fileSystem/tree.test.ts

@@ -3,21 +3,24 @@ import { afterEach, beforeEach, describe, it, suite } from 'mocha'
 import { expect } from 'chai'
 import { stub, restore } from 'sinon'
 import { ensureFileSync } from 'fs-extra'
-import { window, commands, Uri, TextEditor, MessageItem } from 'vscode'
+import { window, commands, Uri, MessageItem, ViewColumn } from 'vscode'
 import { Disposable } from '../../../extension'
 import { exists } from '../../../fileSystem'
 import * as Workspace from '../../../fileSystem/workspace'
-import * as FileSystem from '../../../fileSystem'
-import { getConfigValue, setConfigValue } from '../../../vscode/config'
 import { CliExecutor } from '../../../cli/executor'
 import { Prompt } from '../../../cli/output'
 import * as WorkspaceFolders from '../../../vscode/workspaceFolders'
 import * as Setup from '../../../setup'
-import { dvcDemoPath } from '../util'
+import {
+  activeTextEditorChangedEvent,
+  dvcDemoPath,
+  getActiveTextEditorFilename
+} from '../util'
 import {
   RegisteredCliCommands,
   RegisteredCommands
 } from '../../../commands/external'
+import { setConfigValue } from '../../../vscode/config'
 
 suite('Tracked Explorer Tree Test Suite', () => {
   window.showInformationMessage('Start all tracked explorer tree tests.')
@@ -26,19 +29,16 @@ suite('Tracked Explorer Tree Test Suite', () => {
 
   const disposable = Disposable.fn()
 
-  const noOpenUnsupportedOption =
-    'dvc.views.trackedExplorerTree.noOpenUnsupported'
-  const noPromptPullMissingOption =
-    'dvc.views.trackedExplorerTree.noPromptPullMissing'
-
   beforeEach(() => {
     restore()
   })
 
   afterEach(() => {
     disposable.dispose()
-    setConfigValue(noOpenUnsupportedOption, undefined)
-    setConfigValue(noPromptPullMissingOption, undefined)
+    setConfigValue(
+      'dvc.views.trackedExplorerTree.noPromptPullMissing',
+      undefined
+    )
     return commands.executeCommand('workbench.action.closeAllEditors')
   })
 
@@ -97,90 +97,37 @@ suite('Tracked Explorer Tree Test Suite', () => {
       expect(mockSetup).not.to.be.called
     })
 
-    it('should be able to open a non-binary file', async () => {
-      const relPath = join('logs', 'acc.tsv')
-      const absPath = join(dvcDemoPath, relPath)
-      stub(path, 'relative').returns(relPath)
-      stub(FileSystem, 'exists').returns(true)
-
-      const uri = Uri.file(absPath)
+    it('should be able to open a file', async () => {
+      expect(getActiveTextEditorFilename()).not.to.equal(__filename)
+      const uri = Uri.file(__filename)
 
-      const mockShowTextDocument = stub(window, 'showTextDocument').resolves({
-        document: { fileName: absPath }
-      } as unknown as TextEditor)
+      const activeEditorChanged = activeTextEditorChangedEvent()
 
-      const textEditor = (await commands.executeCommand(
+      await commands.executeCommand(
         RegisteredCommands.TRACKED_EXPLORER_OPEN_FILE,
         uri
-      )) as TextEditor
+      )
+      await activeEditorChanged
 
-      expect(textEditor.document.fileName).to.equal(absPath)
-      expect(mockShowTextDocument).to.be.calledWith(uri)
+      expect(getActiveTextEditorFilename()).to.equal(__filename)
     })
 
     it('should be able to open a file to the side', async () => {
-      expect(window.activeTextEditor?.document.fileName).not.to.equal(
-        __filename
-      )
+      expect(getActiveTextEditorFilename()).not.to.equal(__filename)
+
+      const activeEditorChanged = activeTextEditorChangedEvent()
 
-      const activeEditorChanged = new Promise(resolve =>
-        window.onDidChangeActiveTextEditor(editor => resolve(editor))
-      )
       await commands.executeCommand(
         RegisteredCommands.TRACKED_EXPLORER_OPEN_TO_THE_SIDE,
         __filename
       )
       await activeEditorChanged
 
-      expect(window.activeTextEditor?.document.fileName).to.equal(__filename)
+      expect(getActiveTextEditorFilename()).to.equal(__filename)
+      expect(window.activeTextEditor?.viewColumn).not.to.equal(ViewColumn.One)
     })
 
-    it('should only call showInformationMessage when trying to open a binary file without the no binary errors option set', async () => {
-      const relPath = 'model.pt'
-      const absPath = join(dvcDemoPath, relPath)
-      const uri = Uri.file(absPath)
-      stub(path, 'relative').returns(relPath)
-      stub(FileSystem, 'exists').returns(true)
-      stub(window, 'showTextDocument').rejects(
-        new Error('File seems to be binary and cannot be opened as text')
-      )
-
-      const mockShowInformationMessage = stub(window, 'showInformationMessage')
-
-      expect(!!getConfigValue(noOpenUnsupportedOption)).to.be.false
-      mockShowInformationMessage.resolves(undefined)
-
-      await commands.executeCommand(
-        RegisteredCommands.TRACKED_EXPLORER_OPEN_FILE,
-        uri
-      )
-
-      expect(mockShowInformationMessage).to.be.calledOnce
-      expect(!!getConfigValue(noOpenUnsupportedOption)).to.be.false
-      mockShowInformationMessage.resetHistory()
-      mockShowInformationMessage.resolves(
-        "Don't Show Again" as unknown as MessageItem
-      )
-
-      await commands.executeCommand(
-        RegisteredCommands.TRACKED_EXPLORER_OPEN_FILE,
-        uri
-      )
-
-      expect(mockShowInformationMessage).to.be.calledOnce
-      expect(getConfigValue(noOpenUnsupportedOption)).to.be.true
-      mockShowInformationMessage.resetHistory()
-
-      await commands.executeCommand(
-        RegisteredCommands.TRACKED_EXPLORER_OPEN_FILE,
-        uri
-      )
-
-      expect(mockShowInformationMessage).not.to.be.called
-      expect(getConfigValue(noOpenUnsupportedOption)).to.be.true
-    })
-
-    it('should be able to pull a file after trying to open it and it does not exist on disk and the no missing errors option is unset', async () => {
+    it('should be able to pull a file after trying to open it when it does not exist on disk', async () => {
       const missingFile = 'non-existent.txt'
       const absPath = join(dvcDemoPath, missingFile)
       const uri = Uri.file(absPath)
@@ -198,8 +145,12 @@ suite('Tracked Explorer Tree Test Suite', () => {
         uri
       )
 
-      expect(mockShowInformationMessage).to.be.calledOnce
-      expect(mockPull).not.to.be.called
+      expect(
+        mockShowInformationMessage,
+        'should show the user an information prompt'
+      ).to.be.calledOnce
+      expect(mockPull, 'should not call pull if the prompt is dismissed').not.to
+        .be.called
 
       mockShowInformationMessage.resetHistory()
       mockShowInformationMessage.resolves('Pull File' as unknown as MessageItem)
@@ -209,8 +160,11 @@ suite('Tracked Explorer Tree Test Suite', () => {
         uri
       )
 
-      expect(mockShowInformationMessage).to.be.calledOnce
-      expect(mockPull).to.be.calledOnce
+      expect(
+        mockShowInformationMessage,
+        'should show the user an information prompt'
+      ).to.be.calledOnce
+      expect(mockPull, 'should pull the file if prompted').to.be.calledOnce
 
       mockPull.resetHistory()
       mockShowInformationMessage.resetHistory()
@@ -223,8 +177,14 @@ suite('Tracked Explorer Tree Test Suite', () => {
         uri
       )
 
-      expect(mockShowInformationMessage).to.be.calledOnce
-      expect(mockPull).not.to.be.called
+      expect(
+        mockShowInformationMessage,
+        'should show the user an information prompt'
+      ).to.be.calledOnce
+      expect(
+        mockPull,
+        'should not pull the file if the user chooses do not show again'
+      ).not.to.be.called
 
       mockShowInformationMessage.resetHistory()
 
@@ -233,8 +193,11 @@ suite('Tracked Explorer Tree Test Suite', () => {
         uri
       )
 
-      expect(mockShowInformationMessage).not.to.be.called
-      expect(mockPull).not.to.be.called
+      expect(
+        mockShowInformationMessage,
+        'should not show the information prompt if the appropriate config option is set'
+      ).not.to.be.called
+      expect(mockPull, 'should not pull the file').not.to.be.called
     })
 
     it('should be able to run dvc.removeTarget without error', async () => {

extension/src/test/suite/util.ts

@@ -1,6 +1,13 @@
 import { resolve } from 'path'
 import { SinonSpy, SinonStub } from 'sinon'
-import { commands, ConfigurationChangeEvent, Uri, workspace } from 'vscode'
+import {
+  commands,
+  ConfigurationChangeEvent,
+  TextEditor,
+  Uri,
+  window,
+  workspace
+} from 'vscode'
 import { ExperimentsRepository } from '../../experiments/repository'
 import { Disposable, Disposer } from '../../extension'
 
@@ -60,3 +67,13 @@ export const experimentsUpdatedEvent = (
 
 export const getFirstArgOfCall = (spy: SinonSpy, call: number) =>
   spy.getCall(call).args[0]
+
+export const activeTextEditorChangedEvent = (): Promise<
+  TextEditor | undefined
+> =>
+  new Promise(resolve =>
+    window.onDidChangeActiveTextEditor(editor => resolve(editor))
+  )
+
+export const getActiveTextEditorFilename = (): string | undefined =>
+  window.activeTextEditor?.document.fileName

extension/src/vscode/commands.ts

@@ -23,3 +23,6 @@ export const reRegisterVsCodeCommands = (
     path => executeCommand('explorer.openToSide', path)
   )
 }
+
+export const openFile = (uri: Uri) =>
+  commands.executeCommand('vscode.open', uri)