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

Finish becoming an LSP client and porting to Serenata 5.0 #460

Closed
20 tasks done
Gert-dev opened this issue Dec 17, 2018 · 3 comments
Closed
20 tasks done

Finish becoming an LSP client and porting to Serenata 5.0 #460

Gert-dev opened this issue Dec 17, 2018 · 3 comments

Comments

@Gert-dev
Copy link
Owner

Gert-dev commented Dec 17, 2018

The branch feature/lsp-client currently contains all the work towards becoming an LSP client that works with the master branch of the Serenata server - which is pretty much a working LSP server now.

There are still some things left to do on the client side here, which is mostly cleanup and bugfixing. I'll just drop a list of these here:

  • Readd catching fatal server errors to Proxy and displaying notifications or logs about them
    • Automatically attempt to restart server when it crashes - this may be possible with atom-languageclient via client.restartAllServers()
  • Start/(re)initialize the server when there was no project to start it for before and the user has now set it up via Set up current project
  • Fix setting up a new project failing if the .serenata folder does not exist yet in the project folder (simply create that intermediate directory when needed)
  • Reimplement Reindex project, we may be able to do that by simply calling client.restartAllServers() because the server will rescan the project on every initialization (we asked this from the client end before)
  • Investigate if switching projects properly restarts (reinitializes) the server for the new project; we're just looking at the first project folder, so if the new first project folder is another project (or none at all), the server needs to restart (close)
    • Works as long as you open (or close and reopen) a PHP file afterwards, as the server doesn't appear to be started by atom-languageclient until that happens (the previous one is properly shut down on project switch, though)
  • Clean up base package and remaining things that are commented out by reimplementing them or removing them - preferably everything but the legacy annotations and refactoring code should be using proper ES6 classes now; we won't bother with the legacy code because it's going to be replaced in the future anyway
  • Document in CHANGELOG that project-manager is no longer required as a dependency
  • Document in CHANGELOG that autocompletion now obeys the minimumWordLength setting from the autocomplete-plus package - if users want autocompletion to immediately trigger for every character, as was the case before, change this setting to 0
  • Reenable server installation via Composer - currently still commented out whilst migrating everything to the new SerenataClient class
    • Working successfully, need to test setting up new projects and using existing projects right after installation properly after updating server version to 5.0.0-RC2 (or whatever version comes next), as currently the "Interface doesn't exist" bug that was already fixed upstream is still present
  • Provide upgrade path for existing projects - alternatively, if this is too hard, impossible or too obtrusive (as we need to create new files), show a notification that asks users if this is okay or that they need to rerun Set up current project
  • Send project file via initializationOptions once server supports it
  • When trying to perform code navigation on qualified class names, such as A\B, only part of the name will be underlined
    • This happens because the server cannot officially indicate the range of the symbol and atom-languageclient thus determines the range of the symbol by itself based on a list of "word separators"; it will also try to fetch document symbols to determine it first, so this can also be fixed by implementing textDocument/documentHighlight in the server - at least for qualified class, constant and function names anyway - and enabling the code-highlight service here
  • The client sends a lot of getClassListForFile events on file open, which probably yield the same results and aren't cached by client nor server. We should cache this client- or server-side
    • Another alternative is to have the server support "code lenses" and use Atom's inline decoration functionality to show something like "Override" or "Implementation" above the methods and properties. This way the server can generate more of these in the future and we only have one request and response. See also this Serenata ticket.
  • Autocompletion for docblock tags such as @inheritDoc does not seem to replace the existing @, it is likely the replacementPrefix is not set correctly (or the server is providing the wrong textEdit that does not include it in its range)
  • Autocompletion suggestions returned by the server are being additionally filtered and resorted incorrectly by the client
  • Errors are shown when requests are cancelled, perhaps the server isn't returning a valid response code for cancelled requests. Should investigate further what the specification says and if we are doing something wrong in the client
    • Nevermind, I think we're doing everything right. The languageclient library reports that the response is successfully cancelled, it then just proceeds to log a (spurious) error message. I reported this upstream, but it doesn't break anything, so I'll leave it as-is for now.
  • The server is closed when closing the last PHP file, which then shows a socket error. This is a valid shutdown event and should not show any errors
  • There appears to be a bug somewhere where the atom-languageclient closes the server due to "Stopping unused servers", even though there are still files open. I could reproduce this earlier, but I can't anymore now.
Gert-dev added a commit that referenced this issue May 18, 2019
This also cleans up the server startup in Proxy since most of the 
restarting logic is now handled by the Atom language client.

References #460
Gert-dev added a commit that referenced this issue May 18, 2019
Gert-dev added a commit that referenced this issue May 18, 2019
Gert-dev added a commit that referenced this issue May 19, 2019
Gert-dev added a commit that referenced this issue May 19, 2019
Gert-dev added a commit that referenced this issue May 19, 2019
Gert-dev added a commit that referenced this issue May 19, 2019
Gert-dev added a commit that referenced this issue May 19, 2019
Gert-dev added a commit that referenced this issue May 19, 2019
Gert-dev added a commit that referenced this issue May 19, 2019
Gert-dev added a commit that referenced this issue May 19, 2019
Gert-dev added a commit that referenced this issue Jun 2, 2019
These have been redundant for a long time, we have been warning about 
them for quite some time, and the name of the package has changed; there 
should now be little confusion as to whether these packages are still 
relevant for this one.

References #460
Gert-dev added a commit that referenced this issue Jun 2, 2019
Gert-dev added a commit that referenced this issue Jun 2, 2019
Gert-dev added a commit that referenced this issue Jun 2, 2019
Gert-dev added a commit that referenced this issue Jun 2, 2019
Gert-dev added a commit that referenced this issue Jun 2, 2019
Gert-dev added a commit that referenced this issue Jun 2, 2019
Apparently the other one I updated was an unused property.

References #460
Gert-dev added a commit that referenced this issue Jun 2, 2019
Gert-dev added a commit that referenced this issue Jun 2, 2019
Gert-dev added a commit that referenced this issue Jun 4, 2019
Gert-dev added a commit that referenced this issue Jun 4, 2019
Gert-dev added a commit that referenced this issue Jun 4, 2019
Gert-dev added a commit that referenced this issue Sep 2, 2019
This replaces the annotations we had with code lenses, which are 
displayed as inline markers that do not interfere with the code itself. 
These look like badges that can be clicked, much like gutter annotations 
that can be clicked.

The advantage of lenses is that they aren't almost invisible in the 
gutter and they support arbitrary commands via the server. The ones 
we're supporting now just open files, but the server could add lenses in 
the future that perform refactoring actions or do other tasks.

This only wires in retrieving them from the server and displaying them 
in the editor. A click handler is already present, but I have yet to 
request that the server execute the command when that happens (and to 
handle the server asking to open a file at a specific location, as a 
result).

References #460
Gert-dev added a commit that referenced this issue Sep 3, 2019
Gert-dev added a commit that referenced this issue Sep 3, 2019
Gert-dev added a commit that referenced this issue Sep 3, 2019
This caused the editor to get a new "long title", thus causing the 
previous lenses to never be cleaned up.

References #460
Gert-dev added a commit that referenced this issue Sep 4, 2019
This can happen when multiple properties are on one line and more than 
one is an override.

References #460
@nelson6e65
Copy link

Hi. Thanks for your awesome work, @Gert-dev. I'm using your "gratis" tool since some months (years?) ago and its an amazing tool.

I found this when I was just about to start a kind of minimal tool for this, and I'm glad I found this wonderful tool.

I just want to say muchísimas gracias. I mean, I know working on such opensource tools is hard, specially because of time... time you could be using in your payed job. Thanks for keeping this alive. I hope I can donate soon, al least a bit in bit, because I feel that just saying "thank you" is not enough when I used for a long time in all my PHP projects. 😅

I hope you can continue your marvelous work 🥇 keeping this tool alive. ¡Gracias! 🎉

Gert-dev added a commit that referenced this issue Sep 9, 2019
I wanted to place the index inside the .serenata folder, but it turns 
out that is a very bad idea. Atom will start firing massive amounts of 
change requests, due to it watching the database file, every time it is 
modified. We can disable this for our package, but not for other 
packages, which will still receive these events en masse uselessly, 
which not only prevents any other responses from being handled in the 
meantime, it also spikes CPU usage.

Existing indexes will continue to work, this just sets the default for 
new projects to be the php-ide-serenata cache folder. You can still opt 
to save them in the project, by changing it, if you want.

References #460
Gert-dev added a commit that referenced this issue Sep 14, 2019
Gert-dev added a commit that referenced this issue Sep 14, 2019
Gert-dev added a commit that referenced this issue Sep 14, 2019
Gert-dev added a commit that referenced this issue Sep 14, 2019
This prevents closing it and reopening it every time the installation 
fails and also makes it consistent with the "Test my setup" flow.

References #460
Gert-dev added a commit that referenced this issue Sep 14, 2019
Gert-dev added a commit that referenced this issue Sep 14, 2019
I accidentally uploaded the wrong Serenata files, I've corrected that 
now.

References #460
Gert-dev added a commit that referenced this issue Sep 14, 2019
This is already fixed upstream, but not in the PHAR we download.

References #460
Gert-dev added a commit that referenced this issue Sep 14, 2019
They shouldn't be sent until the server is properly set up. I was 
apparently checking for the wrong condition.

References #460
@Gert-dev
Copy link
Owner Author

Gert-dev commented Sep 14, 2019

If someone is using Windows or macOS with this package and following this, it would be neat if someone could test the latest version. It should be fairly easy;

  1. Grab the latest master.
  2. Put it in your Atom packages folder (be sure to temporarily move and disable the stable version).
  3. Run apm install in the folder.
  4. Start Atom.

Does installation work? Do things such as autocomplete work? It should work the same way as it did before (except for intended changes, of course), but where I expect problems to occur:

  • Installations using Docker for running on macOS.
  • Problems causing projects to not index properly or the index database being mislocated due to file URIs on Windows as they look different than on macOS and Linux.
  • Installations using Docker for running on Windows, especially because paths and URIs need to be converted.

I want to release at some point in the future, but I want to avoid releasing a version that works fine on Linux but breaks on pretty much every other platform because I have no access to these, which will be frustrating to users.

@nelson6e65 Thanks for your kind words!

I admit I'm not always equally motivated to work on Serenata, but I've been using it myself since the start as well (even where I work), so that kept me hanging on in those moments.

I still like working on Serenata because there is a lot of potential feature-wise, especially refactoring. The other edge of the sword is that there is always a lot to do in other areas as well 😄 .

@Gert-dev
Copy link
Owner Author

Well, since no one has taken me up on my request or reported any bugs, I'm assuming either no one found any glaring bugs or no one tested (likely the latter).

I can hardly delay the next version using Serenata 5.0 indefinitely so I'm going to be releasing it soon, total breakage for users be damned - I don't like it, but it is how it is, I guess - I don't have access to Windows or macOS, so I can't test either of them.

I moved the "unused" servers issue to #475 and am closing this issue as the initial migration is complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants