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

Fix CodeLens positions being off by one while editing and reference count being wrong #59

Merged
merged 8 commits into from Nov 5, 2018

Conversation

@uwx
Copy link
Contributor

uwx commented Nov 3, 2018

I made a couple fixes to the CodeLens support which is by far my favorite feature of this extension. The code is quite messy and I couldn't get all of the tests to pass (but the results were consistent between both patched and unpatched versions)

Changes

  • Fixes CodeLens positions being off by one when editing a file
  • Fix codelens reference count being 0 initially until a document change happens
    • This was fixed by adding a runSemanticAnalysisIfNeeded call to getReferenceCount, matching some of the other methods. I'm not sure if there's any other places where the call should be added as well.

* Add "fast" variants of facade methods
* These variants don't reload the file from disk, instead reusing VSCode's document when possible.
This could fix bugs where the extension loads the disk (old) version of a dirty file until the file is changed (workspace.onDidChangeTextDocument gets triggered), but I didn't find any such bugs during testing.
* This is the messiest part of the patch. It could maybe be improved by adding an optional fileContents: string parameter to the existing methods rather than creating "fast" variants of them, but for a quick and dirty fix I didn't want to leave any room for missed calls that should use the fast variant but don't do so.

  • Add a missing semicolon
    • This is by far the most important part of the patch. It does not change anything functionally.
uwx added 6 commits Nov 3, 2018
These variants don't reload the file from disk, instead reusing VSCode's document when possible.
This could fix bugs where the extension loads the disk (old) version of a dirty file until the file is changed (workspace.onDidChangeTextDocument gets triggered), but I didn't find any such bugs during testing.
@uwx

This comment has been minimized.

Copy link
Contributor Author

uwx commented Nov 3, 2018

I just noticed I missed a backend.loadGrammar call without getText in onDidOpenTextDocument but it shouldn't make much of a difference.

@mike-lischke

This comment has been minimized.

Copy link
Owner

mike-lischke commented Nov 4, 2018

@uwx thanks for taking care to fix issues related to the code lens feature. Looking at the patch I wonder if you have seen that the editor content is usually not passed via the filename to the backend APIs. The filename is most of the time just the key to look up the appropriate source context. Perhaps I should rename the fileName parameter to something more neutral like contextKey?

Instead the backend is updated via setText everytime something changed and so always up to date (which is mandatory to make code completion correctly working). There's simply no need to pass the text also in most of the calls to the backend. Thus, all the *Fast() functions are superfluous.

@uwx

This comment has been minimized.

Copy link
Contributor Author

uwx commented Nov 4, 2018

I see. I created the fast versions of the methods for the ones that call getContext, which calls loadGrammar if it can't find the entry in sourceContexts, which then reads the file from disk. This only happens once, since, as you said, the contexts get updated by setText afterwards, so I don't think there are any serious performance implications from reading the file from disk vs using document.getText().

However, I wonder if there's any inconsistency potentially introduced by reading the file from disk if the version on disk is outdated, say if the contents of the document being edited are different and unsaved. I'm fairly new to VSCode so I don't know if that's a situation that can happen. (Maybe when the document is edited just before the extension is loaded?)

I'll restore the old behavior of the context methods in a bit. I fixed a couple other things since making this PR, but it's already getting pretty big, so I'll save those for another PR.

uwx added 2 commits Nov 4, 2018
…es in comments, method positions and method visibility"

This reverts commits 5083674 and 26050b5.

The editor content is usually not passed via the filename to the backend APIs. The filename is most of the time just the key to look up the appropriate source context. Therefore, the *Fast() functions are superfluous.
@uwx uwx changed the title Fix CodeLens positions being off by one while editing, reference count being wrong + internal changes Fix CodeLens positions being off by one while editing and reference count being wrong Nov 4, 2018
@mike-lischke

This comment has been minimized.

Copy link
Owner

mike-lischke commented Nov 5, 2018

Yes, the original idea was to implicitly load the file content if not yet done when any of the APIs is called. This is probably no longer necessary, since the backend is no longer a dedicated library (as it was a while ago). Now you always load the file upfront and continuously update the backend on each edit action, to stay current all the time. From this approach I cannot see where an inconsistency could happen. The time from opening a document to the moment the extension is loaded and ready is pretty short usually. The file is loaded immediately and the content in memory constantly updated.

@mike-lischke mike-lischke merged commit c9e78eb into mike-lischke:master Nov 5, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.