-
Notifications
You must be signed in to change notification settings - Fork 337
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
Add file system support for jars #3750
base: main
Are you sure you want to change the base?
Conversation
Wow! This looks awesome! This could replace the packages view in Metals tree view if I understand correctly? Does this show up in the file explorer?
I would say this is the only blocking thing, very similar to the notebook issue where we have another URI pattern that is not possible for Metals to recognise.
I think this doesn't work currently also.
It's not possible currently, so this would be fine.
Do you mean the focused document?
IS there any update sent to Metals about it? Maybe we could just show an error message and ask to reload? |
Having breadcrumbs in dependency files seems awesome @Arthurm1 ! However, after a bit of plating, I spotted one issue. Namely, the fact that the user has to deal with an additional workspace may be problematic:
I wonder if we could implement this feature in a more transparent way from the user perspective and it seems like we could
After looking at https://marketplace.visualstudio.com/items?itemName=sourcegraph.sourcegraph and https://github.com/olafurpg/sourcegraph-vscode/blob/diffs-view/src/file-system/FilesTreeDataProvider.ts it seems like we could replace Metals tree view and have breadcrumbs without adding a new workspace folder. I will even move the package view from Metals tab to the |
Yes - it appears in the main file explorer as a new workspace folder below the main workspace folder. It could appear at the top but when you change the first workspace folder then VSCode restarts the extensions which isn't a great UI feel.
I agree - this is the main blocker. I'm starting to think that using the real jar URI
No but you can navigate directly to a class file through the file explorer now so it would be good to implement it and it's fairly simple since one of the methods you have to implement on the filesystem api is
I mean when you open a jar document (e.g. through
I'll check - probably. |
Having a custom tree view is another option. It would certainly get round the issue of URIs not matching between the file explorer and Metals as it wouldn't rely on the URI to match the tree. Maybe I should give the custom tree view a go and compare the results |
I'm going to close this since the way the FileSystem API makes the tree UI exactly reflect the URI makes it unworkable for the way Metals wants to use URIs and the way we want to display libraries. |
The initial approach in the PR description looked pretty awesome to me, why not revert to that? |
@tgodzik I closed this because I couldn't see an easy way to match off the 2 different URI formats (i.e. the one Metals uses to identify classes in jars and the one the VSCode filesystem api uses). Instead I thought this would be better implemented using a custom treeview But now I see that breadcrumbs only appear when the filesystem supports them so using the filesystem API is the only way to get breadcrumbs on jar dependencies. see here Currently Metals understands dependencies when they are in I'm going to look at changing the way Metals understands jar URIs to match the URI on the filesystem API. So something like: Note the lack of This should be viable because it will only affect clients that support virtual documents. The content of the virtual documents is supplied by Metals so the format of the URI shouldn't matter to the client. The issue I think I'm going to run up against is that I think there are 2 options here:
I'm leaning towards the 2nd option as it wouldn't affect any other code and should cover both source files and class files that need decompiling. Unless you can think of an easier option. |
@Arthurm1 I recently got back from vacations and only know managed to take a look at your response, sorry! Looks like a second solution is better, though I am still trying to figure out if we can make it any simpler 🤔 |
@tgodzik No worries - glad you've had a break. I haven't been able to think of another option so I've gone ahead with a I've separated out jdks, source jars, workspace jars into subdirs (but that's just my preference - Metals can now dictate the folder structure in the file explorer) The user can navigate using I've (nearly) got auto-decompiling and interactive semanticdb working for Still a lot to do though and I have to support readonly directory for those non-VSCode legacy clients 😉 metals-1649865855473.mp4 |
whoa whoa whoa 😄 |
2cc3fb5
to
1468162
Compare
I think this is now almost functionally complete. There's a lot of code I need to tidy and many tests to write but it would be good to get some feedback on whether the approach is good. By that I mean both how the UI works and whether the custom URL handler and custom filesystem I've had to write are overkill or disrupt something I haven't thought of. There's probably some optimisation needed as well. This is linked to scalameta/metals-vscode#963 if you'd like to try it out. Please do. After initial indexing it should populate the VSCode file explorer with a new file system root which may (on the first time only) cause your VSCode to reload. I'm not sure yet whether this is avoidable. For non-vscode clients, the readonly stuff should work as usual and they shouldn't even be aware that metals is using a |
@tgodzik When you have a moment - could you have a look through this and see whether you're happy with the approach regarding custom java filesystem and custom url handler. You may decide it's too invasive. It's not urgent. It's also easy enough to try out yourself with the vscode PR as well. Code's a bit of a mess but I thought I'd stop here in case you're dead against it. |
It's on my list! I will take a look 😅 |
Maybe we could remove the added folder in |
I will try to see what's going on!
Damn, it would be good to be able to be figure out a way around it. I will take a look.
That could be a good idea, I can also take a look if you don't have time?
I will investigate this as soon as I can. Thanks for the great work! I showed your work in a couple places and people would love to have it I think 🎉 |
@tgodzik I haven't had much time recently and I'm off on holiday for a few weeks. I don't think there's much to do in terms of functionality - just some finessing on the points you've raised. I have got a chunk of tests to write though which I'm slowly getting through. 😞 |
@Arthurm1 actually, could we avoid adding the workspace. Everything works really nicely without it and I don't get any issues, any at all really. Using Tree View works pretty well in the case and we could just make it work with reveal command. We wouldn't see files, but I think it's overall a better experience. And we can improve the tree view further to also show jars etc. Btw. I can finish up with the tests if you don't find the time. Anyway, this is pretty exciting! |
@tgodzik I agree. I think the switch to multi-workspace when adding |
That should be fine as long as it's not the default and we are not forcing the users. And it might be pretty useful to some. I like the experience of the separate file system provider. Would be cool to replicate this in tree view, but not sure if that is currently possible. |
Hi! What's the status for this change? |
@jeengbe I haven't looked at it in a long time. I got the performance issues sorted but still had issues with |
An experiment in using the VSCode FileSystem API to explore library dependencies
Here's an example....
Stuff that works...
Stuff that doesn't work...
Weird stuff
Metals - libraries
workspace folder. Can't see a way to stop this. It doesn't cause an error, it's just that the functionality is then lost. It would be re-added onReload Window
FileSystem API uses URIs to reference files. With the flat approach taken here (i.e.
root/all_jars/XXX
path) the real jar URIs will not match with the flat URIs.So
metalslibs:/scala-library-2.13.8-sources.jar!/scala/io/source.scala
!=jar:file:///localRepoPath/scala-library-2.13.8-sources.jar!/scala/io/source.scala
Because of this VSCode thinks these are 2 different files. Metals also doesn't recognise the former URI and code would have to be added to interpret this URI in the same way the
readonly
directory is handled. Even so - VSCode would still think these were different files.Another option would be to take a non-flat structure and have the real directory structure leading to all the jars (albeit only showing dirs with jars in). I haven't checked if this will actually work as VSCode corrupts URIs given to it (e.g. root has to ==
/
when I'd like it to befile:///
).This probably wouldn't be as easy to use and I'm still exploring if it's possible.
@kpodsiad Here's a working, but limited, jar explorer.