Add new LSP extension 'metals/pickInput' to implement the "Create Scala file" command #1447

alekseiAlefirov · 2020-02-17T18:49:51Z

After new-scala-file command has been added to metals #1339 , client-side service was discussed scalameta/metals-vscode#183 , and it was decided to move and implemet quick-pick communication to server.

alekseiAlefirov · 2020-02-17T19:04:26Z

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

+      .orElse(focusedDocument.map(_.parent))
+
+    val newlyCreatedFile =
+      askForKind


I've made some kind of DSL here, to cover Future[Option[_]] handling (making this method more or readable). Future is for about remote requests, and Option reflect that the request can be cancelled (thus, nothing should be returned eventually).
Not sure, if it's good here.

alekseiAlefirov · 2020-02-17T19:06:49Z

tests/unit/src/main/scala/tests/TestingClient.scala

@@ -71,6 +71,7 @@ final class TestingClient(workspace: AbsolutePath, buffers: Buffers)
    _: ShowMessageRequestParams =>
      None
  }
+  var inputBoxHandler: MetalsInputBoxParams => Option[MetalsInputBoxResult] = _


while picking kind of file can be expressed via showMessageRequest, asking for name of the new file cannot, so I had to add this handler for testing purposes.

Can you add a default implementation returning some random name? You can have var nameToPick = "Foo" and replace that if needed. You also have None as the default if it's an issue.

Otherwise, we will get a null pointer in case of some particular changes.

alekseiAlefirov · 2020-02-17T19:11:54Z

metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala

@@ -106,6 +111,8 @@ object MetalsServerConfig {
          openFilesOnRenames = true,
          executeClientCommand = ExecuteClientCommandConfig.on,
          globSyntax = GlobSyntaxConfig.vscode,
+          isInputBoxEnabled = true,


I had to change it as well, because entering name for the new file cannot be expressed via showMessageRequest. This resulted into a bit changed UI when formatting and no scalafmt version is set.
Also, I would expect for such configs to be set by client, but okay, I mimicked it with isPickInputEnabled.

That should be in future enabled with the client capabilities, so after @ckipp01 's PR you can move it there.

I would even remove it from here, since we want to drop the properties.

ckipp01 · 2020-02-17T19:48:59Z

docs/editors/new-editor.md

@@ -134,6 +134,14 @@ Possible values:
  Metals tries to fallback to `window/showMessageRequest` when possible.
 - `on`: the `metals/inputBox` request is fully supported.

+### `-Dmetals.pick-input`


Related to some of the work I started in here: #1414, is it possible to not introduce this as a property, but rather as a clientExperimentalCapabilities? I feel like this would fit well there.

Agree, as I said in my previous comment, I would expect it to be a client config, rather then a server one. I've just repeated after input-box. If it's to be changed -- then this is to be changed as well :)

Let's use the recently merged way defining capabilities.

tgodzik

The PR is really good! I added quite a few comments, but I really like direction you've taken and mostly I just mentioned minor stuff.

tgodzik · 2020-02-18T10:19:25Z

metals/src/main/scala/scala/meta/internal/metals/ConfiguredLanguageClient.scala

@@ -117,4 +129,13 @@ final class ConfiguredLanguageClient(
    }
  }

+  private implicit def metalsPickInputParams2ShowMessageRequestInputParams(


Let's not use the implicit conversions here, they are not neccessarily good for readibility. And maybe just rename to toShowMessageRequestParams?

done, changed to

private def toShowMessageRequestParams( params: MetalsPickInputParams ): ShowMessageRequestParams

tgodzik · 2020-02-19T14:44:56Z

docs/editors/new-editor.md

+  /**
+   * An optional flag to include the description when filtering the picks.
+   */
+  matchOnDescription?: boolean;


Are the additional properties needed to match the ones in VSCode API?

good question to discuss.
I've added them to match, yes, to be able to use the most of VSCode API.
But if we don't want them - we can remove them, they'll be initialized to default values on VSCode side.

It should be fine, we can leave it as is. Unless they might cause issues for implementing this in other clients?

I think, it won't be no problem, thanks to JS agility. Even now it got implicitly converted from Metals interface to VSCode one.

tgodzik · 2020-02-19T15:13:29Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageClient.scala

@@ -112,3 +123,33 @@ case class MetalsInputBoxResult(
    @Nullable value: String = null,
    @Nullable cancelled: java.lang.Boolean = null
 )
+
+case class MetalsPickInputParams(


Can we move it to a separate file?

I just followed MetalsInputBoxParams, defined in this file before.

tgodzik · 2020-02-19T16:10:52Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala

+              Some(directory.getAsString()).map(new URI(_))
+            )
+          case Seq(_: JsonNull) =>
+            newFilesProvider.createNewFileDialog(None)


Maybe use a named parameter for None

tgodzik · 2020-02-19T16:16:23Z

metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala

@@ -106,6 +111,8 @@ object MetalsServerConfig {
          openFilesOnRenames = true,
          executeClientCommand = ExecuteClientCommandConfig.on,
          globSyntax = GlobSyntaxConfig.vscode,
+          isInputBoxEnabled = true,


That should be in future enabled with the client capabilities, so after @ckipp01 's PR you can move it there.

tgodzik · 2020-02-19T18:01:55Z

tests/unit/src/main/scala/tests/TestingClient.scala

@@ -71,6 +71,7 @@ final class TestingClient(workspace: AbsolutePath, buffers: Buffers)
    _: ShowMessageRequestParams =>
      None
  }
+  var inputBoxHandler: MetalsInputBoxParams => Option[MetalsInputBoxResult] = _


Can you add a default implementation returning some random name? You can have var nameToPick = "Foo" and replace that if needed. You also have None as the default if it's an issue.

Otherwise, we will get a null pointer in case of some particular changes.

tgodzik · 2020-02-19T18:14:20Z

tests/unit/src/test/scala/tests/NewFilesLspSuite.scala

  test("new-worksheet") {
    cleanCompileCache("a")
    RecursivelyDelete(workspace.resolve("a"))
    Files.createDirectories(
      workspace.resolve("a/src/main/scala/").toNIO
    )
+    client.showMessageRequestHandler = { params =>


Maybe a utility closure:

def createMessageRequestHander(name : String) : MessageRequestParams => Option[MessageRequestResult] = { params => if (NewScalaFile.isSelectTheKindOfFile(params)) { params.getActions().asScala.find(_.getTitle() == name) } else { None } }

then you can just use:
client.showMessageRequestHandler = createMessageRequestHander("worksheet")

Or even better create a check method:

def check(picked : String, expectedOutput : String, name : String = "Foo", shouldCheckForName : Boolean)

then you can just have:

check( "object", """| object Foo { |} |""".stripMargin

createMessageRequestHande might not be needed then.

not sure, if I got you correctly, but I've done something there - please, check.

tgodzik · 2020-02-19T18:28:05Z

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

+
+  implicit class DialogContinuation[A](state: Future[Option[A]]) {
+
+    def continueWith[B](


Looks good, this is almost a MonadTransformer haha.

Maybe flatMapOption and mapOption ? This will be a bit more natural.

And these methods might be actually useful to add to MetalsEnrichments, I remember situations when we were combining Futures and Options.

Yeah, I also thought about OptionT :D . Done

tgodzik · 2020-02-19T18:32:53Z

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

    val result = Future {
-      Files.createFile(path.toNIO).toUri()
+      AbsolutePath(
+        Files.createFile(path.toNIO)


You can use touch from XtensionAbsolutePathBuffers

touch does not return the path, and although I already have the path, I like to reuse output of the function.

Can we change touch to return the path?

tgodzik · 2020-02-19T18:36:55Z

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

+  )(
+      implicit ec: ExecutionContext
+  ): Future[AbsolutePath] = {
+    val path = directory.getOrElse(workspace).resolve(name + ".scala")


What happens if the name is a/b/c/A - I think we should create the directories there. Most likely createFile should also create directories? Package will then be added automatically which is neat.

it fails. Not really sure, it looks neat to ask for the name for the new class foo/Bar to create class Barat foo/Bar.scala

Could we implement it here? I think we just need to do getParent.mkdirs

ckipp01 · 2020-02-20T12:15:10Z

metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala

@@ -106,6 +111,8 @@ object MetalsServerConfig {
          openFilesOnRenames = true,
          executeClientCommand = ExecuteClientCommandConfig.on,
          globSyntax = GlobSyntaxConfig.vscode,
+          isInputBoxEnabled = true,
+          isPickInputEnabled = true,


As a follow-up to my previous comment, and similiar to what @tgodzik mentioned, if you rebase on master now, you'll see the pattern I followed in #1414. Moving forward for most cases, we shouldn't need to add new server properties, but rather list them under ClientExperimentalCapabilities.

I've added client properties, but server ones are still there, cause scalameta/metals-vscode#207 is not being pushed.

But that's because those properties already exist and are relied upon via clients that are already using them. This is a brand new one that we are introducing, so I'm not sure I understand why we also need to support this with client properties since nothing is relying on it? We can just only support it via ClientExperimentalCapabilities, and introduce it in vscode via experimentl.

isPickInputEnabled removed now, and capability added to the client in scalameta/metals-vscode#183. Though isInputBoxEnabled = true still in this place, I am not sure, if it should not be here now.

Yea, for now that's fine since that property already existed anyways. Once we release, and the the vs code pr gets merged, we can then remove the isInputBoxEnabled since then it will be set via experimental from vs code.

alekseiAlefirov

Thanks for the profound review, @tgodzik . I've addressed your comments, please, check it out.

olafurpg

Nice job writing the docs and implementing the endpoint! Just a small comment on the name of the endpoint.

olafurpg · 2020-02-21T10:49:16Z

docs/editors/new-editor.md

@@ -541,6 +541,73 @@ export interface MetalsInputBoxResult {
 }
 ```

+### `metals/pickInput`


Would it make sense to name this metals/quickPick? In general I try to follow the same naming convention as in the VS Code APIs

Yeah, I was not sure about the name, so if you think, this one's better, okay then. Renamed.

tgodzik

Couple of follow up comments, I think it's almost ready!

tgodzik · 2020-02-21T13:06:03Z

metals/src/main/scala/scala/meta/internal/metals/Messages.scala

+    def selectTheKindOfFileMessage = "Select the kind of file to create"
+    def enterNameMessage(kind: String): String = s"Enter name for the new $kind"
+
+    def isSelectTheKindOfFile(params: ShowMessageRequestParams): Boolean =


That's what I meant, I think they can just be move to the test class.

tgodzik · 2020-02-21T13:07:26Z

metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala

+      )
+  }
+
+  implicit class OptionFutureLift[A](state: Future[A]) {


There should already be XtensionScalaFuture

tgodzik · 2020-02-21T13:08:26Z

metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala

@@ -106,6 +111,8 @@ object MetalsServerConfig {
          openFilesOnRenames = true,
          executeClientCommand = ExecuteClientCommandConfig.on,
          globSyntax = GlobSyntaxConfig.vscode,
+          isInputBoxEnabled = true,


I would even remove it from here, since we want to drop the properties.

tgodzik · 2020-02-21T13:09:48Z

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

+  )(
+      implicit ec: ExecutionContext
+  ): Future[AbsolutePath] = {
+    val path = directory.getOrElse(workspace).resolve(name + ".scala")


Could we implement it here? I think we just need to do getParent.mkdirs

tgodzik · 2020-02-21T13:10:31Z

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

    val result = Future {
-      Files.createFile(path.toNIO).toUri()
+      AbsolutePath(
+        Files.createFile(path.toNIO)


Can we change touch to return the path?

tgodzik · 2020-02-21T13:12:59Z

tests/unit/src/test/scala/tests/NewFilesLspSuite.scala


+  private def check(


I like the direction, but I thought of something similar to RenameLspSuite and other, where we would even invoke test from here.

alekseiAlefirov · 2020-02-21T16:27:22Z

@tgodzik

isInputBoxEnabled = true,

I would even remove it from here, since we want to drop the properties.

What should I do with the client part scalameta/metals-vscode#183 ?

I could add inputBox capability, but it's already in @ckipp01 's PR Add in clientExperimentalCapabilities from old property settings metals-vscode#207
I could rebase on that PR, so it is adequate to this PR without the isInputBoxEnabled line
I could leave it, just wait for that PR to be merged.

alekseiAlefirov · 2020-02-21T16:44:29Z

@tgodzik PR reworked,

That's what I meant, I think they can just be move to the test class.

Moved.

I would even remove it from here, since we want to drop the properties.

removed, but see previous comment.

Could we implement it here? I think we just need to do getParent.mkdirs

done.

Can we change touch to return the path?

Nah, I just returned path there (and changed Files.create to touch).

tgodzik

LGTM! Thanks for addressing the comments!

olafurpg

Great work @alekseiAlefirov ! 🎉 I'm super excited to start using this feature 😄

ckipp01 · 2020-02-24T12:45:56Z

metals/src/main/scala/scala/meta/internal/metals/Messages.scala

+  object NewScalaFile {
+    def selectTheKindOfFileMessage = "Select the kind of file to create"
+    def enterNameMessage(kind: String): String =
+      s"Enter the name for the new $kind"


Super nitpicky, but I just tested this, and is it possible to just add a : in here?

Suggested change

s"Enter the name for the new $kind"

s"Enter the name for the new $kind:"

it seems not to be going look good here:

Let's leave it as is, I don't think it adds that much visually and it's not really being used in UI anywhere.

Hmmm, yea I see what you're saying. That's tricky, because in coc-metals there isn't a nice break, so it seems like there should really be a :. No worries, I'll just add it in on the vim side.

Also, this feature works great 🎉 !!

thanks 😌

alekseiAlefirov mentioned this pull request Feb 17, 2020

Added context menu command to create new worksheet scalameta/metals-vscode#183

Merged

alekseiAlefirov commented Feb 17, 2020

View reviewed changes

ckipp01 reviewed Feb 17, 2020

View reviewed changes

tgodzik requested changes Feb 19, 2020

View reviewed changes

ckipp01 reviewed Feb 20, 2020

View reviewed changes

alekseiAlefirov force-pushed the pick-input branch 2 times, most recently from a055063 to 248ab2a Compare February 20, 2020 15:29

alekseiAlefirov commented Feb 20, 2020

View reviewed changes

olafurpg changed the title ~~metals/pickInput method endpoint added, new-scala-file re-implemented on it~~ Add new LSP extension 'metals/pickInput' to implement the "Create Scala file" command Feb 21, 2020

olafurpg reviewed Feb 21, 2020

View reviewed changes

Aleksei Alefirov added 4 commits February 21, 2020 13:00

metals/pickInput method endpoint added, new-files re-implemented on it

b02008c

PR reworks + client experimental settings added

eec9a38

Moved pickInput enable setting from server to client

618f92b

pickInput renamed to quickpick

2431896

alekseiAlefirov force-pushed the pick-input branch from 72f5636 to 2431896 Compare February 21, 2020 12:04

alekseiAlefirov marked this pull request as ready for review February 21, 2020 12:07

alekseiAlefirov requested a review from tgodzik February 21, 2020 12:07

tgodzik requested changes Feb 21, 2020

View reviewed changes

PR reworks

948961c

alekseiAlefirov requested a review from tgodzik February 21, 2020 16:44

tgodzik approved these changes Feb 24, 2020

View reviewed changes

olafurpg approved these changes Feb 24, 2020

View reviewed changes

ckipp01 reviewed Feb 24, 2020

View reviewed changes

ckipp01 mentioned this pull request Feb 24, 2020

add in new way to create a scala file scalameta/coc-metals#92

Merged

tgodzik merged commit b897fe2 into scalameta:master Feb 24, 2020


		implicit class DialogContinuation[A](state: Future[Option[A]]) {

		def continueWith[B](

	s"Enter the name for the new $kind"
	s"Enter the name for the new $kind:"

Add new LSP extension 'metals/pickInput' to implement the "Create Scala file" command #1447

Add new LSP extension 'metals/pickInput' to implement the "Create Scala file" command #1447

Conversation

alekseiAlefirov commented Feb 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alekseiAlefirov Feb 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alekseiAlefirov Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alekseiAlefirov left a comment • edited Loading

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alekseiAlefirov Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alekseiAlefirov commented Feb 21, 2020

alekseiAlefirov commented Feb 21, 2020

tgodzik left a comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alekseiAlefirov Feb 17, 2020 •

edited

Loading

alekseiAlefirov Feb 20, 2020 •

edited

Loading

alekseiAlefirov left a comment •

edited

Loading

alekseiAlefirov Feb 21, 2020 •

edited

Loading