-
Notifications
You must be signed in to change notification settings - Fork 294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix export to use the source file for the directory it wants to open #4072
Changes from all commits
8f685e9
12abc5a
591f70b
7c8ae7b
f687cff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix the directory for exporting from the interactive window and notebooks to match the directory where the original file was created. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import { inject, injectable } from 'inversify'; | ||
import * as path from 'path'; | ||
import { SaveDialogOptions, Uri } from 'vscode'; | ||
import { IApplicationShell, IWorkspaceService } from '../../common/application/types'; | ||
import * as localize from '../../common/utils/localize'; | ||
import { computeWorkingDirectory } from '../jupyter/jupyterUtils'; | ||
import { ExportFormat, IExportDialog } from './types'; | ||
|
||
// File extensions for each export method | ||
export const PDFExtensions = { PDF: ['pdf'] }; | ||
export const HTMLExtensions = { HTML: ['html', 'htm'] }; | ||
export const PythonExtensions = { Python: ['py'] }; | ||
|
||
@injectable() | ||
export class ExportDialog implements IExportDialog { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file used to be called ExportManagerFilePicker. I thought this name made more sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bleh, github didn't pick up the rename though? Can't easily tell what changed... |
||
constructor( | ||
@inject(IApplicationShell) private readonly applicationShell: IApplicationShell, | ||
@inject(IWorkspaceService) private workspaceService: IWorkspaceService | ||
) {} | ||
|
||
public async showDialog( | ||
format: ExportFormat, | ||
source: Uri | undefined, | ||
defaultFileName?: string | ||
): Promise<Uri | undefined> { | ||
// map each export method to a set of file extensions | ||
let fileExtensions: { [name: string]: string[] } = {}; | ||
let extension: string | undefined; | ||
switch (format) { | ||
case ExportFormat.python: | ||
fileExtensions = PythonExtensions; | ||
extension = '.py'; | ||
break; | ||
|
||
case ExportFormat.pdf: | ||
extension = '.pdf'; | ||
fileExtensions = PDFExtensions; | ||
break; | ||
|
||
case ExportFormat.html: | ||
extension = '.html'; | ||
fileExtensions = HTMLExtensions; | ||
break; | ||
|
||
case ExportFormat.ipynb: | ||
extension = '.ipynb'; | ||
const filtersKey = localize.DataScience.exportDialogFilter(); | ||
fileExtensions[filtersKey] = ['ipynb']; | ||
break; | ||
|
||
default: | ||
return; | ||
} | ||
|
||
const targetFileName = | ||
defaultFileName || !source | ||
? defaultFileName || '' | ||
: `${path.basename(source.fsPath, path.extname(source.fsPath))}${extension}`; | ||
|
||
const options: SaveDialogOptions = { | ||
defaultUri: await this.getDefaultUri(source, targetFileName), | ||
saveLabel: localize.DataScience.exportButtonTitle(), | ||
filters: fileExtensions | ||
}; | ||
|
||
return this.applicationShell.showSaveDialog(options); | ||
} | ||
|
||
private async getDefaultUri(source: Uri | undefined, targetFileName: string): Promise<Uri> { | ||
if (!source || source.scheme === 'file' || source.scheme === 'untitled') { | ||
// Just combine the working directory with the file | ||
return Uri.file(path.join(await computeWorkingDirectory(source, this.workspaceService), targetFileName)); | ||
} | ||
|
||
// Otherwise split off the end of the path and combine it with the target file name | ||
const newPath = path.join(path.dirname(source.path), targetFileName); | ||
return Uri.parse(`${source.scheme}://${newPath}`); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check calling this Export command from the command palette with all our types of editor? I thought when I was looking at this recently there was a case where a Model was getting passed in. I think that it might have been Old editor active editor = Command is called with all undefined. CustomEditor active editor = Command is called with an INotebookModel. Not 100% sure I'm recalling correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this should work (I'll double check). This happens when you try to export a notebook from the command palette (instead of from our toolbar).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thing I double checked. Found another issue. Interactive window would show the export commands if a notebook had ever been opened. Since the active editor here is from the notebook, the export command would essentially do nothing.
The bug was in our tracking of active editor. We weren't firing the change of the active editor on close.