godotengine / godot-vscode-plugin

Godot development tools for VSCode
MIT License
1.48k stars 148 forks source link

Various QoL Improvements #529

Closed DaelonSuzuka closed 7 months ago

DaelonSuzuka commented 8 months ago
sandygk commented 7 months ago

I can confirm this PR fixes https://github.com/godotengine/godot-vscode-plugin/issues/532

image
sandygk commented 7 months ago

I am seeing an issue on this branch that I am not seeing on master, I am getting no icons on the godot scene preview

here is how it looks on this branch

image

and this is master for reference

image

I even pulled master from this branch to make sure, but still got the same, maybe it's something I am doing wrong, or it may be a known issue

DaelonSuzuka commented 7 months ago

@sandygk Honestly that's pretty weird. This has happened to me but I seem to recall it was a local problem. Try stopping the "Watch" process in the terminal, deleting the out/ folder in the root directory, and restarting the extension host.

sandygk commented 7 months ago

@sandygk Honestly that's pretty weird. This has happened to me but I seem to recall it was a local problem. Try stopping the "Watch" process in the terminal, deleting the out/ folder in the root directory, and restarting the extension host.

I tried that, but it didn't fix it, I removed the out/ dir and ran npm run package and got the same result, when I change to master and run npm run package and install that version it works fine.

It's weird, because I noticed a similar comment was made for a different branch here, but I ran it after it was merged into master, and it works fine for me on master

sandygk commented 7 months ago

Ok so I tried to debug this, and I noticed something that may be of help:

If I run the extension in debug mode, it works fine

image

but if I run npm run package and install the .vsix, then I don't get the icons:

image
DaelonSuzuka commented 7 months ago

That makes a lot more sense! I changed some typescript settings recently, I bet that messed up the bundler. Should be an easy fix.

sandygk commented 7 months ago

Another thing I noticed in this branch: I have VSCode set up to format on save, with the main branch it doesn't do anything, but with this branch it tries to fix the formatting, but it breaks the indentantion in a bunch of places:

image

I think it may be replacing 8 spaces with 1 tab

DaelonSuzuka commented 7 months ago

Yeah the formatter is brand new and it's still somewhat experimental. Mixed tabs and spaces and correctly preserving indentation is something it doesn't handle well yet. As a stopgap I think I can just make it not touch leading whitespace at all, until I figure that out.

I have VSCode set up to format on save

I'm way too much of a paranoid control freak to ever do this. Imagine implicitly trusting some random jackass from the internet to not stealthily install a broken formatter in your text editor.

I think I'm going to put the formatter behind a setting for now, since I clearly have not been able to test it deeply enough.

sandygk commented 7 months ago

The last commit did fix the issue.

I'm using this branch for development. I'll post if I find any other issues with the formatter or something else.

sandygk commented 7 months ago

Mixed tabs and spaces and correctly preserving indentation is something it doesn't handle well yet. As a stopgap I think I can just make it not touch leading whitespace at all, until I figure that out.

I think it may be a good idea to rely on VSCode for that, it already has features to handle tabs and spaces:

image image

Maybe you can call that command as part of the formatter function.

I would love for that to be optional, if possible, I prefer using two spaces rather than a tab.

DaelonSuzuka commented 7 months ago

@sandygk The scene preview icons should be fixed now. The "Active Scene Tree" view in the debugger also had broken icons, which should be fixed too.

I'm not exactly sure what the problem was because I reverted my tsconfig changes and it didn't fix it. Best guess is that I've reorganized modules to use index.ts for cleaner imports, and that's causing the bundler to be invoked differently that just folders + files.

sandygk commented 7 months ago

@sandygk The scene preview icons should be fixed now. The "Active Scene Tree" view in the debugger also had broken icons, which should be fixed too.

Yup, it works!

sandygk commented 7 months ago

@DaelonSuzuka I found another issue with the formatter, it seems to have issues formatting lambda functions, it tends to remove more spaces than usual, and in some cases it just breaks the code, here is an example:

it turns this:

image

into this:

image

but if you write it like this, it works mostly fine, but it still removes the spaces around the ->:

image

Also, the return type for lambdas is not colored (see the images above), but that is happening in master as well, let me know if you want me to create a bug for that, or if this is already on your radar, and you want to tackle it here

DaelonSuzuka commented 7 months ago

let me know if you want me to create a bug for that, or if this is already on your radar, and you want to tackle it here

@sandygk Nope, reporting here is exactly right. This feedback is fantastic, so just keep it comin'!

DaelonSuzuka commented 7 months ago

@sandygk Try it now.

image

sandygk commented 7 months ago

Nice, that was fast!

It does fix the issue, but there are a couple of things I noticed:

It's adding spaces around the -> for the lambdas, but now it's removing the spaces for the methods:

image

(notice how the lambda in the last line is the only one that's working now, it basically inverted it)

And the example I gave above is working, but now for some reason, node is not colored:

image

but if I write it like this, it does work fine, as before:

image
DaelonSuzuka commented 7 months ago

Once more, with feeling.

sandygk commented 7 months ago

image working like a charm! 😃

DaelonSuzuka commented 7 months ago

New things added today

Go to Definition now correctly scrolls to the symbol name.

Code_pnOxz7hvPA

I'm partway through fixing broken code boxes in hovers and docs.

Inlay hints to help reading scene files:

Code_YYyYfShnDu

Go to Definition now works in scene files:

Code_lf83ELz3qV

sandygk commented 7 months ago

I noticed another minor issue, I think it was introduced recently, it seems to be adding one space before not where it shouldn't

image
DaelonSuzuka commented 7 months ago

This looks like it'll be a harder one to untangle without a deeper rewrite than I'd like. In the meantime you could use ! instead of not?

sandygk commented 7 months ago

That's ok, do you want me to create a bug for it once the PR is merged?

DaelonSuzuka commented 7 months ago

No, I'm still gonna fix it, it just won't be within the hour like I usually manage.

sandygk commented 7 months ago

it just won't be within the hour like I usually manage

image

to think I trusted you and this is how you choose to pay me, the nerve

DaelonSuzuka commented 7 months ago

@sandygk I completely rewrote the formatter. This new version uses the TextMate engine for parsing, and it's able to use the GDScript grammar we already use for syntax highlighting.

As unexpected but pleasant surprise, I immediately found several bugs in the GDScript syntax highlighting.

sandygk commented 7 months ago

@DaelonSuzuka, nice, I found a couple of issues with the new version of the formatter, here are undesirable changes introduced by the new formatter on my current project, some even break the code.

image image image image image image image image image image image image image image

It does fix the issue with the space before not though

Removing the space after await seems to happen every time and introduces a compilation error

DaelonSuzuka commented 7 months ago

Fixed:

I don't think Utils.chance(1, /3) is valid syntax

Having less whitespace inside function parens is intentional.

sandygk commented 7 months ago

Utils.chance(1, /3)

It's a dot after the 1, not a comma. It's to make it a float, the same as 1.0, but I just do 1., this syntax is valid

DaelonSuzuka commented 7 months ago

It's to make it a float, the same as 1.0, but I just do 1., this syntax is valid

What an absolute madlad.

print(1. / 3)

This works now too.

sandygk commented 7 months ago

Here are the issues I am getting now on my project:

image image image image image image image image image image image image image image

notice that the color for func in lambdas is incorrect

sandygk commented 7 months ago

When you do func() it colors it well, but if you do func () the color breaks

sandygk commented 7 months ago

It would be good if you could get your hands on large godot 3 and 4 repos to run the formatter on, that way you can check the diff that it gives you as a form of testing. If you want I can send you mine, but it's not that large to be honest.

sandygk commented 7 months ago

We can also build a GDScript for v3 and v4 with all the syntax that we can think of, and make sure we format it correctly. We can then run the formatter on it and use it as a unit test. And we could keep growing it every time we fix a bug in the formatter to include the syntax that triggers the bug. At the beginning it won't be perfect, but if we keep this practice, it could become really robust in no time, and is a solid guarantee that fixing a bug is not creating another, at least not re-introducing an old bug, because that one would be contemplated in the script.

I can work on that if you want.

DaelonSuzuka commented 7 months ago

We can also build a GDScript for v3 and v4 with all the syntax that we can think of, and make sure we format it correctly. We can then run the formatter on it and use it as a unit test. And we could keep growing it every time we fix a bug in the formatter to include the syntax that triggers the bug.

There are already some example files in syntaxes/examples that I've used to manually check for highlighting regressions. I borrowed chunks from the engine docs and added cases when I fixed bugs.

src/formatter/formatter.test.ts has the first test I've ever written for this project. It's not good but it's proof that the harness/runner works.

src/formatter/tests/test1.in.gd and src/formatter/tests/test1.out.gd are a random thought for how these tests could be organized. I have no clue if they're going to be a good idea or not.

I can work on that if you want.

That would be awesome!

I think the easiest way to collaborate on that is for you to make a branch and open a PR, so let me clean up a few things here and then I can merge this (this PR has lived too long anyways).

sandygk commented 7 months ago

I think the easiest way to collaborate on that is for you to make a branch and open a PR, so let me clean up a few things here and then I can merge this (this PR has lived too long anyways).

Sounds good, let's do that!

DaelonSuzuka commented 7 months ago

Big refactor of the docs manager:

Code_ReQ8qECsKf

DaelonSuzuka commented 7 months ago

I've rewritten the documentation handler. The previous "Open Godot Documentation" context menu command has been removed, everything is handled via go-to-def and .gddoc custom file handling.

It works for me when I move the cursor with arrow keys and hit F12, so please let me know if there's any issues with your vim movements, @levidavidmurray

As a cool bonus, we get this behavior:

Code_tqplQfnTsy

Because the documentation is now a virtual custom file, reloading the editor will cause vscode to automatically reload the docs, too (assuming that it connects to the LSP in a timely manner, of course).

levidavidmurray commented 7 months ago

It works for me when I move the cursor with arrow keys and hit F12, so please let me know if there's any issues with your vim movements, @levidavidmurray

@DaelonSuzuka No issues at all—it works perfectly! Thank you very much for all your hard work! 🚀

I'm curious if there's any reason the VSCode Marketplace extension hasn't been updated in over a year? Is the original publisher (Geequlim?) just not around to do it?

DaelonSuzuka commented 7 months ago

I've been working on the debugger for basically that entire time and hadn't managed to get anything polished enough to release. This PR is probably the last major item that's going into the next version, and I really want to get that out this week.

Your effort with testing and redesigning the go-to-def provider was actually a big help, so thanks again for that!