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

improvement: thresholds for folding regions #7013

Merged
merged 1 commit into from
Dec 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ abstract class MetalsLspService(
trees,
buffers,
foldOnlyLines = initializeParams.foldOnlyLines,
clientConfig.initialConfig.foldingRageMinimumSpan,
)

protected val diagnostics: Diagnostics = new Diagnostics(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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](
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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] = {
Expand All @@ -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
}
Expand All @@ -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]()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import java.util
import java.util.Collections

import scala.meta.inputs.Input
import scala.meta.inputs.Position
import scala.meta.internal.metals.Buffers
import scala.meta.internal.metals.MetalsEnrichments._
import scala.meta.internal.parsing.FoldingRangeProvider._
Expand All @@ -16,6 +15,7 @@ final class FoldingRangeProvider(
val trees: Trees,
buffers: Buffers,
foldOnlyLines: Boolean,
foldingRageMinimumSpan: Int,
) {

def getRangedForScala(
Expand All @@ -32,7 +32,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())
Expand All @@ -45,7 +49,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())
Expand All @@ -61,11 +67,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)
Expand Down Expand Up @@ -93,13 +94,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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>>{
Expand All @@ -41,13 +42,13 @@ String hello(String str) >>region>>{
return "asssd";
}<<region<<

void openingCommentInStr() >>region>>{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we lose a region here? That should be 3 lines with all the {}

Same below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I counted only the lines that actually get folded, so:

def a = {
  1
}

is one

def a =
  1

is also one. Think it's a bit more intuitive than if these had two and one respectively. I can offset by one, so both count as 2, would that be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let's try this solution. I don't really have preference and it looks sensible. Thanks for explaining!

void openingCommentInStr() {
System.out.println("/* ignore");
}<<region<<
}

void closingCommentInStr() >>region>>{
void closingCommentInStr() {
System.out.println("*/ ignore");
}<<region<<
}

void handleLineWithBlockAndCode() >>region>>{
if (true) >>region>>{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
def xd =>>region>>
val t =
1 + 2

t match
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 have a region for match as weel

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
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public class Main {

abstract class A {
abstract void hello();
abstract void hello2();
}

abstract class B extends A {
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def fooNested(): Unit =>>region>>

def fooWithMatch(): Unit =>>region>>
def bar(): Unit =>>region>>
??? match
??? match>>region>>
case 1 =>>>region>>
???
???
Expand All @@ -56,7 +56,7 @@ def fooWithMatch(): Unit =>>region>>
case 6 => ???<<region<<
???
???
???<<region<<<<region<<
???<<region<<<<region<<<<region<<
???
???
???
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ class A >>region>>{
}<<region<<

def noSpacing =>>region>>
for>>region>>{
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 still have this region as well, no?

for{
x <- ???
}<<region<<{
}>>region>>{
???
???
???
???
}<<region<<
}<<region<<<<region<<

def why =>>region>>
for
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ class A >>region>>{
def noSpacing =>>region>>
for{
x <- ???
}yield{
}yield>>region>>{
???
???
???
}<<region<<
}<<region<<<<region<<

def why =>>region>>
for
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
class A>>region>>{
val multilineString =>>region>>
"""
""">>region>>
|
|
|
|
|
|
""".stripMargin<<region<<
"""<<region<<.stripMargin<<region<<

val b = ???
}<<region<<
9 changes: 5 additions & 4 deletions tests/unit/src/test/resources/foldingRange/expect/Main.java
Original file line number Diff line number Diff line change
Expand Up @@ -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>>{
Expand All @@ -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>>{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
class A >>region>>{
val tryCatch =>>region>>
try ???
catch {
catch >>region>>{
case 0 =>
case 1 => println()
case 2 =>>>region>>
Expand All @@ -12,7 +12,7 @@ class A >>region>>{
case 3 =>
println()
case _ => println()
}<<region<<
}<<region<<<<region<<

val patternMatching =>>region>> ??? match {
case 0 =>
Expand Down
Loading
Loading