improvement: thresholds for folding regions

- don't merge region with parent when if parent too small without it - allow to configure folding region threshold using server properties - make threshold work consistently
scalameta · Dec 6, 2024 · 6e3f017 · 6e3f017
1 parent 41a2d57
commit 6e3f017
Show file tree

Hide file tree

Showing 19 changed files with 118 additions and 53 deletions.
diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
@@ -255,6 +255,7 @@ abstract class MetalsLspService(
     trees,
     buffers,
     foldOnlyLines = initializeParams.foldOnlyLines,
+    clientConfig.initialConfig.foldingRageMinimumSpan,
   )
 
   protected val diagnostics: Diagnostics = new Diagnostics(

diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala
@@ -121,6 +121,11 @@ final case class MetalsServerConfig(
       "metals.enable-best-effort",
       default = false,
     ),
+    foldingRageMinimumSpan: Int =
+      Option(System.getProperty("metals.folding-range-minimum-span"))
+        .filter(_.forall(Character.isDigit(_)))
+        .map(_.toInt)
+        .getOrElse(3),
 ) {
   override def toString: String =
     List[String](
@@ -144,6 +149,7 @@ final case class MetalsServerConfig(
       s"worksheet-timeout=$worksheetTimeout",
       s"debug-server-start-timeout=$debugServerStartTimeout",
       s"enable-best-effort=$enableBestEffort",
+      s"folding-range-minimum-span=$foldingRageMinimumSpan",
     ).mkString("MetalsServerConfig(\n  ", ",\n  ", "\n)")
 }
 object MetalsServerConfig {

diff --git a/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeExtractor.scala b/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeExtractor.scala
@@ -18,10 +18,8 @@ import org.eclipse.{lsp4j => l}
 final class FoldingRangeExtractor(
     distance: TokenEditDistance,
     foldOnlyLines: Boolean,
+    spanThreshold: Int,
 ) {
-  private val spanThreshold = 2
-  private val distanceToEnclosingThreshold = 3
-
   private val ranges = new FoldingRanges(foldOnlyLines)
 
   def extract(tree: Tree): util.List[FoldingRange] = {
@@ -31,19 +29,19 @@ final class FoldingRangeExtractor(
   }
 
   def extractFrom(tree: Tree, enclosing: Position): Unit = {
-    // All Defn statements except one-liners must fold
-    if (span(tree.pos) > spanThreshold) {
+    if (span(tree.pos) > 0) {
       val newEnclosing = tree match {
+        case _ if tree.pos.startLine == enclosing.startLine =>
+          enclosing
         case Foldable((pos, adjust))
-            if tree.is[Defn] || span(enclosing) - span(
-              pos
-            ) > distanceToEnclosingThreshold =>
+            if pos.startLine != enclosing.startLine && spanAboveThreshold(
+              pos,
+              adjust,
+            ) =>
           distance.toRevised(pos.toLsp) match {
             case Some(revisedPos) =>
-              if (pos.startLine != enclosing.startLine) {
-                val range = createRange(revisedPos)
-                ranges.add(Region, range, adjust)
-              }
+              val range = createRange(revisedPos)
+              ranges.add(Region, range, adjust)
               pos
             case None => enclosing
           }
@@ -63,6 +61,11 @@ final class FoldingRangeExtractor(
     }
   }
 
+  private def spanAboveThreshold(pos: Position, adjust: Boolean) = {
+    val adjustValue = if (adjust && foldOnlyLines) 1 else 0
+    span(pos) - adjustValue >= spanThreshold
+  }
+
   private def extractImports(trees: List[Tree]) = {
     val importGroups = mutable.ListBuffer(mutable.ListBuffer.empty[Import])
     val nonImport = mutable.ListBuffer[Tree]()

diff --git a/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeProvider.scala b/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeProvider.scala
@@ -16,6 +16,7 @@ final class FoldingRangeProvider(
     val trees: Trees,
     buffers: Buffers,
     foldOnlyLines: Boolean,
+    foldingRageMinimumSpan: Int,
 ) {
 
   def getRangedForScala(
@@ -32,7 +33,11 @@ final class FoldingRangeProvider(
       val distance = TokenEditDistance(fromTree, revised, trees).getOrElse(
         TokenEditDistance.NoMatch
       )
-      val extractor = new FoldingRangeExtractor(distance, foldOnlyLines)
+      val extractor = new FoldingRangeExtractor(
+        distance,
+        foldOnlyLines,
+        foldingRageMinimumSpan,
+      )
       extractor.extract(tree)
     }
     result.getOrElse(util.Collections.emptyList())
@@ -45,7 +50,9 @@ final class FoldingRangeProvider(
       code <- buffers.get(filePath)
       if filePath.isJava
     } yield {
-      JavaFoldingRangeExtractor.extract(code, filePath, foldOnlyLines).asJava
+      JavaFoldingRangeExtractor
+        .extract(code, filePath, foldOnlyLines, foldingRageMinimumSpan)
+        .asJava
     }
 
     result.getOrElse(util.Collections.emptyList())
@@ -61,11 +68,6 @@ final class FoldingRanges(foldOnlyLines: Boolean) {
 
   def get: util.List[FoldingRange] = Collections.unmodifiableList(allRanges)
 
-  def add(kind: String, pos: Position): Unit = {
-    val range = createRange(pos)
-    add(kind, range)
-  }
-
   def add(kind: String, range: FoldingRange): Unit = {
     range.setKind(kind)
     add(range, adjust = true)
@@ -93,13 +95,6 @@ final class FoldingRanges(foldOnlyLines: Boolean) {
     }
   }
 
-  private def createRange(pos: Position): FoldingRange = {
-    val range = new FoldingRange(pos.startLine, pos.endLine)
-    range.setStartCharacter(pos.startColumn)
-    range.setEndCharacter(pos.endColumn)
-    range
-  }
-
   // examples of collapsed: "class A {}" or "def foo = {}"
   private def isNotCollapsed(range: FoldingRange): Boolean =
     spansMultipleLines(range) || foldsMoreThanThreshold(range)

diff --git a/metals/src/main/scala/scala/meta/internal/parsing/JavaFoldingRangeExtractor.scala b/metals/src/main/scala/scala/meta/internal/parsing/JavaFoldingRangeExtractor.scala
@@ -20,7 +20,6 @@ import org.eclipse.lsp4j.FoldingRange
 import org.eclipse.lsp4j.FoldingRangeKind
 
 final object JavaFoldingRangeExtractor {
-  private val spanThreshold = 2
 
   private case class Range(
       startPos: Long,
@@ -165,6 +164,7 @@ final object JavaFoldingRangeExtractor {
       text: String,
       path: AbsolutePath,
       foldOnlyLines: Boolean,
+      spanThreshold: Int,
   ): List[FoldingRange] = {
     getTrees(text, path) match {
       case Some((trees, root)) =>

diff --git a/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/Main.java b/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/Main.java
@@ -23,6 +23,7 @@ public class Main >>region>>{
 
     abstract class A >>region>>{
         abstract void hello();
+        abstract void hello2();
     }<<region<<
 
     abstract class B extends A >>region>>{
@@ -41,13 +42,13 @@ String hello(String str) >>region>>{
             return "asssd";
         }<<region<<
 
-        void openingCommentInStr() >>region>>{
+        void openingCommentInStr() {
             System.out.println("/* ignore");
-        }<<region<<
+        }
 
-        void closingCommentInStr() >>region>>{
+        void closingCommentInStr() {
             System.out.println("*/ ignore");
-        }<<region<<
+        }
 
         void handleLineWithBlockAndCode() >>region>>{
             if (true) >>region>>{

diff --git a/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/i6938.scala b/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/i6938.scala
@@ -0,0 +1,28 @@
+def xd =>>region>>
+  val t =
+    1 + 2
+
+  t match
+    case 3 => println("ok")
+    case _ => println("ko")<<region<<
+end xd
+
+
+def xd2 =>>region>>
+  val t = 1 + 2
+
+  t match>>region>>
+    case 3 =>
+      println("ok")
+    case _ => println("ko")<<region<<<<region<<
+end xd2
+
+def xd3 =>>region>>
+  val t =
+    1 + 2
+
+  t match>>region>>
+    case 3 =>
+        println("ok")
+    case _ => println("ko")<<region<<<<region<<
+end xd3
diff --git a/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/input/Main.java b/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/input/Main.java
@@ -23,6 +23,7 @@ public class Main {
 
     abstract class A {
         abstract void hello();
+        abstract void hello2();
     }
 
     abstract class B extends A {

diff --git a/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/input/i6938.scala b/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/input/i6938.scala
@@ -0,0 +1,28 @@
+def xd =
+  val t =
+    1 + 2
+
+  t match
+    case 3 => println("ok")
+    case _ => println("ko")
+end xd
+
+
+def xd2 =
+  val t = 1 + 2
+
+  t match
+    case 3 =>
+      println("ok")
+    case _ => println("ko")
+end xd2
+
+def xd3 =
+  val t =
+    1 + 2
+
+  t match
+    case 3 =>
+        println("ok")
+    case _ => println("ko")
+end xd3
diff --git a/tests/unit/src/test/resources/foldingRange-scala3/expect/BlockWithNoBraces.scala b/tests/unit/src/test/resources/foldingRange-scala3/expect/BlockWithNoBraces.scala
@@ -43,7 +43,7 @@ def fooNested(): Unit =>>region>>
 
 def fooWithMatch(): Unit =>>region>>
   def bar(): Unit =>>region>>
-    ??? match
+    ??? match>>region>>
       case 1 =>>>region>>
         ???
         ???
@@ -56,7 +56,7 @@ def fooWithMatch(): Unit =>>region>>
           case 6 => ???<<region<<
         ???
         ???
-        ???<<region<<<<region<<
+        ???<<region<<<<region<<<<region<<
   ???
   ???
   ???

diff --git a/tests/unit/src/test/resources/foldingRange/expect/Blocks.scala b/tests/unit/src/test/resources/foldingRange/expect/Blocks.scala
@@ -19,20 +19,20 @@ class A >>region>>{
 
   def chain =>>region>> Seq(1).map{
     x =>
-    x + 1
-     + 1
+    >>region>>x + 1
      + 1
      + 1
      + 1
      + 1
      + 1
+     + 1<<region<<
   }.map>>region>>{
-    _ + 1
-    + 1
+    >>region>>_ + 1
     + 1
     + 1
     + 1
     + 1
+    + 1<<region<<
   }<<region<<<<region<<
 
   def chain =>>region>> Seq(1).map(

diff --git a/tests/unit/src/test/resources/foldingRange/expect/ForLoop.scala b/tests/unit/src/test/resources/foldingRange/expect/ForLoop.scala
@@ -7,14 +7,14 @@ class A >>region>>{
     }<<region<<
 
   def noSpacing =>>region>>
-    for>>region>>{
+    for{
       x <- ???
-    }<<region<<{
+    }>>region>>{
       ???
       ???
       ???
       ???
-    }<<region<<
+    }<<region<<<<region<<
 
   def why =>>region>>
     for

diff --git a/tests/unit/src/test/resources/foldingRange/expect/ForYield.scala b/tests/unit/src/test/resources/foldingRange/expect/ForYield.scala
@@ -9,11 +9,11 @@ class A >>region>>{
   def noSpacing =>>region>>
     for{
       x <- ???
-    }yield{
+    }yield>>region>>{
       ???
       ???
       ???
-    }<<region<<
+    }<<region<<<<region<<
 
   def why =>>region>>
     for

diff --git a/tests/unit/src/test/resources/foldingRange/expect/Literals.scala b/tests/unit/src/test/resources/foldingRange/expect/Literals.scala
@@ -1,13 +1,13 @@
 class A>>region>>{
   val multilineString =>>region>>
-    """
+    """>>region>>
       |
       |
       |
       |
       |
       |
-    """.stripMargin<<region<<
+    """<<region<<.stripMargin<<region<<
 
   val b = ???
 }<<region<<
diff --git a/tests/unit/src/test/resources/foldingRange/expect/Main.java b/tests/unit/src/test/resources/foldingRange/expect/Main.java
@@ -23,6 +23,7 @@ public class Main >>region>>{
 
     abstract class A >>region>>{
         abstract void hello();
+        abstract void aaa();
     }<<region<<
 
     abstract class B extends A >>region>>{
@@ -41,13 +42,13 @@ String hello(String str) >>region>>{
             return "asssd";
         }<<region<<
 
-        void openingCommentInStr() >>region>>{
+        void openingCommentInStr() {
             System.out.println("/* ignore");
-        }<<region<<
+        }
 
-        void closingCommentInStr() >>region>>{
+        void closingCommentInStr() {
             System.out.println("*/ ignore");
-        }<<region<<
+        }
 
         void handleLineWithBlockAndCode() >>region>>{
             if (true) >>region>>{

diff --git a/tests/unit/src/test/resources/foldingRange/expect/PartialFunctionCases.scala b/tests/unit/src/test/resources/foldingRange/expect/PartialFunctionCases.scala
@@ -1,7 +1,7 @@
 class A >>region>>{
   val tryCatch =>>region>>
     try ???
-    catch {
+    catch >>region>>{
       case 0 =>
       case 1 => println()
       case 2 =>>>region>>
@@ -12,7 +12,7 @@ class A >>region>>{
       case 3 =>
         println()
       case _ => println()
-    }<<region<<
+    }<<region<<<<region<<
 
   val patternMatching =>>region>> ??? match {
     case 0 =>

diff --git a/tests/unit/src/test/resources/foldingRange/expect/Try.scala b/tests/unit/src/test/resources/foldingRange/expect/Try.scala
@@ -1,7 +1,7 @@
 class A>>region>>{
   val shortExpr =>>region>>
     try ???
-    catch {
+    catch >>region>>{
       case 0 =>
       case 1 => println()
       case 2 =>>>region>>
@@ -12,7 +12,7 @@ class A>>region>>{
       case 3 =>
         println()
       case _ => println()
-    }<<region<<
+    }<<region<<<<region<<
 
   val aTry =>>region>>
     try >>region>>{