scala / scala-dev

Scala 2 team issues. Not for user-facing bugs or directly actionable user-facing improvements. For build/test/infra and for longer-term planning and idea tracking. Our bug tracker is at https://github.com/scala/bug/issues
Apache License 2.0
130 stars 14 forks source link

JLine 3: resolve double-tab behavior somehow for 2.13.2 #698

Closed SethTisue closed 4 years ago

SethTisue commented 4 years ago

under JLine 2 double tab had this behavior: on first tab it only shows the methods you're likeliest to want, then on second tab it adds more: things like toString, and also methods you only get through implicit conversions:

scala 2.13.1> class C { def x = 3; def y = 4 }
defined class C

scala 2.13.1> (new C).
x   y

scala 2.13.1> (new C).
!=   ->             ensuring   formatted   isInstanceOf   notifyAll      wait   →   
##   ==             eq         getClass    ne             synchronized   x          
+    asInstanceOf   equals     hashCode    notify         toString       y          

note the presence of ensuring, which is an extension method.

under JLine 3, that behavior still exists, but in a funny way. you have to ask for the same completion again, and that gives you what used to be the double-tab behavior.

ah, JLine 3 has a "grouping" feature, where you can divide offered completions into groups, and even show a name for each group! so we could have a main group, an extension method group, and an "other" group. maybe a "deprecated" group (https://github.com/scala/scala/pull/7869)

even if we can't get the grouping thing going in time, we need to do something for 2.13.2, IMO

SethTisue commented 4 years ago

as I already noted on https://github.com/scala/scala/pull/7869, an alternative to using the grouping feature would be

to set the descr field of org.jline.reader.Candidate to "deprecated". this would show up in the UI as e.g.:

Screen Shot 2020-04-02 at 6 21 03 PM
SethTisue commented 4 years ago

as for how to make this happen in the code,

PresentationCompilationResult has type Candidates = (Int, List[String]) and def candidates(tabCount: Int): Candidates

This is compiler stuff so we're free to change these signatures, but doing so makes the tooling authors grumpy. A minimalist, good-enough-for-2.13.2 approach might be to leave all of that alone for now and call candidates twice, once with tabCount of 0 and again with tabCount of 1, and then present the results in two groups.

But it's kind of a craptastic solution, because there are multiple reasons something might not show up until the second tab: there's deprecation, but there's also stuff like ## being filtered out until the second tab; we can't distinguish between those without changing the API.

I suppose we could continue to offer the old API but deprecate it, and have better API alongside it.

SethTisue commented 4 years ago

enabling groups is simple, as shown below, but I have a major frustrations with the resulting behavior: keyboard navigation is all messed up, the arrow keys make the cursor jump around weirdly

so in order to go the groups route, we'd either need to figure out if there's a workaround, or submit a fix to the JLine folks upstream

--- src/repl-frontend/scala/tools/nsc/interpreter/jline/Reader.scala
+++ src/repl-frontend/scala/tools/nsc/interpreter/jline/Reader.scala
@@ -85,12 +85,13 @@ object Reader {
     locally {
       import LineReader._, Option._
       builder
-        .option(AUTO_GROUP, false)
+        .option(AUTO_GROUP, true)
         .option(LIST_PACKED, true)  // TODO
         .option(INSERT_TAB, true)   // At the beginning of the line, insert tab instead of completing
         .variable(HISTORY_FILE, config.historyFile) // Save history to file
         .variable(SECONDARY_PROMPT_PATTERN, config.encolor(config.continueText)) // Continue prompt
-        .option(Option.DISABLE_EVENT_EXPANSION, true) // Otherwise `scala> println(raw"\n".toList)` gives `List(n)` !!
+        .variable(OTHERS_GROUP_NAME, " ")
+        .option(DISABLE_EVENT_EXPANSION, true) // Otherwise `scala> println(raw"\n".toList)` gives `List(n)` !!
     }

     val reader = builder.build()
@@ -231,11 +232,11 @@ class Completion(delegate: shell.Completion) extends shell.Completion with Compl
         case CompletionCandidate.Nilary => "()"
         case _ => "("
       })
-      val group = null        // results may be grouped
-      val descr =             // displayed alongside
+      val group =             // resulted may be grouped
         if (cc.isDeprecated) "deprecated"
         else if (cc.isUniversal) "universal"
         else null
+      val descr = null        // displayed alongside
       val suffix = null       // such as slash after directory name
       val key = null          // same key implies mergeable result
       val complete = false    // more to complete?
SethTisue commented 4 years ago

set the descr field

I have working code that does this, but in the absence of grouping, it's pretty annoying to have all the methods mixed together, so I guess the next step is to do the grouping ourselves by sorting

SethTisue commented 4 years ago

I guess the next step is to do the grouping ourselves by sorting

FAIL — JLine wants to sort the candidates itself (by value, which is the actual string that will be inserted when a completion is selected), and I don't see any way to override this. at least, I sunk as much time into it as I'm willing to sink at the moment

for 2.13.2, we'll have to live with everything mixed together:

Screen Shot 2020-04-21 at 8 30 21 PM

which is not so bad, really, given the presence of the (deprecated) and (universal) markers

SethTisue commented 4 years ago

closing on the expectation https://github.com/scala/scala/pull/8905 will be merged

SethTisue commented 4 years ago

this seems to indicate that we should try the grouping feature again in JLine 3.16.1 once it is released: https://github.com/jline/jline3/issues/580