From 1170b9fc78365fa672b659233584925fcc6f7456 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Tue, 23 Mar 2021 09:39:18 +0000 Subject: [PATCH] Collect a filename for create file. The input dialog is currently really a filename dialog, but there's plenty of scope to generalise it to take a form component which gets its state passed in, validated, then returned. Add an e2e test for file creation. https://github.com/microbit-foundation/python-editor-next/issues/21 --- src/common/InputDialog.tsx | 102 ++++++++++++++++++++++++++++++ src/common/use-dialogs.tsx | 36 ++++++++++- src/e2e/app.ts | 23 +++++++ src/e2e/multiple-files.test.ts | 9 +++ src/project/project-actions.tsx | 39 ++++++++---- src/project/project-utils.test.ts | 31 +++++++++ src/project/project-utils.ts | 33 +++++++++- 7 files changed, 255 insertions(+), 18 deletions(-) create mode 100644 src/common/InputDialog.tsx create mode 100644 src/project/project-utils.test.ts diff --git a/src/common/InputDialog.tsx b/src/common/InputDialog.tsx new file mode 100644 index 000000000..1541fbb33 --- /dev/null +++ b/src/common/InputDialog.tsx @@ -0,0 +1,102 @@ +import { Button } from "@chakra-ui/button"; +import { + FormControl, + FormErrorMessage, + FormHelperText, + FormLabel, +} from "@chakra-ui/form-control"; +import { Input } from "@chakra-ui/input"; +import { VStack } from "@chakra-ui/layout"; +import { + Modal, + ModalBody, + ModalContent, + ModalFooter, + ModalHeader, + ModalOverlay, +} from "@chakra-ui/modal"; +import { ReactNode, useRef, useState } from "react"; + +export interface InputDialogParameters { + header: ReactNode; + body: ReactNode; + actionLabel: string; + validate: (input: string) => string | undefined; +} + +export interface InputDialogParametersWithActions + extends InputDialogParameters { + header: ReactNode; + body: ReactNode; + actionLabel: string; + onConfirm: (validValue: string) => void; + onCancel: () => void; +} + +export interface InputDialogProps extends InputDialogParametersWithActions { + isOpen: boolean; +} + +/** + * File name input dialog. + * + * Generally not used directly. Prefer the useDialogs hook. + */ +export const InputDialog = ({ + header, + body, + actionLabel, + isOpen, + validate, + onConfirm, + onCancel, +}: InputDialogProps) => { + const [value, setValue] = useState(""); + const [error, setError] = useState(undefined); + const leastDestructiveRef = useRef(null); + return ( + + + + + {header} + + + + {body} + + Name + { + const value = e.target.value; + setValue(value); + setError(validate(value)); + }} + > + + We'll add the ".py" extension for you. + + {error} + + + + + + + + + + + ); +}; diff --git a/src/common/use-dialogs.tsx b/src/common/use-dialogs.tsx index 41ee9244e..9d06ce367 100644 --- a/src/common/use-dialogs.tsx +++ b/src/common/use-dialogs.tsx @@ -4,6 +4,11 @@ import { ConfirmDialogParametersWithActions, ConfirmDialog, } from "./ConfirmDialog"; +import { + InputDialog, + InputDialogParameters, + InputDialogParametersWithActions, +} from "./InputDialog"; const DialogContext = React.createContext(undefined); @@ -12,14 +17,22 @@ interface DialogProviderProps { } export const DialogProvider = ({ children }: DialogProviderProps) => { - const [state, setState] = useState< + const [confirmDialogState, setConfirmDialogState] = useState< ConfirmDialogParametersWithActions | undefined >(undefined); - const dialogs = useMemo(() => new Dialogs(setState), [setState]); + const [inputDialogState, setInputDialogState] = useState< + InputDialogParametersWithActions | undefined + >(undefined); + + const dialogs = useMemo( + () => new Dialogs(setConfirmDialogState, setInputDialogState), + [setConfirmDialogState] + ); return ( <> - {state && } + {confirmDialogState && } + {inputDialogState && } {children} @@ -30,6 +43,9 @@ export class Dialogs { constructor( private confirmDialogSetState: ( options: ConfirmDialogParametersWithActions | undefined + ) => void, + private inputDialogSetState: ( + options: InputDialogParametersWithActions | undefined ) => void ) {} @@ -46,6 +62,20 @@ export class Dialogs { }); }); } + + async input(options: InputDialogParameters): Promise { + return new Promise((_resolve) => { + const resolve = (result: string | undefined) => { + this.inputDialogSetState(undefined); + _resolve(result); + }; + this.inputDialogSetState({ + ...options, + onCancel: () => resolve(undefined), + onConfirm: (validValue: string) => resolve(validValue), + }); + }); + } } export const useDialogs = () => { diff --git a/src/e2e/app.ts b/src/e2e/app.ts index 02dbf5d86..bff11bb02 100644 --- a/src/e2e/app.ts +++ b/src/e2e/app.ts @@ -5,6 +5,7 @@ import * as os from "os"; import * as path from "path"; import "pptr-testing-library/extend"; import puppeteer, { ElementHandle, Page } from "puppeteer"; +import { createBinary } from "typescript"; export interface BrowserDownload { filename: string; @@ -52,6 +53,28 @@ export class App { await openInput.uploadFile(filePath); } + /** + * Create a new file using the files tab. + * + * @param name The name to enter in the dialog. + */ + async createNewFile(name: string): Promise { + await this.selectSideBar("Files"); + const document = await this.document(); + const newButton = await document.findByRole("button", { + name: "Create new file", + }); + await newButton.click(); + const nameField = await document.findByRole("textbox", { + name: "Name", + }); + await nameField.type(name); + const createButton = await document.findByRole("button", { + name: "Create", + }); + await createButton.click(); + } + /** * Upload a file to the file system using the file chooser. * diff --git a/src/e2e/multiple-files.test.ts b/src/e2e/multiple-files.test.ts index 7767a5937..87d3d9636 100644 --- a/src/e2e/multiple-files.test.ts +++ b/src/e2e/multiple-files.test.ts @@ -16,6 +16,15 @@ describe("Browser - multiple and missing file cases", () => { ); }); + it("Create a new file", async () => { + await app.createNewFile("test"); + + // This should happen automatically but is not yet implemented. + await app.switchToEditing("test.py"); + + await app.findVisibleEditorContents(/Your new file/); + }); + it("Prevents deleting main.py", async () => { expect(await app.canDeleteFile("main.py")).toEqual(false); }); diff --git a/src/project/project-actions.tsx b/src/project/project-actions.tsx index e13cfbac2..e92b80a25 100644 --- a/src/project/project-actions.tsx +++ b/src/project/project-actions.tsx @@ -1,6 +1,7 @@ import { saveAs } from "file-saver"; import Separate, { br } from "../common/Separate"; import { ActionFeedback } from "../common/use-action-feedback"; +import { Dialogs } from "../common/use-dialogs"; import { BoardId } from "../device/board-id"; import { ConnectionStatus, @@ -17,7 +18,7 @@ import { import { VersionAction } from "../fs/storage"; import { Logging } from "../logging/logging"; import translation from "../translation"; -import { Dialogs } from "../common/use-dialogs"; +import { ensurePythonExtension, validateNewFilename } from "./project-utils"; class HexGenerationError extends Error {} @@ -268,18 +269,30 @@ export class ProjectActions { action: "create-file", }); - const filename = "new file.py"; - try { - await this.fs.write( - filename, - "# Your new file!", - VersionAction.INCREMENT - ); - this.actionFeedback.success({ - title: `Created ${filename}`, - }); - } catch (e) { - this.actionFeedback.unexpectedError(e); + const preexistingFiles = new Set(this.fs.project.files.map((f) => f.name)); + const validate = (filename: string) => + validateNewFilename(filename, (f) => preexistingFiles.has(f)); + const filenameWithoutExtension = await this.dialogs.input({ + header: "Create a new Python file", + body: null, + actionLabel: "Create", + validate, + }); + + if (filenameWithoutExtension) { + try { + const filename = ensurePythonExtension(filenameWithoutExtension); + await this.fs.write( + filename, + "# Your new file!", + VersionAction.INCREMENT + ); + this.actionFeedback.success({ + title: `Created ${filename}`, + }); + } catch (e) { + this.actionFeedback.unexpectedError(e); + } } }; /** diff --git a/src/project/project-utils.test.ts b/src/project/project-utils.test.ts new file mode 100644 index 000000000..345880ec5 --- /dev/null +++ b/src/project/project-utils.test.ts @@ -0,0 +1,31 @@ +import { validateNewFilename } from "./project-utils"; + +describe("validateNewFilename", () => { + const exists = (filename: string) => filename === "main.py"; + + it("errors for Python extensions", () => { + expect(validateNewFilename("foo.py", exists)).toEqual( + "Python files should have lowercase names with no spaces" + ); + }); + it("errors for spaces", () => { + expect(validateNewFilename("spaces are not allowed", exists)).toEqual( + "Python files should have lowercase names with no spaces" + ); + }); + it("errors for uppercase", () => { + expect(validateNewFilename("OHNO", exists)).toEqual( + "Python files should have lowercase names with no spaces" + ); + }); + it("errors for file clashes", () => { + expect(validateNewFilename("main", exists)).toEqual( + "This file already exists" + ); + }); + it("accepts valid names", () => { + expect( + validateNewFilename("underscores_are_allowed", exists) + ).toBeUndefined(); + }); +}); diff --git a/src/project/project-utils.ts b/src/project/project-utils.ts index 705964f38..4af0de550 100644 --- a/src/project/project-utils.ts +++ b/src/project/project-utils.ts @@ -1,2 +1,31 @@ -export const isEditableFile = (filename: string) => - filename.match(/\.[Pp][Yy]$/); +export const isPythonFile = (filename: string) => filename.match(/\.[Pp][Yy]$/); + +export const ensurePythonExtension = (filename: string) => + isPythonFile(filename) ? filename : `${filename}.py`; + +export const isEditableFile = isPythonFile; + +/** + * From https://www.python.org/dev/peps/pep-0008/#package-and-module-names + * + * "Modules should have short, all-lowercase names. + * Underscores can be used in the module name if it improves readability." + * + * @param filename The name to check. May be user input without a file extension. + * @param exists A function to check whether a file exists. + */ +export const validateNewFilename = ( + filename: string, + exists: (f: string) => boolean +): string | undefined => { + if (filename.length === 0) { + return "The name cannot be empty"; + } + if (!filename.match(/^[\p{Ll}_]+$/u)) { + return "Python files should have lowercase names with no spaces"; + } + if (exists(ensurePythonExtension(filename))) { + return "This file already exists"; + } + return undefined; +};