Skip to content
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

Provide "New File" default implementation (#13303) #13344

Merged

Conversation

dhuebner
Copy link
Member

@dhuebner dhuebner commented Feb 2, 2024

What it does

Fixes #13303

Adds a new dynamic Create New File item to the create file picker. The new item will appear as soon as the user types something in the input field. Accepting the item will execute a file.newFile command handler and bypass the file name to it.

How to test

  • In the browser app select New File...item from the File menu.
  • Start typing. A new item Create New File should become visible
  • Selecting this item will open a "save file" dialog with a file name pre-set. The parent directory should be the current user working directory or a workspace folder if selected.
Bildschirmfoto 2024-02-02 um 12 43 46

Follow-ups

Review checklist

Reminder for reviewers

@dhuebner dhuebner force-pushed the huebner/newFileCommand-13303 branch 3 times, most recently from 219e08e to 102c88c Compare February 16, 2024 12:18
@dhuebner dhuebner marked this pull request as ready for review February 16, 2024 12:30
@martin-fleck-at martin-fleck-at self-requested a review February 16, 2024 13:17
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Dennis, thank you very much for those changes, I think it definitely fills a gap in a very common functionality.

I added some comments but I also have a core question: Is there any particular reason we enforce this to be the same as the 'New File' command we have in the workspace? I feel like a separation may reflect the difference in functionality a little bit better:

  • The Workspace 'New File' command is the one that you can trigger from the File Navigator toolbar and the one where we can actually create files in a known root folder.
  • The Common 'New File' command is one that can be triggered from the command palette even if there is no open workspace and that will ask you where to save the file. Currently calling the New File command from the menu when no workspace is open will simply not show the option to create a file which is a shame cause basically all the pieces are already in place.

We can combine the code, of course, but I think aligning with VS Code might be better at the moment, i.e., always asking where to save the file and if we are in a workspace context and we do know about a selected URI we can propose this as the parent but we should always show the dialog. This ensures that the code always works properly.

I hope it is clear what I mean, if not, let me know and I'll try my best to explain better.

packages/workspace/src/browser/workspace-commands.ts Outdated Show resolved Hide resolved
packages/workspace/src/browser/workspace-commands.ts Outdated Show resolved Hide resolved
packages/workspace/src/browser/workspace-commands.ts Outdated Show resolved Hide resolved
@dhuebner
Copy link
Member Author

Thank you for your comments!

  • The Workspace 'New File' command is the one that you can trigger from the File Navigator toolbar and the one where we can actually create files in a known root folder.
  • The Common 'New File' command is one that can be triggered from the command palette even if there is no open workspace and that will ask you where to save the file. Currently calling the New File command from the menu when no workspace is open will simply not show the option to create a file which is a shame cause basically all the pieces are already in place.

Yes, I was thinking about this as well. In my PR Draft I tried to explain it in the PR description and offered some option how to implement it. So I'm really glad to have somebody to discuss it now!

In fact the 'New File...' command in vs-code is the one from the welcome page and just opens a system 'Save' dialog. This behavior is really confusing when you already have a workspace open IMHO. I also think that a lot of people will tend to use the menu entry to create a new workspace file, especially in case where the Explorer view is closed or hidden. See also the user expectation here.

I think the best would be:

  • when we don't have a workspace open - open a "Save" system dialog (just like vs-code does)
  • if we do have workspace open - do what the PR currently does

This leads me to some other questions:

  1. Should we keep the "New File..." menu in the core package, or move it to workspace package? (Currently you can not save the new created untitled files when in browser mode)
  2. How to open a new "Save" system dialog? :)

FDYT?

@martin-fleck-at
Copy link
Contributor

Yes, I was thinking about this as well. In my PR Draft I tried to explain it in the PR description and offered some option how to implement it. So I'm really glad to have somebody to discuss it now!

In fact the 'New File...' command in vs-code is the one from the welcome page and just opens a system 'Save' dialog. This behavior is really confusing when you already have a workspace open IMHO. I also think that a lot of people will tend to use the menu entry to create a new workspace file, especially in case where the Explorer view is closed or hidden. See also the user expectation here.

I think the best would be:

  • when we don't have a workspace open - open a "Save" system dialog (just like vs-code does)
  • if we do have workspace open - do what the PR currently does

This leads me to some other questions:

  1. Should we keep the "New File..." menu in the core package, or move it to workspace package? (Currently you can not save the new created untitled files when in browser mode)
  2. How to open a new "Save" system dialog? :)

FDYT?

So in VS Code it seems they not only trigger it from the Welcome Page but they also have a 'New File...' menu item in the 'File' menu, exactly the same as we do. And that item is available no matter whether you have a workspace open or not (i.e., just having a new empty window) by always opening a save dialog after the user provided some input. So I believe we should at least take that as a baseline of things that need to work. You can open a save dialog through the FileDialogService.showSaveDialog which is implemented for both the browser and the Electron context. I believe you can even provide the input the user has given in the quickbox as suggested default name.

I believe most of the "confusion" comes from the fact that we are mixing two different actions under the same name:

  • A common NEW_FILE command that creates a new file independent from any workspace. That is currently used in the File menu of VS Code and Theia and basically provides a list of quick inputs through the CommonMenus.FILE_NEW_CONTRIBUTIONS in Theia. Extensions may add additional mechanisms, e.g., the Python extension. When we look at how they do it (even if a workspace is closed and even in the browser context), we can see that there is definitely no assumption of any kind of workspace but simply providing a save dialog.

  • A workspace command that can create a new file based on the workspace context (usually provided through the toolbar in the File Explorer or the context menu of the file explorer). Here we can take the shorter way.

I believe we should not mix them and even if we were to have the information in case 1, since other contributions may not utilize that, I believe we should simply align with VS Code for now. However, that is just my opinion and others may disagree so if you want to consider the context (in case we have a workspace context), we can definitely involve other people in this discussion.

@dhuebner dhuebner force-pushed the huebner/newFileCommand-13303 branch from 102c88c to 1641649 Compare February 22, 2024 10:44
@dhuebner dhuebner force-pushed the huebner/newFileCommand-13303 branch 2 times, most recently from f5afa73 to a35b9fc Compare March 1, 2024 12:30
@dhuebner
Copy link
Member Author

dhuebner commented Mar 1, 2024

@martin-fleck-at

I believe we should simply align with VS Code for now.

It is also my opinion. I changed the implementation and hope now all the use cases work the same way as the do in vs-code. I also added New File... widget to the Welcome page.

Bildschirmfoto 2024-03-01 um 13 38 24

@dhuebner dhuebner requested a review from martin-fleck-at March 1, 2024 12:42
@dhuebner
Copy link
Member Author

dhuebner commented Mar 1, 2024

The test ../../src/tests/theia-main-menu.test.ts:95:9 › Theia Main Menu › open about dialog using menu is failing in GH actions, but works fine locally. Is it flaky?

@dhuebner dhuebner force-pushed the huebner/newFileCommand-13303 branch 3 times, most recently from ab68746 to 53ae257 Compare March 1, 2024 15:34
@planger
Copy link
Contributor

planger commented Mar 7, 2024

The test ../../src/tests/theia-main-menu.test.ts:95:9 › Theia Main Menu › open about dialog using menu is failing in GH actions, but works fine locally. Is it flaky?

Not that I'm aware of. I'm monitoring the playwright tests and didn't see this one in particular failing before. There is however a certain amount of flakiness in our Playwright tests in general (< 1 % though). So I'll add this one to my list of "suspects" and observe whether I see this failing again.

@martin-fleck-at
Copy link
Contributor

I'll have another look at this today, thank you for your patience.

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhuebner I really, really like the new set of changes! Now everything is very clear cut and the usability is also fantastic, so thank you very much!

I do have some minor comments on the code which you may want to address but the functionality is definitely there.

@JonasHelming
Copy link
Contributor

@dhuebner Do you plan to adress the comments? Would be great to have this in the upcoming release.

@dhuebner
Copy link
Member Author

@JonasHelming
Yes, sure. I was sick last week, will apply the suggestions this week.

@dhuebner dhuebner force-pushed the huebner/newFileCommand-13303 branch from 5ce56f1 to add38f4 Compare March 19, 2024 15:40
@dhuebner
Copy link
Member Author

@martin-fleck-at
I've applied all the suggested changes, please have a look.

@martin-fleck-at
Copy link
Contributor

@dhuebner Thank you for the update, I'll have a look at this tomorrow!

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhuebner Fantastic, thank you very much for the great contribution! Everything looks good to me now!

@martin-fleck-at martin-fleck-at merged commit cf63b20 into eclipse-theia:master Mar 21, 2024
14 checks passed
@dhuebner dhuebner deleted the huebner/newFileCommand-13303 branch March 22, 2024 07:04
@jfaltermeier jfaltermeier added this to the 1.48.0 milestone Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

New File doesn't accept a filename
5 participants