-
Notifications
You must be signed in to change notification settings - Fork 26
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
Switch between filenames #19 #33
Conversation
Just opening to show progress, this is still not working. Nothing happens when calling "Show definition" to the |
@ScalaJSDefined | ||
class ScalaTextContentProvider extends ITextModelContentProvider { | ||
override def provideTextContent(resource: Uri): Promise[IModel] = { | ||
val model = Editor.createModel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One possible fix for the "cannot instantiate second model with the same URI" error would be to first lookup if a model already exists using https://microsoft.github.io/monaco-editor/api/modules/monaco.editor.html#getmodel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That did the trick, thanks!
@@ -2624,3 +2671,27 @@ package worker { | |||
object Monaco extends js.Object { | |||
type MarkedString = String | js.Any | |||
} | |||
|
|||
package services { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we put the service definitions in a separateMonacoServices.scala
file and document rigorously anything that would help a future upgrade or Monaco.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I've added a note about the ITextModelResolverService rename.
@@ -11,12 +11,12 @@ import monaco.languages.Languages.Definition | |||
import monaco.editor.Editor.BuiltinTheme | |||
import monaco.editor.Editor.LineNumbersOption | |||
|
|||
@js.native | |||
@ScalaJSDefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? If we can get away with only using monaco.Promise I think that is preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary. I managed to avoid it by using Future[T], JSConverters.toJSPromise and a case to monaco.Thenable[T].
package common { | ||
|
||
@ScalaJSDefined | ||
trait IDisposable extends js.Object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks similar to the one in monaco.IDisposable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ScalaJSDefined | ||
class ScalaTextContentProvider extends ITextModelContentProvider { | ||
override def provideTextContent(resource: Uri): Promise[IModel] = { | ||
val model = Editor.createModel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That did the trick, thanks!
package common { | ||
|
||
@ScalaJSDefined | ||
trait IDisposable extends js.Object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, removed.
@@ -11,12 +11,12 @@ import monaco.languages.Languages.Definition | |||
import monaco.editor.Editor.BuiltinTheme | |||
import monaco.editor.Editor.LineNumbersOption | |||
|
|||
@js.native | |||
@ScalaJSDefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary. I managed to avoid it by using Future[T], JSConverters.toJSPromise and a case to monaco.Thenable[T].
@@ -2624,3 +2671,27 @@ package worker { | |||
object Monaco extends js.Object { | |||
type MarkedString = String | js.Any | |||
} | |||
|
|||
package services { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I've added a note about the ITextModelResolverService rename.
It seems the ITextContentProvider is more relevant for us than the editor service, but I can't figure out how to override the ITextContentProvider. This is just a demo for how to add random content when switching URI.
- Missing a robust converstion from Future[T] to Thenable[T]. - This currently does something when calling "Show references" but nothing when calling "show definition"
- Search "Chunk" - "See references" - double click on refernce in Chunk.scala - File contents change and active model too. This is not a very robust solution. There seems to be an assumption that the editor is a SimpleEditor, and implements methods like getControl(). This commit just adds a stub method for this method. Eventually, we may need to implement the full editor ourselves.
This PR is ready for another review. It seems there's been a regression in the definition provider, but the reference provider works for multiple files now. I'm investigating what's gone wrong with the definition provider. |
The model was undefined when jumping to Chunk.scala, causing a NPE. This commit changes the definition provider to create the model if it doesn't exist before returning the definition location.
This makes it possible to use the browser's back/forward buttons to switch files.
Previously, we assumed Attributes for a single file was always available, which was used to power services such as definition provider. This works no longer since the active file can change. This commit reorganizes the codebase so that attributes are lazily fetched by uri. We currently don't cache attributes, but that might be a good idea eventually.
I'm think I'm getting the hang of this 😸 It's not possible to jump to definitions across files, and use the browser's back/forward button. Reference provider returns weird positions for cross-file references, I'm investigating what's causing that. |
Now that we can switch between files and attributes are lazily fetched, we can load symbols lazily from `symbols/`. This shrinks metadoc.index, which should help with scaling metadoc for larger codebases.
Document symbol provider only needs the definition.
The uri encoding did not handle weird characters like `#`. Persisting the references in metadoc.index is wasteful, this change makes it possible to lookup the references only when needed.
Previously, the reference provider used the same model for all found references, including references from other files. This caused the ranges to be OK for the active file but wrong for other files. This commit changes reference provider to create a new model for each reference before creating the range selection.
- Remove unused imports - Reorganize code.
All services are fully functioning now for multiple files so I think this PR is ready to go. The diff became quite big, a lot of things had to be refactored to handle the new multi-file scheme. I added the paiges test sources to the corpus and it's pretty clear that the current reference provider will not become too slow for larger repos since the reference provider eagerly loads ALL references and creates the model for file that contains a reference. I think this is a very promising start, however. |
We can worry about caching/optimizations later 😄 |
Previously, searching for a non-existent symbol resulted in an uncaught exception. Now we handle 404s and missing symbols, causing the UI to render a friendly "No results" page instead of "Loading....".
I took the liberty to push the site from the latest commit in the PR http://scalameta.org/metadoc I noticed that the results are weird when using reference provider on |
// do nothing for symbols that are not globally relevant. | ||
case Symbol.Local(_) => | ||
case Symbol.Global(_, _: Signature.TypeParameter) => | ||
case Symbol.Global(_, _: Signature.TermParameter) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed these changes break the definition provider for local symbols. We should be able to work around that by parsing the active model value instead storing them in the global index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow 👏 good stuff. If you prefer we can merge ASAP and do follow ups in separate PRs.
) | ||
editor.setModel(model) | ||
|
||
dom.window.onhashchange = { e: Event => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Didn't know it was this simple to get basic routing working.
It seems doable to also get "jump to line" working if we push the line into the URL and have onDidChangeModel
call revealPosition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! Line numbers would definitely be nice, I opened #38
import scala.concurrent.ExecutionContext.Implicits.global | ||
import scala.concurrent.Future | ||
import scala.meta._ | ||
import scala.meta.internal.semantic.{schema => s} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does using this API offer compared to Database.load
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is "lower-level", it's the plain protobuf messages. For metadoc I've found the protobuf messages to be very useful, they match closely with the monaco api.
val model = Editor.getModel(input.resource) | ||
editor.setModel(model) | ||
selection.foreach { range => | ||
val pos = new Position(range.startLineNumber, range.startColumn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Didn't know about that one, although it requires a cast to Range
from IRange
.
|
||
def findModel(model: IModel, data: IResourceInput): IEditorModel = { | ||
if (model.uri.toString() != data.resource.toString()) { | ||
null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be interested in having this log something to make sure we don't miss any cases. I am also wondering if findModel
ever gets called. Looking at the Simple services code it is private
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this is indeed dead code. Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I agree we should log or fail fast on error cases!
// It's possible to hit those errors for example in the reference provider, where | ||
// we call `Future.sequence(Seq.map(modelReference))`. Since js is single threaded, | ||
// we are safe from race conditions. | ||
private val cache = mutable.Map.empty[String, IModel] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So editor.getModel(s)
didn't work as a cache? We need to figure out if Monaco ever calls model.dispose()
and we subsequently need to invalidate those entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
editor.getModel(s)
worked fine until I called Future.sequence(Seq[Future[IModel]])
, the guard on line 39 is not sufficient since it's possible another callback creates a new model after the guard but before we finish fetching attributes on line 44. Maybe there's a cleaner way to accomplish this, this was a quick hack.
@@ -513,6 +513,57 @@ class Token protected () extends js.Object { | |||
} | |||
|
|||
package editor { | |||
@ScalaJSDefined | |||
trait IEditorService extends js.Object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind if we also move this (eventually) to monaco.services
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Good idea.
@@ -1962,6 +2017,7 @@ package editor { | |||
diffEditor: IStandaloneDiffEditor, | |||
opts: IDiffNavigatorOptions = ??? | |||
): IDiffNavigator = js.native | |||
// NOTE: Do not call this method, use MetadocTextModelService.createModel instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the cache and duplicate URI issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amongst other things, it will also be useful to have a single place to create models in #34
@@ -2286,7 +2342,7 @@ package languages { | |||
position: Position, | |||
context: ReferenceContext, | |||
token: CancellationToken | |||
): js.Array[Location] | Thenable[js.Array[Location]] | |||
): Thenable[js.Array[Location]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify the signature in our implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and to (sadly) please IntelliJ.
def `object`: T | ||
} | ||
|
||
object IReference { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IReference
signifies "interface" so I suggest we do something like this instead to also signal the lifecycle guarantees:
@ScalaJSDefined
class ImmortalReference[T](override val `object`: T) extends IReference[T] {
override def dispose(): Unit = ()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! Done.
case class MetadocSite(semanticdb: Seq[AbsolutePath], index: d.Index) | ||
case class MetadocSite( | ||
semanticdb: Seq[AbsolutePath], | ||
symbols: Seq[d.Symbol], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the reason for adding back symbols here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to support the reference provider without storing references
in Index. The index gets really fat quickly and it's unnecessary to download all references to all symbols on startup time. This allows us to lookup references lazily when reference provider is invoked.
Note that Index.symbols
is still there, but I strip out the references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review! I also just noticed #39
Let's merge this and follow up with fixes.
case class MetadocSite(semanticdb: Seq[AbsolutePath], index: d.Index) | ||
case class MetadocSite( | ||
semanticdb: Seq[AbsolutePath], | ||
symbols: Seq[d.Symbol], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to support the reference provider without storing references
in Index. The index gets really fat quickly and it's unnecessary to download all references to all symbols on startup time. This allows us to lookup references lazily when reference provider is invoked.
Note that Index.symbols
is still there, but I strip out the references.
) | ||
editor.setModel(model) | ||
|
||
dom.window.onhashchange = { e: Event => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! Line numbers would definitely be nice, I opened #38
import scala.concurrent.ExecutionContext.Implicits.global | ||
import scala.concurrent.Future | ||
import scala.meta._ | ||
import scala.meta.internal.semantic.{schema => s} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is "lower-level", it's the plain protobuf messages. For metadoc I've found the protobuf messages to be very useful, they match closely with the monaco api.
case _: NoSuchElementException => None // 404, not found | ||
} | ||
|
||
def fetchsAttributes(filename: String): Future[s.Attributes] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a terrible name, it should be something like fetchProtoAttributes
instead
val model = Editor.getModel(input.resource) | ||
editor.setModel(model) | ||
selection.foreach { range => | ||
val pos = new Position(range.startLineNumber, range.startColumn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Didn't know about that one, although it requires a cast to Range
from IRange
.
def toMonacoPromise: Promise[T] = | ||
Promise.wrap(toMonacoThenable) | ||
def toMonacoThenable: Thenable[T] = | ||
future.toJSPromise.asInstanceOf[Thenable[T]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yess 🙏 You can imagine the relief when I saw this worked 😅
@@ -513,6 +513,57 @@ class Token protected () extends js.Object { | |||
} | |||
|
|||
package editor { | |||
@ScalaJSDefined | |||
trait IEditorService extends js.Object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Good idea.
@@ -1962,6 +2017,7 @@ package editor { | |||
diffEditor: IStandaloneDiffEditor, | |||
opts: IDiffNavigatorOptions = ??? | |||
): IDiffNavigator = js.native | |||
// NOTE: Do not call this method, use MetadocTextModelService.createModel instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amongst other things, it will also be useful to have a single place to create models in #34
@@ -2286,7 +2342,7 @@ package languages { | |||
position: Position, | |||
context: ReferenceContext, | |||
token: CancellationToken | |||
): js.Array[Location] | Thenable[js.Array[Location]] | |||
): Thenable[js.Array[Location]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and to (sadly) please IntelliJ.
def `object`: T | ||
} | ||
|
||
object IReference { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! Done.
No description provided.