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

Storing VirtualFile instead of Document for opened document #1082

Closed
angelozerr opened this issue Aug 8, 2023 · 12 comments · Fixed by #1187
Closed

Storing VirtualFile instead of Document for opened document #1082

angelozerr opened this issue Aug 8, 2023 · 12 comments · Fixed by #1187
Labels
Milestone

Comments

@angelozerr
Copy link
Contributor

The LanguageServerWrapper works with Document instead of VirtualFile. I think this choice has been done because LSP4E works like this. LSP4E did this choice because they wanted to manage IFile, File and editor without file.

In IJ, VirtualFile is a File, but it exists an implementation LightVirtualFile which is independant of the file system (the content is in memory instead of get it from the file system), so I wonder if we could remove the Map<URI, DocumentContentSynchronizer> and uses just only List<VirtualFile> for the opened file.

I think it will simplify the code.

@jeffmaury what do you think about this idea?

@jeffmaury
Copy link
Member

Just wondering because they don't have the same scope if I remember correctly

@angelozerr
Copy link
Contributor Author

Just wondering because they don't have the same scope if I remember correctly

What do you mean with scope?

@angelozerr
Copy link
Contributor Author

angelozerr commented Aug 8, 2023

The motivation about changing Document to VirtualFile is when I'm trying to understand #1081 I read

https://plugins.jetbrains.com/docs/intellij/documents.html#how-long-does-a-document-persist

They say:

Storing Document references in long-term data structures of a plugin will cause memory leaks.

And today we store the Document.

@jeffmaury
Copy link
Member

Agree but when document is closed it should be evicted.

I mean Document has Project scope and VirtualFile has app/JVM scope.

So if the same file is opened in 2 different projects, you need to make sure LSP is properly setup with regards to editors

@angelozerr
Copy link
Contributor Author

So if the same file is opened in 2 different projects, you need to make sure LSP is properly setup with regards to editors

How to have this usecase? I'm trying to have this usecase for trying to have this issue #1081

Anyway, Language server accessor which creates the language servers is mapped with a project scope,so I think we will have no problem.

@jeffmaury
Copy link
Member

  1. Create 2 empty IJ project into 2 different folders
  2. Add a Maven module from an exiting Maven project into those 2 projects
  3. Open the same file from the 2 IJ projects

@angelozerr
Copy link
Contributor Author

Thanks!

It seems that Document instance are the same for the 2projects.

@jeffmaury
Copy link
Member

So maybe they have the same scope, getting too old

@angelozerr
Copy link
Contributor Author

I don't know.

In conclusion,are you OK to swith to VirtualFile?

@jeffmaury
Copy link
Member

I can't answer that would require me to look deeper into the code and do some testing.

But if they have the same scope that would be ok

@angelozerr
Copy link
Contributor Author

Ok thanks for your feedback @jeffmaury !

I will try it.

@angelozerr
Copy link
Contributor Author

While working on #1185 I notice that we spend our time to convert Document to VirtualFile (ex : to get the Uri of the document, we get the VirtualFile). In other words all code API are using Document but internally we convert it to VirtualFile to get some information like Uri.

@angelozerr angelozerr changed the title What about storing VirtualFile instead of Document for opened document? Storing VirtualFile instead of Document for opened document Sep 29, 2023
@angelozerr angelozerr added this to the 1.29.0 milestone Sep 29, 2023
angelozerr added a commit to angelozerr/intellij-quarkus that referenced this issue Sep 29, 2023
angelozerr added a commit to angelozerr/intellij-quarkus that referenced this issue Sep 30, 2023
angelozerr added a commit to angelozerr/intellij-quarkus that referenced this issue Sep 30, 2023
angelozerr added a commit to angelozerr/intellij-quarkus that referenced this issue Sep 30, 2023
angelozerr added a commit to angelozerr/intellij-quarkus that referenced this issue Sep 30, 2023
angelozerr added a commit to angelozerr/intellij-quarkus that referenced this issue Oct 1, 2023
angelozerr added a commit to angelozerr/intellij-quarkus that referenced this issue Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants