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

Added context menu command to create new worksheet #183

Merged
merged 7 commits into from
Feb 27, 2020

Conversation

alekseiAlefirov
Copy link
Collaborator

@alekseiAlefirov alekseiAlefirov commented Jan 22, 2020

Issue: #179
(PR for the server: scalameta/metals#1339)

From the explorer context menu:
new-worksheet-explorer

From Command Palette:
new-worksheet-command-palette

@tgodzik
Copy link
Contributor

tgodzik commented Jan 23, 2020

Looks cool! Thanks @alekseiAlefirov !

I am wondering if we should have a separate TAB with also new class/object etc.

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

This is very cool! I wonder if we can generalize this feature to more kinds of files than just worksheets 🤔 it would be nice to make it work more similarly to "New" in IntelliJ with options to create a Scala file, worksheets, maybe test file (automatically inferring the matching test target), etc

@alekseiAlefirov
Copy link
Collaborator Author

@olafurpg , can you please elaborate on test file, how it should work?
It most likely can be done, though it seems there's no such option for multi-level context menus in VSCode, so we can do like it's in IDEA. There an issue in VSCode project for that: microsoft/vscode#9827
-- nicely, some people want it for... creating new files from templates :)
We can reconstruct submenu functionality with quick-pick API of VSCode, probably, though.

@olafurpg
Copy link
Member

  • you have open file src/main/scala/foo/Foo.scala
  • Run "Create test file" command
  • it creates src/test/scala/foo/FooSuite.scala

@olafurpg
Copy link
Member

It's just an idea. It would be good to be able to at least handle a growing number of file kinds

@tgodzik
Copy link
Contributor

tgodzik commented Jan 27, 2020

I agree it would be really good to have a couple of possible file templates, but if it's not yet supported sensibly within vs code then I agree we can start with this one.

Although we could prepare a couple of commands within Metals itself.

@olafurpg
Copy link
Member

My concern is that the "Create Scala worksheet" command will demand too much attention and this approach isn't scalable to a large number of file kinds. I would prefer instead to expose this feature only as a command and a button in the Metals tree view (maybe under "build" explorer 🤔 )

@tgodzik
Copy link
Contributor

tgodzik commented Jan 27, 2020

My concern is that the "Create Scala worksheet" command will demand too much attention and this approach isn't scalable to a large number of file kinds. I would prefer instead to expose this feature only as a command and a button in the Metals tree view (maybe under "build" explorer)

I think we need to put it in the explorer - people are already super confused how to create that. I fear a button in the TreeView will too hidden and we will not achieve anything of the original purpose.

We can just have the basic file kinds for now: .worksheet.sc, .sc, .sbt, .scala. It's not perfect, but it's not that large of a menu yet.

@olafurpg
Copy link
Member

How about having a single "New Scala file" button that opens a quick open dialogue for the available options?

@olafurpg
Copy link
Member

I agree the button should be available in the context menu, it would not be discoverable in the tree view

@alekseiAlefirov
Copy link
Collaborator Author

alekseiAlefirov commented Jan 29, 2020

Context menu changed to support creating new scala class:
NewScalaClass
@olafurpg please check this out.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Overall, great work! I love the direction this is taking.

Comments here basically the same as in the server PR - we should reduce it all to a single level and we can jsut send an object via parameters.

src/extension.ts Outdated
return withName(name => {
client.sendRequest(ExecuteCommandRequest.type, {
command: "new-scala-class",
arguments: [dir, name, classKindPick.kind]
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to have an object as an argument here and we can reduce it all to a single pick. Worksheet/object/trait/class together.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@alekseiAlefirov
Copy link
Collaborator Author

All kinds of file (different classes along with worksheet) now in one menu:
new-scala-file

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@gabro would you be able to review the PR? I don't trust myself when it comes to typescript 😅

src/extension.ts Outdated
const worksheetPick = { label: "Worksheet", kind: "worksheet" };

return window
.showQuickPick(
Copy link
Member

Choose a reason for hiding this comment

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

Instead of implementing the quick pick here, I think it would be nice to add a new LSP extension so the Metals server can open a quick pick. We can use window/showMessageRequest as a fallback.

I have tried to follow the principle of avoiding too much custom logic in the vscode client and allowing the server to push behavior instead. The benefit of this strategy is that we 1) we can reuse more code to support multiple editors and 2) next time we need a quick pick we may not have to update the client code.

Copy link
Member

Choose a reason for hiding this comment

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

I have some comments about the implementation, but I too think we should move this to the server similarly to how we do for other extensions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@olafurpg , so this way part of the logic for the dialog about a new file (it's kind and name) is going to be transferred to the server and inside of the new-scala-file command execution. Dialog parts can be cancelled, so I have a question - how this should be handled by the command? Currently it returns a string/URI of the newly created file. Would using null in case of cancellation be alright for this? Or the request should finish with failure? Or instead of just string as the return type, we can use some Cancelable interface / generic container with it inside?

Copy link
Member

Choose a reason for hiding this comment

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

If the command “create new scala file” is sent to the server with a directory as the argument (or something like that) then I think it doesn’t matter what Metals returns. Metals already knows how to do the quick pick, do input box, apply workspace edit and also change the focused window (goto locations, it’s used in tree view protocol). Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

The directory argument can be optional, for example when triggered via the command palette. Metals already knows the focused document.

src/extension.ts Outdated
Comment on lines 511 to 513
// NOTE(aleksei): I would return workspace folder here, but not sure if there's API for that.
// Moving this funcitonality to server.
return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

moving stuff to the server makes sense, but I would change this note to something more "final".

Also, what API are you looking for? workspace.workspaceFolders[0] should give you what you want

src/protocol.ts Outdated
export interface MetalsNewScalaFileParams {
directory?: string;
name: string;
kind: string;
Copy link
Member

Choose a reason for hiding this comment

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

can this kind be more specific?

Suggested change
kind: string;
kind: 'class' | 'object' | 'trait' | 'package-object' | 'worksheet' ;

src/extension.ts Outdated
const worksheetPick = { label: "Worksheet", kind: "worksheet" };

return window
.showQuickPick(
Copy link
Member

Choose a reason for hiding this comment

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

I have some comments about the implementation, but I too think we should move this to the server similarly to how we do for other extensions.

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@alekseiAlefirov
Copy link
Collaborator Author

Thanks for the review, @tgodzik , @olafurpg , @gabro . I'll try to create "show quick pick" method in a PR to metals server and return with that soon.

@alekseiAlefirov
Copy link
Collaborator Author

metals/pickInput added, server side PR here: scalameta/metals#1447

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM! I think we can merge this as soon as we do the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants