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

Update diagnostics when focusing an editor (DiagnosticsProvider is lazy) #2317

Merged
merged 3 commits into from
May 18, 2018

Conversation

SirIntruder
Copy link

I noticed I often have to insert random whitespace for vscode to update error reports, so I checked what extension is doing.

Currently, extension updates diagnostics on two events: onDidChangeTextDocument and onDidOpenTextDocument. onDidOpenTextDocument is called only when first openning an editor for a file (docs). If you modify a file "A.cs":

  • diagnostics inside changed file are updated almost instantly, because a document validation request was sent for "A.cs"
  • If project is below HugeProjectLimit, project-wide validation will be requested as well, so changes made in "A.cs" will propagate to other open editors ("B.cs", "C.cs" etc) with a couple of seconds of latency. depending on the size of the project.
  • If your project is above huge project limit, tough luck, outside of the modified file you have to make a random change (to invoke onDidChangeTextDocument ) to see updated diagnostics

This PR would add two vscode events: onDidChangeActiveTextEditor and onDidChangeWindowState, and call the same pathway as onDidOpenTextDocument/onDidChangeTextDocument when user focuses an editor. This both fixes issue with huge projects (without any project wide check, just updates info for the newly focused document) and makes diagnostics as snappy as possible.

Additional requests this PR would introduce are already invoked on every keystroke anyway, so server load should not increase meaningfully.

@dnfclas
Copy link

dnfclas commented May 16, 2018

CLA assistant check
All CLA requirements met.

@codecov
Copy link

codecov bot commented May 16, 2018

Codecov Report

Merging #2317 into master will increase coverage by 0.06%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2317      +/-   ##
==========================================
+ Coverage   59.79%   59.85%   +0.06%     
==========================================
  Files          83       83              
  Lines        3768     3774       +6     
  Branches      547      549       +2     
==========================================
+ Hits         2253     2259       +6     
- Misses       1347     1348       +1     
+ Partials      168      167       -1
Flag Coverage Δ
#integration 51.11% <71.42%> (+0.07%) ⬆️
#unit 82.82% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/features/diagnosticsProvider.ts 73.18% <71.42%> (+1.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9487d98...084f8a2. Read the comment docs.

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good change @SirIntruder. Thanks!

@DustinCampbell DustinCampbell requested a review from akshita31 May 18, 2018 15:33
@akshita31
Copy link
Contributor

@SirIntruder Thanks for your contribution. I pulled your branch and realized that because we have the onDidChangeWindowState every time we change the window - opening, closing , minimise, resize , etc we are doing the diagnostics request. Do you have a specific scenario in mind where the onDidChangeActiveTextEditor would not work and onDidChangeWindowState would be helpful?
Thoughts @DustinCampbell ?

@SirIntruder
Copy link
Author

I added onDidChangeWindowState as support for alt tabbing - that is useful for when you make changes from outside vscode and return, in my case usually git pull. I checked focus flag so it should not send request on minimizing or closing, but I was not aware it is invoked on resize as well. I could add a tracking flag so it would only be sent when focused is actually changed from false to true, would that be ok?

@akshita31
Copy link
Contributor

akshita31 commented May 18, 2018

@SirIntruder I set a breakpoint on https://github.com/OmniSharp/omnisharp-vscode/pull/2317/files#diff-f7f698ea628a7fd476ec3cd2355e1927R163 and it is being hit on minimize and closing as well. Can you try that?

@SirIntruder
Copy link
Author

SirIntruder commented May 18, 2018

@akshita31 It looks like I always get two onDidChangeWindowState events when I debug from vscode - always one windowState.focused === false, one windowState.focused === true. It looks like it is just issue with focus jumping to debugger and back. When I turned breakpoint into logpoint, it worked as you would expect (true only when returning to focus.)

I didn't debug this before, I compiled it with tsc and was printing out some logs to see what is going on - and according to logs it works like it should (focused == true only when returning to focus).

@akshita31
Copy link
Contributor

@SirIntruder Thanks for the analysis. Looks good to me.

@DustinCampbell
Copy link
Member

Thanks very much for the contribution @SirIntruder!

@DustinCampbell DustinCampbell merged commit 56267b1 into dotnet:master May 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants