Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix:selection expansion in comments #4921

Merged
merged 7 commits into from
Apr 26, 2023
Merged

fix:selection expansion in comments #4921

merged 7 commits into from
Apr 26, 2023

Conversation

doofin
Copy link
Contributor

@doofin doofin commented Jan 29, 2023

solves #3615
only works with pure scala 3 project,doesn't seem to work with worksheet

There's redundancy in code but I'm not sure how to reduce it :

  val tkSrc = programStr.parse[Source].toOption.map(_.tokens)
    val tkExpr = programStr.parse[Stat].toOption.map(_.tokens)
    val tkType = programStr.parse[Type].toOption.map(_.tokens)

    val tokens = tkSrc orElse tkExpr orElse tkType

@doofin doofin changed the title improvement:selection expansion in comments [WIP] improvement:selection expansion in comments Jan 29, 2023
@doofin doofin marked this pull request as draft January 30, 2023 13:53
Comment on lines 111 to 117
val programStr = param.text()
// deal with multiple source text. probably a better way ?
val tkSrc = programStr.parse[Source].toOption.map(_.tokens)
val tkExpr = programStr.parse[Stat].toOption.map(_.tokens)
val tkType = programStr.parse[Type].toOption.map(_.tokens)

val tokens = tkSrc orElse tkExpr orElse tkType // orElse tkTopDecls
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use something along the lines of:

val (treeStart, treeEnd) = path.headOption
      .map(t => (t.pos.start, t.pos.end))
      .getOrElse((0, text.size))
val offset = pos.start
val tokens = text.slice(treeStart, treeEnd)tokenize.toOption
      tokens
        .flatMap(t =>
          t.collectFirst {
            case t: Token.Comment
                if treeStart + t.pos.start < currentOffset &&
                  treeStart + t.pos.end >= currentOffset =>
              val range = new SelectionRange()
              range.setRange(t.pos.toLSP)
              Some(range)
          
            case _ =>
              Noene
          }
        )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggestion,but i'm not sure how to update offset to input ,which will cause error if it's not updated (used later in def toLsp: l.Range of extension (pos: SourcePosition)):

(commentList) // ++ merged)
        .filter((commentStart, commentEnd, _) =>
          commentStart <= cursorStartShifted && cursorStartShifted <= commentEnd
        )
        .map { (commentStart, commentEnd, oldPos) =>
          // todo! need to upg input if offset is nonzero!
          val newPos: Position =
            meta.Position.Range(
              oldPos.input,
              commentStart + offsetStart,
              commentEnd + offsetStart,
            )
          newPos
        }

@doofin doofin marked this pull request as ready for review February 24, 2023 15:54
@doofin doofin changed the title [WIP] improvement:selection expansion in comments fix:selection expansion in comments Mar 17, 2023
@doofin doofin requested review from tgodzik and removed request for tgodzik March 17, 2023 20:14
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.

Let's simplify it a bit and could you write some tests?

}

// WIP: when there are multiple single line comments,select them together
val merged = tokenList
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should merge mutiple single line comments, that is usually not that for anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this

Comment on lines 127 to 131
val commentList: List[(Int, Int, Position)] = tokenList.collect {
case x: Comment =>
println(
s"find Comment \"${x}\" at ${(x.start, x.`end`)} " // :
) // , ${(x.start, x.`end`, x.pos)}
(x.start, x.`end`, x.pos)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val commentList: List[(Int, Int, Position)] = tokenList.collect {
case x: Comment =>
println(
s"find Comment \"${x}\" at ${(x.start, x.`end`)} " // :
) // , ${(x.start, x.`end`, x.pos)}
(x.start, x.`end`, x.pos)
}
val commentList: Option[lsp4j.Range] = tokenList.collectFirst {
case x: Comment if cursorStartShifted && cursorStartShifted <= commentEnd =>
SourcePosition(
cursorStart.source,
Spans.Span(
commentStart + offsetStart,
commentEnd + offsetStart,
),
).toLsp
}

this should be enough and can be returned from the function.

Copy link
Contributor Author

@doofin doofin Apr 15, 2023

Choose a reason for hiding this comment

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

Seems not quite so clear how to simplify this since some filter are complicated, but I have cleaned up a bit

@doofin
Copy link
Contributor Author

doofin commented Apr 15, 2023

Let's simplify it a bit and could you write some tests?

I write the test SelectionRangeCommentSuite but unable to run it due to lots of compile errors with testOnly tests.pc.SelectionRangeCommentSuite, maybe I run it with the wrong sbt command since this feature is only for Scala 3 ? The manual test is ok.

@doofin doofin requested a review from tgodzik April 17, 2023 04:23
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.

I took the liberty to change a couple of small things. Thanks for contributing!

@tgodzik tgodzik merged commit af2cbb2 into scalameta:main Apr 26, 2023
@doofin
Copy link
Contributor Author

doofin commented Apr 27, 2023

Thanks ! very excited to my first contribution merged !

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants